Fix incorrect handling of module runtime and environment pointers.

We used to store module runtime and environment pointers in the static
lists Vmodule_runtimes and Vmodule_environments.  However, this is
incorrect because these objects have to be kept per-thread.  With this
naive approach, interleaving module function calls in separate threads
leads to environments being removed in the wrong order, which in turn
can cause local module values to be incorrectly garbage-collected.

Instead, turn Vmodule_runtimes and Vmodule_environments into
hashtables keyed by the thread objects.  The fix is relatively
localized and should therefore be safe enough for the release branch.

Module assertions now have to walk the pointer list for the current
thread, which is more correct since they now only find environments
for the current thread.

Also add a unit test that exemplifies the problem.  It interleaves two
module calls in two threads so that the first call ends while the
second one is still active.  Without this change, this test triggers
an assertion failure.

* src/emacs-module.c (Fmodule_load, initialize_environment)
(finalize_environment, finalize_runtime_unwind): Store runtime and
environment pointers in per-thread lists.
(syms_of_module): Initialize runtimes and environments hashtables.
(module_assert_runtime, module_assert_env, value_to_lisp): Consider
only objects for the current thread.
(module_gc_hash_table_size, module_hash_push, module_hash_pop): New
generic hashtable helper functions.
(module_objects, module_push_pointer, module_pop_pointer): New helper
functions to main thread-specific lists of runtime and environment
pointers.
(mark_modules): Mark all environments in all threads.

* test/data/emacs-module/mod-test.c (Fmod_test_funcall): New test
function.
(emacs_module_init): Bind it.

* test/src/emacs-module-tests.el (emacs-module-tests--variable): New
helper type to guard access to state in a thread-safe way.
(emacs-module-tests--wait-for-variable)
(emacs-module-tests--change-variable): New helper functions.
(emacs-module-tests/interleaved-threads): New unit test.

2 files changed, 60 insertions, 0 deletions

diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index 8d1b421bb40..528b4b4c582 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -547,6 +547,14 @@ Fmod_test_double (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return result;
 }
 
+static emacs_value
+Fmod_test_funcall (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
+                   void *data)
+{
+  assert (0 < nargs);
+  return env->funcall (env, args[0], nargs - 1, args + 1);
+}
+
 /* Lisp utilities for easier readability (simple wrappers).  */
 
 /* Provide FEATURE to Emacs.  */
@@ -629,6 +637,8 @@ emacs_module_init (struct emacs_runtime *ert)
   DEFUN ("mod-test-add-nanosecond", Fmod_test_add_nanosecond, 1, 1, NULL, NULL);
   DEFUN ("mod-test-nanoseconds", Fmod_test_nanoseconds, 1, 1, NULL, NULL);
   DEFUN ("mod-test-double", Fmod_test_double, 1, 1, NULL, NULL);
+  DEFUN ("mod-test-funcall", Fmod_test_funcall, 1, emacs_variadic_function,
+         NULL, NULL);
 
 #undef DEFUN
 
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 9df0b25a0c5..f9bd82e78c6 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -419,4 +419,54 @@ Interactively, you can try hitting \\[keyboard-quit] to quit."
     (ert-info ((format "input: %d" input))
       (should (= (mod-test-double input) (* 2 input))))))
 
+(cl-defstruct (emacs-module-tests--variable
+               (:constructor nil)
+               (:constructor emacs-module-tests--make-variable
+                             (name
+                              &aux
+                              (mutex (make-mutex name))
+                              (condvar (make-condition-variable mutex name))))
+               (:copier nil))
+  "A variable that's protected by a mutex."
+  value
+  (mutex nil :read-only t :type mutex)
+  (condvar nil :read-only t :type condition-variable))
+
+(defun emacs-module-tests--wait-for-variable (variable desired)
+  (with-mutex (emacs-module-tests--variable-mutex variable)
+    (while (not (eq (emacs-module-tests--variable-value variable) desired))
+      (condition-wait (emacs-module-tests--variable-condvar variable)))))
+
+(defun emacs-module-tests--change-variable (variable new)
+  (with-mutex (emacs-module-tests--variable-mutex variable)
+    (setf (emacs-module-tests--variable-value variable) new)
+    (condition-notify (emacs-module-tests--variable-condvar variable) :all)))
+
+(ert-deftest emacs-module-tests/interleaved-threads ()
+  (let* ((state-1 (emacs-module-tests--make-variable "1"))
+         (state-2 (emacs-module-tests--make-variable "2"))
+         (thread-1
+          (make-thread
+           (lambda ()
+             (emacs-module-tests--change-variable state-1 'before-module)
+             (mod-test-funcall
+              (lambda ()
+                (emacs-module-tests--change-variable state-1 'in-module)
+                (emacs-module-tests--wait-for-variable state-2 'in-module)))
+             (emacs-module-tests--change-variable state-1 'after-module))
+           "thread 1"))
+         (thread-2
+          (make-thread
+           (lambda ()
+             (emacs-module-tests--change-variable state-2 'before-module)
+             (emacs-module-tests--wait-for-variable state-1 'in-module)
+             (mod-test-funcall
+              (lambda ()
+                (emacs-module-tests--change-variable state-2 'in-module)
+                (emacs-module-tests--wait-for-variable state-1 'after-module)))
+             (emacs-module-tests--change-variable state-2 'after-module))
+           "thread 2")))
+    (thread-join thread-1)
+    (thread-join thread-2)))
+
 ;;; emacs-module-tests.el ends here