bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Andrea Cardaci]

[-- Attachment #1: Type: text/plain, Size: 471 bytes --]

Hello,

Someone filed a bug for my repo (https://github.com/cyrus-and/zoom/issues/20)
but I'm pretty sure it's a bug in Emacs. Unfortunately the steps to
reproduce it involve setting up `doom-emacs` which is quite cumbersome.

You can find all the details at the above link but in short using
`doom-emacs` with Zoom and NeoTree crashes Emacs with a segmentation fault.

I apologize for the low quality of this bug report, but I think that's
better than nothing.


Andrea

[-- Attachment #2: Type: text/html, Size: 1124 bytes --]

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Noam Postavsky]

tags 31312 + confirmed
found 31312 26.1
quit

Andrea Cardaci <cyrus.and@gmail.com> writes:

> Someone filed a bug for my repo (https://github.com/cyrus-and/zoom/issues/20)
> but I'm pretty sure it's a bug in Emacs. Unfortunately the steps to
> reproduce it involve setting up `doom-emacs` which is quite cumbersome.
>
> You can find all the details at the above link but in short using
> `doom-emacs` with Zoom and NeoTree crashes Emacs with a segmentation fault.

Reproduced with emacs-26, seems there is a window without a buffer.

../../src/buffer.h:914: Emacs fatal error: assertion failed: BUFFERP (a)

Thread 1 "emacs" hit Breakpoint 1, terminate_due_to_signal (sig=6, backtrace_limit=2147483647)
    at ../../src/emacs.c:364
364	  signal (sig, SIG_DFL);
(gdb) bt
#0  terminate_due_to_signal (sig=6, backtrace_limit=2147483647) at ../../src/emacs.c:364
#1  0x0000000000614655 in die (msg=0x74ecf9 "BUFFERP (a)", file=0x74ece6 "../../src/buffer.h", 
    line=914) at ../../src/alloc.c:7423
#2  0x000000000057a1a8 in XBUFFER (a=XIL(0)) at ../../src/buffer.h:914
#3  0x0000000000464716 in reconsider_clip_changes (w=0x60537f0) at ../../src/xdisp.c:13727
#4  0x000000000046514d in redisplay_internal () at ../../src/xdisp.c:13939
#5  0x0000000000463c16 in redisplay () at ../../src/xdisp.c:13518
#6  0x00000000005851ae in read_char (commandflag=1, map=XIL(0x5cf7503), prev_event=XIL(0), 
    used_mouse_menu=0x7fffffffe41f, end_time=0x0) at ../../src/keyboard.c:2480
#7  0x00000000005957c5 in read_key_sequence (keybuf=0x7fffffffe570, bufsize=30, prompt=XIL(0), 
    dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, 
    prevent_redisplay=false) at ../../src/keyboard.c:9147
#8  0x0000000000581d55 in command_loop_1 () at ../../src/keyboard.c:1368
#9  0x0000000000638529 in internal_condition_case (bfun=0x581909 <command_loop_1>, 
    handlers=XIL(0x5250), hfun=0x580f4c <cmd_error>) at ../../src/eval.c:1332
#10 0x0000000000581524 in command_loop_2 (ignore=XIL(0)) at ../../src/keyboard.c:1110
#11 0x0000000000637a32 in internal_catch (tag=XIL(0xc6f0), func=0x5814f7 <command_loop_2>, arg=XIL(0))
    at ../../src/eval.c:1097
#12 0x00000000005814c2 in command_loop () at ../../src/keyboard.c:1089
#13 0x0000000000580a36 in recursive_edit_1 () at ../../src/keyboard.c:695
#14 0x0000000000580c2b in Frecursive_edit () at ../../src/keyboard.c:766
#15 0x000000000057e7c6 in main (argc=1, argv=0x7fffffffea38) at ../../src/emacs.c:1713
(gdb) frame 3
#3  0x0000000000464716 in reconsider_clip_changes (w=0x60537f0) at ../../src/xdisp.c:13727
13727	  struct buffer *b = XBUFFER (w->contents);
(gdb) p w->contents
$1 = XIL(0)

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[martin rudalics]

 > Reproduced with emacs-26, seems there is a window without a buffer.
 >
 > ../../src/buffer.h:914: Emacs fatal error: assertion failed: BUFFERP (a)
 >
 > Thread 1 "emacs" hit Breakpoint 1, terminate_due_to_signal (sig=6, backtrace_limit=2147483647)
 >      at ../../src/emacs.c:364
 > 364	  signal (sig, SIG_DFL);
 > (gdb) bt
 > #0  terminate_due_to_signal (sig=6, backtrace_limit=2147483647) at ../../src/emacs.c:364
 > #1  0x0000000000614655 in die (msg=0x74ecf9 "BUFFERP (a)", file=0x74ece6 "../../src/buffer.h",
 >      line=914) at ../../src/alloc.c:7423
 > #2  0x000000000057a1a8 in XBUFFER (a=XIL(0)) at ../../src/buffer.h:914
 > #3  0x0000000000464716 in reconsider_clip_changes (w=0x60537f0) at ../../src/xdisp.c:13727
 > #4  0x000000000046514d in redisplay_internal () at ../../src/xdisp.c:13939
 > #5  0x0000000000463c16 in redisplay () at ../../src/xdisp.c:13518
 > #6  0x00000000005851ae in read_char (commandflag=1, map=XIL(0x5cf7503), prev_event=XIL(0),
 >      used_mouse_menu=0x7fffffffe41f, end_time=0x0) at ../../src/keyboard.c:2480
 > #7  0x00000000005957c5 in read_key_sequence (keybuf=0x7fffffffe570, bufsize=30, prompt=XIL(0),
 >      dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true,
 >      prevent_redisplay=false) at ../../src/keyboard.c:9147
 > #8  0x0000000000581d55 in command_loop_1 () at ../../src/keyboard.c:1368
 > #9  0x0000000000638529 in internal_condition_case (bfun=0x581909 <command_loop_1>,
 >      handlers=XIL(0x5250), hfun=0x580f4c <cmd_error>) at ../../src/eval.c:1332
 > #10 0x0000000000581524 in command_loop_2 (ignore=XIL(0)) at ../../src/keyboard.c:1110
 > #11 0x0000000000637a32 in internal_catch (tag=XIL(0xc6f0), func=0x5814f7 <command_loop_2>, arg=XIL(0))
 >      at ../../src/eval.c:1097
 > #12 0x00000000005814c2 in command_loop () at ../../src/keyboard.c:1089
 > #13 0x0000000000580a36 in recursive_edit_1 () at ../../src/keyboard.c:695
 > #14 0x0000000000580c2b in Frecursive_edit () at ../../src/keyboard.c:766
 > #15 0x000000000057e7c6 in main (argc=1, argv=0x7fffffffea38) at ../../src/emacs.c:1713
 > (gdb) frame 3

Is w in reconsider_clip_changes the same as XWINDOW (selected_window)
when it fails?  If so, could you instrument select_window_1 (or use a
watchpoint) to check whether selected_window = window ever assigns a
window with XWINDOW (window)->contents equal Qnil?

Thanks, martin

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Noam Postavsky]

martin rudalics <rudalics@gmx.at> writes:

> Is w in reconsider_clip_changes the same as XWINDOW (selected_window)
> when it fails?

Nope.  In fact, doing

    break 13939 if (w != XWINDOW(selected_window))
    
triggers only just before the bad reconsider_clip_changes call.

#0  redisplay_internal () at ../../src/xdisp.c:13939
#1  0x0000000000463c16 in redisplay () at ../../src/xdisp.c:13518
#2  0x00000000005851ae in read_char (commandflag=1, map=XIL(0x5e99db3), prev_event=XIL(0), 
    used_mouse_menu=0x7fffffffe41f, end_time=0x0) at ../../src/keyboard.c:2480
#3  0x00000000005957c5 in read_key_sequence (keybuf=0x7fffffffe570, bufsize=30, prompt=XIL(0), 
    dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true, 
    prevent_redisplay=false) at ../../src/keyboard.c:9147
#4  0x0000000000581d55 in command_loop_1 () at ../../src/keyboard.c:1368
(More stack frames follow...)
(gdb) p w
$16 = (struct window *) 0x47d22f0
(gdb) p sw
$17 = (struct window *) 0x47d22f0
(gdb) p XWINDOW(selected_window)
$18 = (struct window *) 0x61109d0
(gdb) p w->contents
$19 = XIL(0)
(gdb) p XWINDOW(selected_window)->contents
$20 = XIL(0x493e565)
(gdb) xpr
Lisp_Vectorlike
PVEC_BUFFER
$21 = (struct buffer *) 0x493e560
(unsigned char *) 0x4a34568 " *NeoTree*"

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[martin rudalics]

 >> Is w in reconsider_clip_changes the same as XWINDOW (selected_window)
 >> when it fails?
 >
 > Nope.  In fact, doing
 >
 >      break 13939 if (w != XWINDOW(selected_window))
 >
 > triggers only just before the bad reconsider_clip_changes call.
 >
 > #0  redisplay_internal () at ../../src/xdisp.c:13939
 > #1  0x0000000000463c16 in redisplay () at ../../src/xdisp.c:13518
 > #2  0x00000000005851ae in read_char (commandflag=1, map=XIL(0x5e99db3), prev_event=XIL(0),
 >      used_mouse_menu=0x7fffffffe41f, end_time=0x0) at ../../src/keyboard.c:2480
 > #3  0x00000000005957c5 in read_key_sequence (keybuf=0x7fffffffe570, bufsize=30, prompt=XIL(0),
 >      dont_downcase_last=false, can_return_switch_frame=true, fix_current_buffer=true,
 >      prevent_redisplay=false) at ../../src/keyboard.c:9147
 > #4  0x0000000000581d55 in command_loop_1 () at ../../src/keyboard.c:1368
 > (More stack frames follow...)
 > (gdb) p w
 > $16 = (struct window *) 0x47d22f0
 > (gdb) p sw
 > $17 = (struct window *) 0x47d22f0
 > (gdb) p XWINDOW(selected_window)
 > $18 = (struct window *) 0x61109d0
 > (gdb) p w->contents
 > $19 = XIL(0)
 > (gdb) p XWINDOW(selected_window)->contents
 > $20 = XIL(0x493e565)
 > (gdb) xpr
 > Lisp_Vectorlike
 > PVEC_BUFFER
 > $21 = (struct buffer *) 0x493e560
 > (unsigned char *) 0x4a34568 " *NeoTree*"

I'm probably too silly to understand this.  IIUC we are here

   /* do_pending_window_change could change the selected_window due to
      frame resizing which makes the selected window too small.  */
   if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
     sw = w;

   /* Clear frames marked as garbaged.  */
   clear_garbaged_frames ();

   /* Build menubar and tool-bar items.  */
   if (NILP (Vmemory_full))
     prepare_menu_bars ();

   reconsider_clip_changes (w);

and the only ways this could cause w != XWINDOW(selected_window)
before the last call is either !WINDOWP (selected_window) or that
clear_garbaged_frames or prepare_menu_bars change selected_window.
Can you find the responsible?

martin

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Eli Zaretskii]

On May 2, 2018 9:16:26 AM GMT+03:00, martin rudalics <rudalics@gmx.at> wrote:
> >> Is w in reconsider_clip_changes the same as XWINDOW
> (selected_window)
>  >> when it fails?
>  >
>  > Nope.  In fact, doing
>  >
>  >      break 13939 if (w != XWINDOW(selected_window))
>  >
>  > triggers only just before the bad reconsider_clip_changes call.
>  >
>  > #0  redisplay_internal () at ../../src/xdisp.c:13939
>  > #1  0x0000000000463c16 in redisplay () at ../../src/xdisp.c:13518
> > #2  0x00000000005851ae in read_char (commandflag=1,
> map=XIL(0x5e99db3), prev_event=XIL(0),
> >      used_mouse_menu=0x7fffffffe41f, end_time=0x0) at
> ../../src/keyboard.c:2480
> > #3  0x00000000005957c5 in read_key_sequence (keybuf=0x7fffffffe570,
> bufsize=30, prompt=XIL(0),
> >      dont_downcase_last=false, can_return_switch_frame=true,
> fix_current_buffer=true,
>  >      prevent_redisplay=false) at ../../src/keyboard.c:9147
> > #4  0x0000000000581d55 in command_loop_1 () at
> ../../src/keyboard.c:1368
>  > (More stack frames follow...)
>  > (gdb) p w
>  > $16 = (struct window *) 0x47d22f0
>  > (gdb) p sw
>  > $17 = (struct window *) 0x47d22f0
>  > (gdb) p XWINDOW(selected_window)
>  > $18 = (struct window *) 0x61109d0
>  > (gdb) p w->contents
>  > $19 = XIL(0)
>  > (gdb) p XWINDOW(selected_window)->contents
>  > $20 = XIL(0x493e565)
>  > (gdb) xpr
>  > Lisp_Vectorlike
>  > PVEC_BUFFER
>  > $21 = (struct buffer *) 0x493e560
>  > (unsigned char *) 0x4a34568 " *NeoTree*"
> 
> I'm probably too silly to understand this.  IIUC we are here
> 
>    /* do_pending_window_change could change the selected_window due to
>       frame resizing which makes the selected window too small.  */
> if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) !=
> sw)
>      sw = w;
> 
>    /* Clear frames marked as garbaged.  */
>    clear_garbaged_frames ();
> 
>    /* Build menubar and tool-bar items.  */
>    if (NILP (Vmemory_full))
>      prepare_menu_bars ();
> 
>    reconsider_clip_changes (w);
> 
> and the only ways this could cause w != XWINDOW(selected_window)
> before the last call is either !WINDOWP (selected_window) or that
> clear_garbaged_frames or prepare_menu_bars change selected_window.
> Can you find the responsible?
> 
> martin

Those are exactly the questions I'd like to have answers for.

Btw, I tried to run the recipe in "emacs -Q", and it didn't crash, so I guess
something in doom-land is a necessary piece of this puzzle.

Also, please take a look at zoom.el: it hooks window-size-change-functions
and resizes the windows in that hook(!).  And prepare_menu_bars runs
window-size-change-functions, which resets the frame's
window-configuration-changed flag, assuming that no one in their right
mind will change the configuration from a hook that notifies about
changes in configuration.

After all this madness, it's small wonder that we end up with a dead
window in W.

Of course, we need to protect us against such calamities in any case...

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Noam Postavsky]

[-- Attachment #1: Type: text/plain, Size: 3852 bytes --]

martin rudalics <rudalics@gmx.at> writes:

> and the only ways this could cause w != XWINDOW(selected_window)
> before the last call is either !WINDOWP (selected_window) or that
> clear_garbaged_frames or prepare_menu_bars change selected_window.
> Can you find the responsible?

Yes, found it, seems to be a combination of zoom
window-size-change-functions called via prepare_menu_bars and doom
advice on balance-windows.  I got 12 changes of selected_window.  First
hit summarized inline, the rest of the backtraces are attached below.

#0  select_window_1 (window=XIL(0x1601c35), inhibit_point_swap=false) at ../../src/window.c:562
#1  0x00000000004aff15 in select_window (window=XIL(0x1601c35), norecord=XIL(0xc090), inhibit_point_swap=false) at ../../src/window.c:520
#2  0x00000000004b0183 in Fselect_window (window=XIL(0x1601c35), norecord=XIL(0xc090)) at ../../src/window.c:590
#3  0x00000000004beaa7 in Fdelete_window_internal (window=XIL(0x1696c35)) at ../../src/window.c:4667

#79 0x0000000000441b1a in safe_call1 (fn=XIL(0x4a9e6d0), arg=XIL(0x1600c35)) at ../../src/xdisp.c:2644
#80 0x00000000004bb27a in run_window_size_change_functions (frame=XIL(0x1600c35)) at ../../src/window.c:3457
#81 0x0000000000460409 in prepare_menu_bars () at ../../src/xdisp.c:12065
#82 0x0000000000465141 in redisplay_internal () at ../../src/xdisp.c:13937
#83 0x0000000000463c16 in redisplay () at ../../src/xdisp.c:13518

Lisp Backtrace:
[...]
"delete-window" (0xffff9ab0)
[...]
"doom/popup-close" (0xffffa068)
[...]
"doom*popups-save" (0xffffaa00)
[...]
"ad-Advice-balance-windows" (0xffffb530)
"apply" (0xffffb528)
"balance-windows" (0xffffb960)
"let" (0xffffbcc0)
"zoom--update" (0xffffbe10)
[...]
"zoom--handler" (0xffffc928)
"redisplay_internal (C function)" (0x0)

(defun zoom--on ()
  [...]
  (add-hook 'window-size-change-functions #'zoom--handler)

(defun zoom--handler (&optional window-or-frame norecord)
  [...]
    (with-selected-window
      [...]
      (zoom--update))))

(defun zoom--update ()
  "Update the window layout in the current frame."
  (let (;; temporarily disables this mode during resize to avoid infinite
        ;; recursion (probably not needed thanks to the following)
        (zoom-mode nil)
        ;; temporarily disable all (even external) hooks about window
        ;; configuration changes to try to avoid potential flickering since
        ;; `balance-windows' calls them
        (window-configuration-change-hook nil)
        ;; make sure that other windows are resized nicely after resizing the
        ;; selected one
        (window-combination-resize t)
        ;; make sure that the exact same amount of pixels is assigned to all the
        ;; siblings
        (window-resize-pixelwise t))
    ;; start from a balanced layout anyway
    (balance-windows)
    ;; check if the selected window is not ignored
    (unless (zoom--window-ignored-p)
      (zoom--resize)
      (zoom--fix-scroll))))

;; doom/core/core-popups.el:
  (advice-add #'balance-windows :around #'doom*popups-save)

(defun doom*popups-save (orig-fn &rest args)
  "Sets aside all popups before executing the original function, usually to
prevent the popup(s) from messing up the UI (or vice versa)."
  (save-popups! (apply orig-fn args)))

(defmacro save-popups! (&rest body)
  "Sets aside all popups before executing the original function, usually to
prevent the popup(s) from messing up the UI (or vice versa)."
  `(let ((in-popup-p (doom-popup-p))
         (popups (doom-popup-windows))
         (doom-popup-remember-history t)
         (doom-popup-inhibit-autokill t))
     (when popups
       (mapc #'doom/popup-close popups))
     (unwind-protect
         (progn ,@body)
       (when popups
         (let ((origin (selected-window)))
           (doom/popup-restore)
           (unless in-popup-p
             (select-window origin)))))))


[-- Attachment #2: gdbinit modifications to catch bug --]
[-- Type: text/plain, Size: 936 bytes --]

diff --git i/src/.gdbinit w/src/.gdbinit
index cc06b2e11c..0ebf07f0f7 100644
--- i/src/.gdbinit
+++ w/src/.gdbinit
@@ -1264,6 +1264,35 @@ commands
 end
 
 
+watch selected_window
+set $selwindow_watch = $bpnum
+commands $selwindow_watch
+  bt
+end
+disable $selwindow_watch
+
+# this one never happens, I think
+break xdisp.c:13929 if !WINDOWP(selected_window)
+
+break xdisp.c:13929 if WINDOWP(selected_window)
+commands
+  silent
+  enable $selwindow_watch
+  continue
+end
+set $enable_selwindow_watch = $bpnum
+disable $enable_selwindow_watch
+printf "selected window watch enabler = %d\n", $enable_selwindow_watch
+
+break xdisp.c:13939 if !NILP(w->contents)
+commands
+  silent
+  disable $selwindow_watch
+  continue
+end
+
+break xdisp.c:13939 if NILP(w->contents)
+
 # Put the Python code at the end of .gdbinit so that if GDB does not
 # support Python, GDB will do all the above initializations before
 # reporting an error.

[-- Attachment #3: gdb backtraces of the selected_window changes --]
[-- Type: application/gzip, Size: 11386 bytes --]

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Eli Zaretskii]

On May 2, 2018 3:21:15 PM GMT+03:00, Noam Postavsky <npostavs@gmail.com> wrote:
> martin rudalics <rudalics@gmx.at> writes:
> 
> > and the only ways this could cause w != XWINDOW(selected_window)
> > before the last call is either !WINDOWP (selected_window) or that
> > clear_garbaged_frames or prepare_menu_bars change selected_window.
> > Can you find the responsible?
> 
> Yes, found it, seems to be a combination of zoom
> window-size-change-functions called via prepare_menu_bars and doom
> advice on balance-windows.  I got 12 changes of selected_window. 
> First
> hit summarized inline, the rest of the backtraces are attached below.
> 
> #0  select_window_1 (window=XIL(0x1601c35), inhibit_point_swap=false)
> at ../../src/window.c:562
> #1  0x00000000004aff15 in select_window (window=XIL(0x1601c35),
> norecord=XIL(0xc090), inhibit_point_swap=false) at
> ../../src/window.c:520
> #2  0x00000000004b0183 in Fselect_window (window=XIL(0x1601c35),
> norecord=XIL(0xc090)) at ../../src/window.c:590
> #3  0x00000000004beaa7 in Fdelete_window_internal
> (window=XIL(0x1696c35)) at ../../src/window.c:4667
> 
> #79 0x0000000000441b1a in safe_call1 (fn=XIL(0x4a9e6d0),
> arg=XIL(0x1600c35)) at ../../src/xdisp.c:2644
> #80 0x00000000004bb27a in run_window_size_change_functions
> (frame=XIL(0x1600c35)) at ../../src/window.c:3457
> #81 0x0000000000460409 in prepare_menu_bars () at
> ../../src/xdisp.c:12065
> #82 0x0000000000465141 in redisplay_internal () at
> ../../src/xdisp.c:13937
> #83 0x0000000000463c16 in redisplay () at ../../src/xdisp.c:13518
> 
> Lisp Backtrace:
> [...]
> "delete-window" (0xffff9ab0)
> [...]
> "doom/popup-close" (0xffffa068)
> [...]
> "doom*popups-save" (0xffffaa00)
> [...]
> "ad-Advice-balance-windows" (0xffffb530)
> "apply" (0xffffb528)
> "balance-windows" (0xffffb960)
> "let" (0xffffbcc0)
> "zoom--update" (0xffffbe10)
> [...]
> "zoom--handler" (0xffffc928)
> "redisplay_internal (C function)" (0x0)
> 
> (defun zoom--on ()
>   [...]
>   (add-hook 'window-size-change-functions #'zoom--handler)
> 
> (defun zoom--handler (&optional window-or-frame norecord)
>   [...]
>     (with-selected-window
>       [...]
>       (zoom--update))))
> 
> (defun zoom--update ()
>   "Update the window layout in the current frame."
> (let (;; temporarily disables this mode during resize to avoid
> infinite
>         ;; recursion (probably not needed thanks to the following)
>         (zoom-mode nil)
>         ;; temporarily disable all (even external) hooks about window
>    ;; configuration changes to try to avoid potential flickering since
>         ;; `balance-windows' calls them
>         (window-configuration-change-hook nil)
>  ;; make sure that other windows are resized nicely after resizing the
>         ;; selected one
>         (window-combination-resize t)
> ;; make sure that the exact same amount of pixels is assigned to all
> the
>         ;; siblings
>         (window-resize-pixelwise t))
>     ;; start from a balanced layout anyway
>     (balance-windows)
>     ;; check if the selected window is not ignored
>     (unless (zoom--window-ignored-p)
>       (zoom--resize)
>       (zoom--fix-scroll))))
> 
> ;; doom/core/core-popups.el:
>   (advice-add #'balance-windows :around #'doom*popups-save)
> 
> (defun doom*popups-save (orig-fn &rest args)
> "Sets aside all popups before executing the original function, usually
> to
> prevent the popup(s) from messing up the UI (or vice versa)."
>   (save-popups! (apply orig-fn args)))
> 
> (defmacro save-popups! (&rest body)
> "Sets aside all popups before executing the original function, usually
> to
> prevent the popup(s) from messing up the UI (or vice versa)."
>   `(let ((in-popup-p (doom-popup-p))
>          (popups (doom-popup-windows))
>          (doom-popup-remember-history t)
>          (doom-popup-inhibit-autokill t))
>      (when popups
>        (mapc #'doom/popup-close popups))
>      (unwind-protect
>          (progn ,@body)
>        (when popups
>          (let ((origin (selected-window)))
>            (doom/popup-restore)
>            (unless in-popup-p
>              (select-window origin)))))))

So we need the same defense after prepare_menu_bars as we
have after do_pending_changes, I think.

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Andrea Cardaci]

[-- Attachment #1: Type: text/plain, Size: 4990 bytes --]

> Also, please take a look at zoom.el: it hooks window-size-change-functions
> and resizes the windows in that hook(!).

> assuming that no one in their right
> mind will change the configuration from a hook that notifies about
> changes in configuration

Yeah, I'm that guy...

Any advice on this part? I needed to force the size of windows and hooking
`select-window` (via `advice-add`) is apparently not enough to catch all
the cases.

Is there a better way to achieve what I want?

On 2 May 2018 at 14:45, Eli Zaretskii <eliz@gnu.org> wrote:

> On May 2, 2018 3:21:15 PM GMT+03:00, Noam Postavsky <npostavs@gmail.com>
> wrote:
> > martin rudalics <rudalics@gmx.at> writes:
> >
> > > and the only ways this could cause w != XWINDOW(selected_window)
> > > before the last call is either !WINDOWP (selected_window) or that
> > > clear_garbaged_frames or prepare_menu_bars change selected_window.
> > > Can you find the responsible?
> >
> > Yes, found it, seems to be a combination of zoom
> > window-size-change-functions called via prepare_menu_bars and doom
> > advice on balance-windows.  I got 12 changes of selected_window.
> > First
> > hit summarized inline, the rest of the backtraces are attached below.
> >
> > #0  select_window_1 (window=XIL(0x1601c35), inhibit_point_swap=false)
> > at ../../src/window.c:562
> > #1  0x00000000004aff15 in select_window (window=XIL(0x1601c35),
> > norecord=XIL(0xc090), inhibit_point_swap=false) at
> > ../../src/window.c:520
> > #2  0x00000000004b0183 in Fselect_window (window=XIL(0x1601c35),
> > norecord=XIL(0xc090)) at ../../src/window.c:590
> > #3  0x00000000004beaa7 in Fdelete_window_internal
> > (window=XIL(0x1696c35)) at ../../src/window.c:4667
> >
> > #79 0x0000000000441b1a in safe_call1 (fn=XIL(0x4a9e6d0),
> > arg=XIL(0x1600c35)) at ../../src/xdisp.c:2644
> > #80 0x00000000004bb27a in run_window_size_change_functions
> > (frame=XIL(0x1600c35)) at ../../src/window.c:3457
> > #81 0x0000000000460409 in prepare_menu_bars () at
> > ../../src/xdisp.c:12065
> > #82 0x0000000000465141 in redisplay_internal () at
> > ../../src/xdisp.c:13937
> > #83 0x0000000000463c16 in redisplay () at ../../src/xdisp.c:13518
> >
> > Lisp Backtrace:
> > [...]
> > "delete-window" (0xffff9ab0)
> > [...]
> > "doom/popup-close" (0xffffa068)
> > [...]
> > "doom*popups-save" (0xffffaa00)
> > [...]
> > "ad-Advice-balance-windows" (0xffffb530)
> > "apply" (0xffffb528)
> > "balance-windows" (0xffffb960)
> > "let" (0xffffbcc0)
> > "zoom--update" (0xffffbe10)
> > [...]
> > "zoom--handler" (0xffffc928)
> > "redisplay_internal (C function)" (0x0)
> >
> > (defun zoom--on ()
> >   [...]
> >   (add-hook 'window-size-change-functions #'zoom--handler)
> >
> > (defun zoom--handler (&optional window-or-frame norecord)
> >   [...]
> >     (with-selected-window
> >       [...]
> >       (zoom--update))))
> >
> > (defun zoom--update ()
> >   "Update the window layout in the current frame."
> > (let (;; temporarily disables this mode during resize to avoid
> > infinite
> >         ;; recursion (probably not needed thanks to the following)
> >         (zoom-mode nil)
> >         ;; temporarily disable all (even external) hooks about window
> >    ;; configuration changes to try to avoid potential flickering since
> >         ;; `balance-windows' calls them
> >         (window-configuration-change-hook nil)
> >  ;; make sure that other windows are resized nicely after resizing the
> >         ;; selected one
> >         (window-combination-resize t)
> > ;; make sure that the exact same amount of pixels is assigned to all
> > the
> >         ;; siblings
> >         (window-resize-pixelwise t))
> >     ;; start from a balanced layout anyway
> >     (balance-windows)
> >     ;; check if the selected window is not ignored
> >     (unless (zoom--window-ignored-p)
> >       (zoom--resize)
> >       (zoom--fix-scroll))))
> >
> > ;; doom/core/core-popups.el:
> >   (advice-add #'balance-windows :around #'doom*popups-save)
> >
> > (defun doom*popups-save (orig-fn &rest args)
> > "Sets aside all popups before executing the original function, usually
> > to
> > prevent the popup(s) from messing up the UI (or vice versa)."
> >   (save-popups! (apply orig-fn args)))
> >
> > (defmacro save-popups! (&rest body)
> > "Sets aside all popups before executing the original function, usually
> > to
> > prevent the popup(s) from messing up the UI (or vice versa)."
> >   `(let ((in-popup-p (doom-popup-p))
> >          (popups (doom-popup-windows))
> >          (doom-popup-remember-history t)
> >          (doom-popup-inhibit-autokill t))
> >      (when popups
> >        (mapc #'doom/popup-close popups))
> >      (unwind-protect
> >          (progn ,@body)
> >        (when popups
> >          (let ((origin (selected-window)))
> >            (doom/popup-restore)
> >            (unless in-popup-p
> >              (select-window origin)))))))
>
> So we need the same defense after prepare_menu_bars as we
> have after do_pending_changes, I think.
>

[-- Attachment #2: Type: text/html, Size: 7019 bytes --]

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[martin rudalics]

 > Yes, found it, seems to be a combination of zoom
 > window-size-change-functions called via prepare_menu_bars and doom
 > advice on balance-windows.  I got 12 changes of selected_window.  First
 > hit summarized inline, the rest of the backtraces are attached below.

Magnificent, many thanks.

A first interpretation is as follows (please correct me if you think
I'm wrong): The first hit stems from a call of 'delete-window' where
'frame-first-window' apparently finds a bad window.  The remainders
but the last one come from running 'window-configuration-change-hook'
where the saved selected window after running the local part of the
hook is dead.  The last one comes from 'window-size-change-functions'
where a call of 'select-window' apparently succeeds passing us a bad
window.

The 'window-configuration-change-hook' instances are design failures -
we simply have to let the selected window alone when the saved one has
been deleted.  The other two deserve some investigation.

martin

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[martin rudalics]

 > So we need the same defense after prepare_menu_bars as we
 > have after do_pending_changes, I think.

If you mean something like

   if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
     sw = w;

I'm afraid that this would fail since selected_window has no buffer
any more (or may have even been recycled already).  Or am I missing
something?

martin

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[martin rudalics]

 > Any advice on this part? I needed to force the size of windows and hooking
 > `select-window` (via `advice-add`) is apparently not enough to catch all
 > the cases.

It might not help in the case at hand, but instead of advising
'select-window' (which won't do the job anyway when a window is
selected directly via select_window) please add your function to
'buffer-list-update-hook'.  Hooking 'select-window' _is_ bad idea,
always.

martin

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Eli Zaretskii]

> Date: Wed, 02 May 2018 15:42:58 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: 31312@debbugs.gnu.org, Andrea Cardaci <cyrus.and@gmail.com>
> 
>  > So we need the same defense after prepare_menu_bars as we
>  > have after do_pending_changes, I think.
> 
> If you mean something like
> 
>    if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
>      sw = w;
> 
> I'm afraid that this would fail since selected_window has no buffer
> any more (or may have even been recycled already).

Is that a fact?  I might be mistaken, but my take on what Noam found
was that the selected window is OK, it's just that the window held in
W is dead (i.e. it was deleted inside the tempest that happened in
run_window_size_change_functions called by prepare_menu_bars).  So my
suggestion is to update W with the new selected window.

In general, if the selected window has no buffer, we are in deep
trouble, and not just in redisplay.  So I very much hope this is not
what happens here.

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Andrea Cardaci]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

> It might not help in the case at hand, but instead of advising
> 'select-window' (which won't do the job anyway when a window is
> selected directly via select_window) please add your function to
> 'buffer-list-update-hook'.  Hooking 'select-window' _is_ bad idea,
> always.

I will try that, even though I'm pretty sure that I already tried this
approach in the past and switched back to advising `select-window` but I
cannot recall the reason.

On 2 May 2018 at 15:47, martin rudalics <rudalics@gmx.at> wrote:

> > Any advice on this part? I needed to force the size of windows and
> hooking
> > `select-window` (via `advice-add`) is apparently not enough to catch all
> > the cases.
>
> It might not help in the case at hand, but instead of advising
> 'select-window' (which won't do the job anyway when a window is
> selected directly via select_window) please add your function to
> 'buffer-list-update-hook'.  Hooking 'select-window' _is_ bad idea,
> always.
>
> martin
>

[-- Attachment #2: Type: text/html, Size: 1541 bytes --]

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Eli Zaretskii]

> From: Andrea Cardaci <cyrus.and@gmail.com>
> Date: Wed, 2 May 2018 15:27:12 +0200
> Cc: bug-gnu-emacs@gnu.org, Noam Postavsky <npostavs@gmail.com>, 
> 	martin rudalics <rudalics@gmx.at>, 31312@debbugs.gnu.org
> 
> Any advice on this part? I needed to force the size of windows and hooking `select-window` (via `advice-add`)
> is apparently not enough to catch all the cases.
> 
> Is there a better way to achieve what I want?

If what Martin suggested somehow doesn't fit the bill, and no other
ideas are brought up, then I prefer to discuss new features or
infrastructure to satisfy similar needs, rather than force
applications to use such fragile techniques.  E.g., we could provide
some customizable controls in the code that resizes windows when
needed.

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Andrea Cardaci]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

> If what Martin suggested somehow doesn't fit the bill

It doesn't, because it deals with advising `select-window` while the main
issue here is that I resize windows in the `window-size-change-functions`
hook (albeit using a guard to avoid infinite recursion) and I can't come up
any workarounds for that.

On 2 May 2018 at 17:06, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Andrea Cardaci <cyrus.and@gmail.com>
> > Date: Wed, 2 May 2018 15:27:12 +0200
> > Cc: bug-gnu-emacs@gnu.org, Noam Postavsky <npostavs@gmail.com>,
> >       martin rudalics <rudalics@gmx.at>, 31312@debbugs.gnu.org
> >
> > Any advice on this part? I needed to force the size of windows and
> hooking `select-window` (via `advice-add`)
> > is apparently not enough to catch all the cases.
> >
> > Is there a better way to achieve what I want?
>
> If what Martin suggested somehow doesn't fit the bill, and no other
> ideas are brought up, then I prefer to discuss new features or
> infrastructure to satisfy similar needs, rather than force
> applications to use such fragile techniques.  E.g., we could provide
> some customizable controls in the code that resizes windows when
> needed.
>

[-- Attachment #2: Type: text/html, Size: 3043 bytes --]

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Eli Zaretskii]

> From: Andrea Cardaci <cyrus.and@gmail.com>
> Date: Wed, 2 May 2018 17:14:04 +0200
> Cc: Noam Postavsky <npostavs@gmail.com>, martin rudalics <rudalics@gmx.at>, 31312@debbugs.gnu.org
> 
> > If what Martin suggested somehow doesn't fit the bill
> 
> It doesn't, because it deals with advising `select-window` while the main issue here is that I resize windows in
> the `window-size-change-functions` hook (albeit using a guard to avoid infinite recursion) and I can't come up
> any workarounds for that.

In that case, could you explain in more detail what zoom.el attempts
to do, and what does it need from Emacs core to be able to do that?

(It's probably better to start a new discussion on emacs-devel, not
here.)

Thanks.

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[martin rudalics]

 >> If you mean something like
 >>
 >>     if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
 >>       sw = w;
 >>
 >> I'm afraid that this would fail since selected_window has no buffer
 >> any more (or may have even been recycled already).
 >
 > Is that a fact?  I might be mistaken, but my take on what Noam found
 > was that the selected window is OK, it's just that the window held in
 > W is dead (i.e. it was deleted inside the tempest that happened in
 > run_window_size_change_functions called by prepare_menu_bars).  So my
 > suggestion is to update W with the new selected window.

Then I misinterpreted Noam's results.  Anyway, a simple way to
reproduce the bug is

(defun foo (frame)
   (delete-window (selected-window)))

(add-hook 'window-size-change-functions 'foo)

and do C-x 2.

martin

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Eli Zaretskii]

> Date: Wed, 02 May 2018 20:43:30 +0200
> From: martin rudalics <rudalics@gmx.at>
> CC: npostavs@gmail.com, 31312@debbugs.gnu.org, cyrus.and@gmail.com
> 
> (defun foo (frame)
>    (delete-window (selected-window)))
> 
> (add-hook 'window-size-change-functions 'foo)
> 
> and do C-x 2.

Right, and as you can see, selected_window is still a valid leaf
window when we abort in this case.

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Noam Postavsky]

martin rudalics <rudalics@gmx.at> writes:

>> So we need the same defense after prepare_menu_bars as we
>> have after do_pending_changes, I think.
>
> If you mean something like
>
>   if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
>     sw = w;
>
> I'm afraid that this would fail since selected_window has no buffer
> any more (or may have even been recycled already).  Or am I missing
> something?

It seems to work (I don't know enough about the code to explain why).  I
applied this patch:

--- i/src/xdisp.c
+++ w/src/xdisp.c
@@ -13936,6 +13936,11 @@ redisplay_internal (void)
   if (NILP (Vmemory_full))
     prepare_menu_bars ();
 
+  /* prepare_menu_bars may call lisp hooks and hence change the
+     selected_window.  */
+  if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
+    sw = w;
+
   reconsider_clip_changes (w);
 
   /* In most cases selected window displays current buffer.  */

And then following the original recipe does not segfault.  There is a
Lisp error, but I think that's already a bug in zoom and/or doom.

Debugger entered--Lisp error: (wrong-type-argument window-live-p #<window 39>)
  set-window-dedicated-p(#<window 39> t)
  neo-window--init(#<window 39> #<buffer  *NeoTree*>)
  neo-global--create-window()
  neo-global--get-window(t)
  neo-global--open-dir("/home/npostavs/src/doom/")
  neo-global--open()
  neotree-show()
  funcall-interactively(neotree-show)
  call-interactively(neotree-show record nil)
  command-execute(neotree-show record)
  #f(compiled-function (cmd) #<bytecode 0x1289c19>)("neotree-show")
  ivy-call()
  ivy-read("M-x " ("toggle-debug-on-error" "zoom-mode" [...]
  counsel-M-x()
  funcall-interactively(counsel-M-x)
  call-interactively(counsel-M-x nil nil)
  command-execute(counsel-M-x)

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: eliz@gnu.org,  31312@debbugs.gnu.org,  cyrus.and@gmail.com
> Date: Wed, 02 May 2018 20:04:28 -0400
> 
> martin rudalics <rudalics@gmx.at> writes:
> 
> >> So we need the same defense after prepare_menu_bars as we
> >> have after do_pending_changes, I think.
> >
> > If you mean something like
> >
> >   if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
> >     sw = w;
> >
> > I'm afraid that this would fail since selected_window has no buffer
> > any more (or may have even been recycled already).  Or am I missing
> > something?
> 
> It seems to work (I don't know enough about the code to explain why).  I
> applied this patch:
> 
> --- i/src/xdisp.c
> +++ w/src/xdisp.c
> @@ -13936,6 +13936,11 @@ redisplay_internal (void)
>    if (NILP (Vmemory_full))
>      prepare_menu_bars ();
>  
> +  /* prepare_menu_bars may call lisp hooks and hence change the
> +     selected_window.  */
> +  if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
> +    sw = w;
> +
>    reconsider_clip_changes (w);
>  
>    /* In most cases selected window displays current buffer.  */
> 
> And then following the original recipe does not segfault.  There is a
> Lisp error, but I think that's already a bug in zoom and/or doom.

Right.

Please push to the emacs-26 branch, and thanks.

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[martin rudalics]

 > Right, and as you can see, selected_window is still a valid leaf
 > window when we abort in this case.

Yes.  I clearly misinterpreted Noam's log.

martin

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

> Please push to the emacs-26 branch, and thanks.

Wouldn't it make sense to remove the previous check, like this?

--- i/src/xdisp.c
+++ w/src/xdisp.c
@@ -13924,11 +13924,6 @@ redisplay_internal (void)
   /* Notice any pending interrupt request to change frame size.  */
   do_pending_window_change (true);
 
-  /* do_pending_window_change could change the selected_window due to
-     frame resizing which makes the selected window too small.  */
-  if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
-    sw = w;
-
   /* Clear frames marked as garbaged.  */
   clear_garbaged_frames ();
 
@@ -13936,6 +13931,13 @@ redisplay_internal (void)
   if (NILP (Vmemory_full))
     prepare_menu_bars ();
 
+  /* do_pending_window_change could change the selected_window due to
+     frame resizing which makes the selected window too small.
+     prepare_menu_bars may call lisp hooks and hence also change the
+     selected_window.  */
+  if (WINDOWP (selected_window) && (w = XWINDOW (selected_window)) != sw)
+    sw = w;
+
   reconsider_clip_changes (w);
 
   /* In most cases selected window displays current buffer.  */

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: rudalics@gmx.at,  31312@debbugs.gnu.org,  cyrus.and@gmail.com
> Date: Thu, 03 May 2018 08:21:34 -0400
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Please push to the emacs-26 branch, and thanks.
> 
> Wouldn't it make sense to remove the previous check, like this?

That's a tad more risky, but I guess we can afford that.  So please
do.

bug#31312: Segmentation fault with doom-emacs, NeoTree and Zoom

[Noam Postavsky]

tags 31312 fixed
close 31312 26.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Noam Postavsky <npostavs@gmail.com>
>> Cc: rudalics@gmx.at,  31312@debbugs.gnu.org,  cyrus.and@gmail.com
>> Date: Thu, 03 May 2018 08:21:34 -0400
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Please push to the emacs-26 branch, and thanks.
>> 
>> Wouldn't it make sense to remove the previous check, like this?

> That's a tad more risky, but I guess we can afford that.  So please
> do.

Done.

[1: b90ce66d32]: 2018-05-03 20:58:06 -0400
  Handle selected_window change in prepare_menu_bars (Bug#31312)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b90ce66d32ede14a9191008096e596f6dfb9a48b>