bug#30846: 26.0.91; debug-watch of kill-all-local-variables triggers 'assertion failed: found == !EQ (blv->defcell, blv->valcell)'

[Stefan Monnier <monnier@IRO.UMontreal.CA>]

> This results in
>
> ../../src/data.c:98: Emacs fatal error: assertion failed: found == !EQ (blv->defcell, blv->valcell)

OK, I found the culprit: in kill-all-local-variables, we first change
all the (relevant) symbols to point to their global value (with
swap_in_global_binding called from swap_out_buffer_local_variables),
then go and kill them one by one (in reset_buffer_local_variables).
This worked fine in the past, but now that we run watchers while we kill
the vars, the act of running the watchers may undo the effect of
swap_in_global_binding, so we can't kill them quite in the same way.

The patch below against emacs-26 seems to fix the problem (it mostly
merges the code of swap_out_buffer_local_variables into that of
reset_buffer_local_variables so that the swap_in_global_binding is done
just before we actually kill the var, with no opportunity for watchers
to undo the effect).

The patch doesn't only fix this problem, it also changes the time at
which we run the watcher: in the current emacs-26 code,
kill-all-local-variables runs the watcher *after* killing the
corresponding var, whereas usually the watchers are run *before* the var
is modified.  I can split the patch into two, if you want and/or only
apply the part that actually fixes this bug.

If you feel this is too risky for emacs-26, I wouldn't blame you (this
is pretty tricky code): while the assertion crashes Emacs, a normal
build without assertions will likely not notice the problem at all.
I came up with a test case that catches the problem, but I think that in
"real" life it's very unlikely to cause a problem.

It applies unchanged to master (and while looking at this bug I found
a whole bunch of other minor changes that I plan to install into master
anyway).


        Stefan


diff --git a/src/buffer.c b/src/buffer.c
index 9b54e4b778..b0cee71703 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -108,7 +108,6 @@ int last_per_buffer_idx;
 static void call_overlay_mod_hooks (Lisp_Object list, Lisp_Object overlay,
                                     bool after, Lisp_Object arg1,
                                     Lisp_Object arg2, Lisp_Object arg3);
-static void swap_out_buffer_local_variables (struct buffer *b);
 static void reset_buffer_local_variables (struct buffer *, bool);
 
 /* Alist of all buffer names vs the buffers.  This used to be
@@ -991,10 +990,29 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
   else
     {
       Lisp_Object tmp, last = Qnil;
+      Lisp_Object buffer;
+      XSETBUFFER (buffer, b);
+
       for (tmp = BVAR (b, local_var_alist); CONSP (tmp); tmp = XCDR (tmp))
         {
           Lisp_Object local_var = XCAR (XCAR (tmp));
           Lisp_Object prop = Fget (local_var, Qpermanent_local);
+          Lisp_Object sym = local_var;
+
+          /* Watchers are run *before* modifying the var.  */
+          if (XSYMBOL (local_var)->u.s.trapped_write == SYMBOL_TRAPPED_WRITE)
+            notify_variable_watchers (local_var, Qnil,
+                                      Qmakunbound, Fcurrent_buffer ());
+
+          eassert (XSYMBOL (sym)->u.s.redirect == SYMBOL_LOCALIZED);
+          /* Need not do anything if some other buffer's binding is
+	     now cached.  */
+          if (EQ (SYMBOL_BLV (XSYMBOL (sym))->where, buffer))
+	    {
+	      /* Symbol is set up for this buffer's old local value:
+	         swap it out!  */
+	      swap_in_global_binding (XSYMBOL (sym));
+	    }
 
           if (!NILP (prop))
             {
@@ -1034,10 +1052,6 @@ reset_buffer_local_variables (struct buffer *b, bool permanent_too)
             bset_local_var_alist (b, XCDR (tmp));
           else
             XSETCDR (last, XCDR (tmp));
-
-          if (XSYMBOL (local_var)->u.s.trapped_write == SYMBOL_TRAPPED_WRITE)
-            notify_variable_watchers (local_var, Qnil,
-                                      Qmakunbound, Fcurrent_buffer ());
         }
     }
 
@@ -1867,7 +1881,6 @@ cleaning up all windows currently displaying the buffer to be killed. */)
      won't be protected from GC.  They would be protected
      if they happened to remain cached in their symbols.
      This gets rid of them for certain.  */
-  swap_out_buffer_local_variables (b);
   reset_buffer_local_variables (b, 1);
 
   bset_name (b, Qnil);
@@ -2737,11 +2750,6 @@ the normal hook `change-major-mode-hook'.  */)
 {
   run_hook (Qchange_major_mode_hook);
 
-  /* Make sure none of the bindings in local_var_alist
-     remain swapped in, in their symbols.  */
-
-  swap_out_buffer_local_variables (current_buffer);
-
   /* Actually eliminate all local bindings of this buffer.  */
 
   reset_buffer_local_variables (current_buffer, 0);
@@ -2753,31 +2761,6 @@ the normal hook `change-major-mode-hook'.  */)
   return Qnil;
 }
 
-/* Make sure no local variables remain set up with buffer B
-   for their current values.  */
-
-static void
-swap_out_buffer_local_variables (struct buffer *b)
-{
-  Lisp_Object oalist, alist, buffer;
-
-  XSETBUFFER (buffer, b);
-  oalist = BVAR (b, local_var_alist);
-
-  for (alist = oalist; CONSP (alist); alist = XCDR (alist))
-    {
-      Lisp_Object sym = XCAR (XCAR (alist));
-      eassert (XSYMBOL (sym)->u.s.redirect == SYMBOL_LOCALIZED);
-      /* Need not do anything if some other buffer's binding is
-	 now cached.  */
-      if (EQ (SYMBOL_BLV (XSYMBOL (sym))->where, buffer))
-	{
-	  /* Symbol is set up for this buffer's old local value:
-	     swap it out!  */
-	  swap_in_global_binding (XSYMBOL (sym));
-	}
-    }
-}
 \f
 /* Find all the overlays in the current buffer that contain position POS.
    Return the number found, and store them in a vector in *VEC_PTR.
diff --git a/test/src/data-tests.el b/test/src/data-tests.el
index dda1278b6d..34637e4bfb 100644
--- a/test/src/data-tests.el
+++ b/test/src/data-tests.el
@@ -1,4 +1,4 @@
-;;; data-tests.el --- tests for src/data.c
+;;; data-tests.el --- tests for src/data.c  -*- lexical-binding:t -*-
 
 ;; Copyright (C) 2013-2018 Free Software Foundation, Inc.
 
@@ -484,3 +484,20 @@ binding-test-some-local
       (remove-variable-watcher 'data-tests-lvar collect-watch-data)
       (setq data-tests-lvar 6)
       (should (null watch-data)))))
+
+(ert-deftest data-tests-kill-all-local-variables () ;bug#30846
+  (with-temp-buffer
+    (setq-local data-tests-foo1 1)
+    (setq-local data-tests-foo2 2)
+    (setq-local data-tests-foo3 3)
+    (let ((oldfoo2 nil))
+      (add-variable-watcher 'data-tests-foo2
+                            (lambda (&rest _)
+                              (setq oldfoo2 (bound-and-true-p data-tests-foo2))))
+      (kill-all-local-variables)
+      (should (equal oldfoo2 '2)) ;Watcher is run before changing the var.
+      (should (not (or (bound-and-true-p data-tests-foo1)
+                       (bound-and-true-p data-tests-foo2)
+                       (bound-and-true-p data-tests-foo3)))))))
+
+;;; data-tests.el ends here