bug#7728: 24.0.50; GDB backtrace from abort

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

Eli suggested I report this now, even though I have only the backtrace
and didn't follow up with more commands when GDB was open.  This was in
my own setup, not emacs -Q.
 
$ ./gdb -p 3684
GNU gdb (GDB) 7.2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Attaching to process 3684
[New Thread 3684.0x1210]
[New Thread 3684.0x154]
[New Thread 3684.0xe00]
Reading symbols from C:\Emacs-24-2010-12-20\bin\emacs.exe...done.
[Switching to Thread 3684.0xe00]
Warning: c:\drews-lisp-20\bin/../lwlib: No such file or directory.
SIGINT is used by the debugger.
Are you sure you want to change it? (y or n) [answered Y; input not from
terminal]
Environment variable "DISPLAY" not defined.
TERM = cygwin
Breakpoint 1 at 0x1309b68: file w32fns.c, line 7298.
Temporary breakpoint 2 at 0x11cf699: file sysdep.c, line 839.
(gdb) c
Continuing.
 
Program received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 3684.0x1210]
0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
(gdb) bt
#0  0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
#1  0x01309b9f in w32_abort () at w32fns.c:7312
#2  0x0104a0af in die (msg=0x159401c "assertion failed:
WINDOWP(selected_window)",
    file=0x1593ee0 "xdisp.c", line=1156) at alloc.c:6129
#3  0x0116327a in window_text_bottom_y (w=0x58b2c00) at xdisp.c:1156
#4  0x011c0d07 in erase_phys_cursor (w=0x58b2c00) at xdisp.c:23802
#5  0x011c1fcb in display_and_set_cursor (w=0x58b2c00, on=1, hpos=1, vpos=4,
x=8, y=56)
    at xdisp.c:23936
#6  0x011c20e4 in update_window_cursor (w=0x58b2c00, on=1) at xdisp.c:23973
#7  0x011c229e in update_cursor_in_window_tree (w=0x58b2c00, on_p=1) at
xdisp.c:23993
#8  0x011c23e8 in x_update_cursor (f=0x58b2800, on_p=1) at xdisp.c:24007
#9  0x012eb871 in frame_highlight (f=0x58b2800) at w32term.c:2743
#10 0x012ebd84 in x_frame_rehighlight (dpyinfo=0x16c6918) at w32term.c:2901
#11 0x012ebbb2 in w32_frame_rehighlight (frame=0x58b2800) at w32term.c:2873
#12 0x01288ef3 in Fredirect_frame_focus (frame=93005829, focus_frame=93005829)
    at frame.c:2082
#13 0x0127f4c8 in do_switch_frame (frame=93005829, track=1, for_deletion=0,
    norecord=49010714) at frame.c:847
#14 0x01280733 in Fselect_frame (frame=93005829, norecord=49010714) at
frame.c:899
#15 0x01252702 in Fselect_window (window=93006853, norecord=49010714) at
window.c:3581
#16 0x0125e7c8 in Fset_window_configuration (configuration=99327941) at
window.c:6148
#17 0x0103ca56 in unbind_to (count=39, value=49010714) at eval.c:3370
#18 0x0125fefb in Fsave_window_excursion (args=50081366) at window.c:6453
#19 0x010f43ad in Fbyte_code (bytestr=49145937, vector=49031429, maxdepth=28)
    at bytecode.c:840
#20 0x0103bc8e in funcall_lambda (fun=51265541, nargs=2, arg_vector=0x83ea54)
    at eval.c:3174
#21 0x0103b3a5 in Ffuncall (nargs=3, args=0x83ea50) at eval.c:3037
#22 0x010f3824 in Fbyte_code (bytestr=49040673, vector=54388229, maxdepth=16)
    at bytecode.c:679
#23 0x0103bc8e in funcall_lambda (fun=50492101, nargs=0, arg_vector=0x83ed70)
    at eval.c:3174
#24 0x0103b3a5 in Ffuncall (nargs=1, args=0x83ed6c) at eval.c:3037
#25 0x0103a03d in run_hook_with_args (nargs=1, args=0x83ed6c,
cond=to_completion)
    at eval.c:2679
#26 0x01039ce7 in Frun_hooks (nargs=1, args=0x83ee3c) at eval.c:2542
#27 0x0103aa95 in Ffuncall (nargs=2, args=0x83ee38) at eval.c:2970
#28 0x0103a35a in call1 (fn=49166986, arg1=49056298) at eval.c:2789
#29 0x01007e5a in safe_run_hooks_1 () at keyboard.c:2020
#30 0x010369fd in internal_condition_case (bfun=0x1007e27 <safe_run_hooks_1>,
    handlers=49010738, hfun=0x1007e5c <safe_run_hooks_error>) at eval.c:1460
#31 0x01007efa in safe_run_hooks (hook=49056298) at keyboard.c:2046
#32 0x01006c29 in command_loop_1 () at keyboard.c:1743
#33 0x010369fd in internal_condition_case (bfun=0x100562b <command_loop_1>,
    handlers=49064346, hfun=0x1004d5f <cmd_error>) at eval.c:1460
#34 0x01005231 in command_loop_2 (ignore=49010714) at keyboard.c:1327
#35 0x0103647c in internal_catch (tag=49166602, func=0x100520d <command_loop_2>,
    arg=49010714) at eval.c:1204
#36 0x0100519c in command_loop () at keyboard.c:1292
#37 0x0100446b in recursive_edit_1 () at keyboard.c:929
#38 0x010d394e in read_minibuf (map=60546878, initial=20177105, prompt=60971569,
    backup_n=49010714, expflag=1, histvar=49196394, histpos=0, defalt=49010714,
    allow_props=0, inherit_input_method=0) at minibuf.c:711
#39 0x010d49b3 in Fread_from_minibuffer (prompt=60971569,
initial_contents=20177105,
    keymap=60546878, sys_read=49010738, hist=49196394, default_value=49010714,
    inherit_input_method=49010714) at minibuf.c:998
#40 0x0103b24a in Ffuncall (nargs=8, args=0x83f400) at eval.c:3014
#41 0x010f3824 in Fbyte_code (bytestr=60281009, vector=60530821, maxdepth=32)
    at bytecode.c:679
#42 0x0103bc8e in funcall_lambda (fun=58351653, nargs=5, arg_vector=0x83f620)
    at eval.c:3174
#43 0x0103b6b4 in apply_lambda (fun=58351653, args=60588622, eval_flag=1) at
eval.c:3100
#44 0x01039251 in Feval (form=60588630) at eval.c:2395
#45 0x010389a7 in Feval (form=60588638) at eval.c:2314
#46 0x010f6f4e in Fcall_interactively (function=60488066, record_flag=49010714,
    keys=49031941) at callint.c:345
#47 0x0103af5a in Ffuncall (nargs=4, args=0x83fb60) at eval.c:2995
#48 0x0103a3d4 in call3 (fn=49179562, arg1=60488066, arg2=49010714,
arg3=49010714)
    at eval.c:2820
#49 0x01022793 in Fcommand_execute (cmd=60488066, record_flag=49010714,
keys=49010714,
    special=49010714) at keyboard.c:10393
#50 0x01006bcd in command_loop_1 () at keyboard.c:1726
#51 0x010369fd in internal_condition_case (bfun=0x100562b <command_loop_1>,
    handlers=49064346, hfun=0x1004d5f <cmd_error>) at eval.c:1460
#52 0x01005231 in command_loop_2 (ignore=49010714) at keyboard.c:1327
#53 0x0103647c in internal_catch (tag=49062442, func=0x100520d <command_loop_2>,
    arg=49010714) at eval.c:1204
#54 0x010051ed in command_loop () at keyboard.c:1306
#55 0x0100446b in recursive_edit_1 () at keyboard.c:929
#56 0x01004985 in Frecursive_edit () at keyboard.c:991
#57 0x010027d7 in main (argc=3, argv=0x326d8) at emacs.c:1716
 
Lisp Backtrace:
"fit-frame" (0x83ea54)
"1on1-fit-minibuffer-frame" (0x83ed70)
"run-hooks" (0x83ee3c)
"old-read-from-minibuffer" (0x83f404)
"read-from-minibuffer" (0x83f620)
"list" (0x83f85c)
"call-interactively" (0x83fb64)
(gdb) 
 
-------------------------------
 
Some more info:
 
`fit-frame' is one of my commands. It fits a frame to its (typically
sole) buffer. In this case, it was run on a hook, and it was the
standalone minibuffer frame that was being fit.
 
The hook was no doubt from a run-hooks called in or just after (vanilla)
`read-from-minibuffer' (which is written in C - I don't have the recent
source code).
 
I invoke 1on1-fit-minibuffer-frame in several places, but it is on a
hook in only one place. I put it on post-command-hook.  So when
run-hooks was used for post-command hook, 1on1-fit-minibuffer-frame was
called.
 
This is the code that invoked `fit-frame'.
It is this code that determines which window is selected.
 
(defun 1on1-fit-minibuffer-frame ()
  "Fit the standalone minibuffer frame height to its contents.
Repeat to increase the height by 1.
Bind this in minibuffer keymaps to a key such as `C-o' that you can
use during minibuffer input.
This has no effect if you do not also use library `fit-frame.el'."
  (interactive)
  (unless (require 'fit-frame nil t)
    (error "You need to load library `fit-frame.el' to use this command"))
  ;; We could assume the minibuffer frame is `1on1-minibuffer-frame',
  ;; but we don't.
  (when (and 1on1-fit-minibuffer-frame-flag
             (active-minibuffer-window)
             (save-selected-window
               (select-window (minibuffer-window))
               ;; We should be able to use just (one-window-p),
               ;; but an Emacs bug means we need this:
               (one-window-p nil 'selected-frame)))
    (let* ((frame         (save-selected-window
                            (select-window (minibuffer-window))
                            (selected-frame)))
           (frame-height  (frame-height frame)))
      (cond
        ((eq last-command '1on1-fit-minibuffer-frame)
         (set-frame-height frame (1+ (frame-height frame)))
         (1on1-set-minibuffer-frame-top/bottom)
         (condition-case nil (scroll-down (frame-height frame))
           (error nil)))
        (t
         (let* ((beg  (1on1-minibuffer-prompt-end))
                (fit-frame-max-height
                 1on1-fit-minibuffer-frame-max-height)
                (fit-frame-max-height-percent
                 1on1-fit-minibuffer-frame-max-height-percent)
                (fit-frame-min-height  1on1-minibuffer-frame-height)
                (window-min-height     1on1-minibuffer-frame-height)
                (fit-frame-empty-height  1on1-minibuffer-frame-height)
                (fit-frame-empty-special-display-height
                 1on1-minibuffer-frame-height))
           (fit-frame frame (frame-width frame))
           (1on1-set-minibuffer-frame-top/bottom)
           (condition-case nil (scroll-down (frame-height frame))
             (error nil))))))))
 
The window that was expected to be selected was the currently active
minibuffer window.
 
HTH.
 
-------------------------------
 

In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2010-12-20 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4) --no-opt --cflags
-Ic:/imagesupport/include'
 
Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: ENU
  value of $XMODIFIERS: nil
  locale-coding-system: cp1252
  default enable-multibyte-characters: t
 
Major mode: Dired by name
 
Minor modes in effect:
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t
 
Recent input:
<help-echo> <help-echo> <help-echo> <help-echo> <help-echo> 
<help-echo> <help-echo> <help-echo> <menu-bar> <help-menu> 
<send-emacs-bug-report>
 
Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
 
Load-path shadows:
None found.
 
Features:
(shadow sort gnus-util mail-extr message rfc822 mml easymenu mml-sec
mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mailabbrev mail-utils gmm-utils mailheader
emacsbug dired regexp-opt tooltip ediff-hook vc-hooks lisp-float-type
mwheel dos-w32 disp-table ls-lisp w32-win w32-vars tool-bar dnd fontset
image fringe lisp-mode register page menu-bar rfn-eshadow timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core frame cham
georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese hebrew greek romanian slovak czech european ethiopic
indian cyrillic chinese case-table epa-hook jka-cmpr-hook help simple
abbrev button minibuffer faces cus-face files text-properties overlay
md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote make-network-process multi-tty emacs)

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: "Drew Adams" <drew.adams@oracle.com>
> Date: Fri, 24 Dec 2010 08:55:20 -0800
> Cc: 
> 
> Program received signal SIGTRAP, Trace/breakpoint trap.
> [Switching to Thread 3684.0x1210]
> 0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
> (gdb) bt
> #0  0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
> #1  0x01309b9f in w32_abort () at w32fns.c:7312
> #2  0x0104a0af in die (msg=0x159401c "assertion failed:
> WINDOWP(selected_window)",
>     file=0x1593ee0 "xdisp.c", line=1156) at alloc.c:6129
> #3  0x0116327a in window_text_bottom_y (w=0x58b2c00) at xdisp.c:1156

The abort happens in this source line in window_text_bottom_y:

    height -= CURRENT_MODE_LINE_HEIGHT (w);

The macro CURRENT_MODE_LINE_HEIGHT accesses selected_window:

  #define CURRENT_MODE_LINE_HEIGHT(W)				\
       (current_mode_line_height >= 0				\
	? current_mode_line_height				\
	: (MATRIX_MODE_LINE_HEIGHT ((W)->current_matrix)	\
	   ? MATRIX_MODE_LINE_HEIGHT ((W)->current_matrix)	\
	   : estimate_mode_line_height (XFRAME ((W)->frame),	\
					CURRENT_MODE_LINE_FACE_ID (W))))

  #define CURRENT_MODE_LINE_FACE_ID(W)		\
	  (CURRENT_MODE_LINE_FACE_ID_3((W), XWINDOW (selected_window), (W)))

and XWINDOW tries to assert (under ENABLE_CHECKING) that its argument
is indeed a window.

So it looks like somehow we get into selected_window a value that is
not a window.  I'm guessing it was nil in this case.

If having selected_window that is not a window is legitimate, then one
possible solution would be to extend CURRENT_MODE_LINE_FACE_ID to
handle that case gracefully.  After all, all it does is choose between
MODE_LINE_FACE_ID and MODE_LINE_INACTIVE_FACE_ID.

bug#7728: 24.0.50; GDB backtrace from abort

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> So it looks like somehow we get into selected_window a value that is
> not a window.  I'm guessing it was nil in this case.

There is exactly one place where selected_window can be nil:

/* Note that selected_window can be nil when this is called from
   Fset_window_configuration.  */

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Drew Adams <drew.adams@oracle.com>,  7728@debbugs.gnu.org
> Date: Sat, 25 Dec 2010 11:44:21 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So it looks like somehow we get into selected_window a value that is
> > not a window.  I'm guessing it was nil in this case.
> 
> There is exactly one place where selected_window can be nil:
> 
> /* Note that selected_window can be nil when this is called from
>    Fset_window_configuration.  */

And Fset_window_configuration->Fselect_window call is indeed on the
call stack.

bug#7728: 24.0.50; GDB backtrace from abort

[Stefan Monnier]

>> Program received signal SIGTRAP, Trace/breakpoint trap.
>> [Switching to Thread 3684.0x1210]
>> 0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
>> (gdb) bt
>> #0  0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
>> #1  0x01309b9f in w32_abort () at w32fns.c:7312
>> #2  0x0104a0af in die (msg=0x159401c "assertion failed:
>> WINDOWP(selected_window)",
>> file=0x1593ee0 "xdisp.c", line=1156) at alloc.c:6129
>> #3  0x0116327a in window_text_bottom_y (w=0x58b2c00) at xdisp.c:1156

> The abort happens in this source line in window_text_bottom_y:

>     height -= CURRENT_MODE_LINE_HEIGHT (w);

> The macro CURRENT_MODE_LINE_HEIGHT accesses selected_window:

>   #define CURRENT_MODE_LINE_HEIGHT(W)				\
>        (current_mode_line_height >= 0				\
> 	? current_mode_line_height				\
> 	: (MATRIX_MODE_LINE_HEIGHT ((W)->current_matrix)	\
> 	   ? MATRIX_MODE_LINE_HEIGHT ((W)->current_matrix)	\
> 	   : estimate_mode_line_height (XFRAME ((W)->frame),	\
> 					CURRENT_MODE_LINE_FACE_ID (W))))

>   #define CURRENT_MODE_LINE_FACE_ID(W)		\
> 	  (CURRENT_MODE_LINE_FACE_ID_3((W), XWINDOW (selected_window), (W)))

> and XWINDOW tries to assert (under ENABLE_CHECKING) that its argument
> is indeed a window.

> So it looks like somehow we get into selected_window a value that is
> not a window.  I'm guessing it was nil in this case.

If selected_window is nil because of Fset_window_configuration, then it
is presumably nil for all the intervening and some of the subsequent
code, and I suspect fixing it earlier in the call chain will be
preferable: e.g., it doesn't make sense to "display_and_set_cursor" in
the "selected_window = nil" case.


        Stefan

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Drew Adams <drew.adams@oracle.com>,  7728@debbugs.gnu.org
> Date: Sat, 25 Dec 2010 15:35:31 -0500
> 
> If selected_window is nil because of Fset_window_configuration, then it
> is presumably nil for all the intervening and some of the subsequent
> code, and I suspect fixing it earlier in the call chain will be
> preferable: e.g., it doesn't make sense to "display_and_set_cursor" in
> the "selected_window = nil" case.

After some looking around, I'm sorry to report that I don't see how to
do that.  Here are the details:

We set selected_window to nil as a semi-kludgey way of preventing
select-window from storing in the old selected window the value of
point of the buffer that has been restored into that window.
select-window has this code:

  sf = SELECTED_FRAME ();
  if (XFRAME (WINDOW_FRAME (w)) != sf)
    {
      XFRAME (WINDOW_FRAME (w))->selected_window = window;
      /* Use this rather than Fhandle_switch_frame
	 so that FRAME_FOCUS_FRAME is moved appropriately as we
	 move around in the state where a minibuffer in a separate
	 frame is active.  */
      Fselect_frame (WINDOW_FRAME (w), norecord);
      /* Fselect_frame called us back so we've done all the work already.  */
      eassert (EQ (window, selected_window));
      return window;
    }
  else
    sf->selected_window = window;

  /* Store the current buffer's actual point into the
     old selected window.  It belongs to that window,
     and when the window is not selected, must be in the window.  */
  if (!NILP (selected_window))
    {
      ow = XWINDOW (selected_window);
      if (! NILP (ow->buffer))
	set_marker_both (ow->pointm, ow->buffer,
			 BUF_PT (XBUFFER (ow->buffer)),
			 BUF_PT_BYTE (XBUFFER (ow->buffer)));
    }

  selected_window = window;

The last `if' clause is what we want to bypass, when we restore window
configuration as the last part of save-window-excursion.  Note that
select-window stores the right value into selected_window right after
that, but the call to Fselect_frame that crashes happens before that,
so selected_window is still nil.

Now, the sequence of calls leading to the crash inside Fselect_frame
is as follows:

  Fselect_frame
  -> do_switch_frame
     -> Fredirect_frame_focus
        -> w32_frame_rehighlight / XTframe_rehighlight
           -> x_frame_rehighlight
              -> frame_highlight
                 -> x_update_cursor
                    -> update_cursor_in_window_tree
                       -> update_window_cursor
                          -> display_and_set_cursor
                             -> erase_phys_cursor
                                -> window_text_bottom_y
                                   -> die

My conclusion after studying this is that everything that happens
below Fselect_frame is reasonable: we switch to the frame and redraw
the cursor in all of its windows.  In particular,
update_cursor_in_window_tree simply walks the entire window tree of
the newly selected frame.  I don't see how we can avoid any of this
when selected_window is nil, because selected_window has nothing to do
with the windows that are being processed.  None of these functions
even references selected_window, which is TRT.  The first place that
does reference selected_window is the CURRENT_MODE_LINE_HEIGHT macro
used in window_text_bottom_y, and that leads to the abort.

So I see 2 ways to prevent this particular problem:

 1) Handle the case of selected_window == Qnil in
    CURRENT_MODE_LINE_FACE_ID.

 2) Change the code of Fset_window_configuration and Fselect_window,
    to have some other way of preventing the latter from storing point
    in the old selected window, without setting selected_window to
    nil.

Any other ideas are welcome.

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

Ping!

This is a bug that I think we should fix for Emacs 23.3.

> Date: Sat, 01 Jan 2011 20:02:17 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 7728@debbugs.gnu.org
> 
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: Drew Adams <drew.adams@oracle.com>,  7728@debbugs.gnu.org
> > Date: Sat, 25 Dec 2010 15:35:31 -0500
> > 
> > If selected_window is nil because of Fset_window_configuration, then it
> > is presumably nil for all the intervening and some of the subsequent
> > code, and I suspect fixing it earlier in the call chain will be
> > preferable: e.g., it doesn't make sense to "display_and_set_cursor" in
> > the "selected_window = nil" case.
> 
> After some looking around, I'm sorry to report that I don't see how to
> do that.  Here are the details:
> 
> We set selected_window to nil as a semi-kludgey way of preventing
> select-window from storing in the old selected window the value of
> point of the buffer that has been restored into that window.
> select-window has this code:
> 
>   sf = SELECTED_FRAME ();
>   if (XFRAME (WINDOW_FRAME (w)) != sf)
>     {
>       XFRAME (WINDOW_FRAME (w))->selected_window = window;
>       /* Use this rather than Fhandle_switch_frame
> 	 so that FRAME_FOCUS_FRAME is moved appropriately as we
> 	 move around in the state where a minibuffer in a separate
> 	 frame is active.  */
>       Fselect_frame (WINDOW_FRAME (w), norecord);
>       /* Fselect_frame called us back so we've done all the work already.  */
>       eassert (EQ (window, selected_window));
>       return window;
>     }
>   else
>     sf->selected_window = window;
> 
>   /* Store the current buffer's actual point into the
>      old selected window.  It belongs to that window,
>      and when the window is not selected, must be in the window.  */
>   if (!NILP (selected_window))
>     {
>       ow = XWINDOW (selected_window);
>       if (! NILP (ow->buffer))
> 	set_marker_both (ow->pointm, ow->buffer,
> 			 BUF_PT (XBUFFER (ow->buffer)),
> 			 BUF_PT_BYTE (XBUFFER (ow->buffer)));
>     }
> 
>   selected_window = window;
> 
> The last `if' clause is what we want to bypass, when we restore window
> configuration as the last part of save-window-excursion.  Note that
> select-window stores the right value into selected_window right after
> that, but the call to Fselect_frame that crashes happens before that,
> so selected_window is still nil.
> 
> Now, the sequence of calls leading to the crash inside Fselect_frame
> is as follows:
> 
>   Fselect_frame
>   -> do_switch_frame
>      -> Fredirect_frame_focus
>         -> w32_frame_rehighlight / XTframe_rehighlight
>            -> x_frame_rehighlight
>               -> frame_highlight
>                  -> x_update_cursor
>                     -> update_cursor_in_window_tree
>                        -> update_window_cursor
>                           -> display_and_set_cursor
>                              -> erase_phys_cursor
>                                 -> window_text_bottom_y
>                                    -> die
> 
> My conclusion after studying this is that everything that happens
> below Fselect_frame is reasonable: we switch to the frame and redraw
> the cursor in all of its windows.  In particular,
> update_cursor_in_window_tree simply walks the entire window tree of
> the newly selected frame.  I don't see how we can avoid any of this
> when selected_window is nil, because selected_window has nothing to do
> with the windows that are being processed.  None of these functions
> even references selected_window, which is TRT.  The first place that
> does reference selected_window is the CURRENT_MODE_LINE_HEIGHT macro
> used in window_text_bottom_y, and that leads to the abort.
> 
> So I see 2 ways to prevent this particular problem:
> 
>  1) Handle the case of selected_window == Qnil in
>     CURRENT_MODE_LINE_FACE_ID.
> 
>  2) Change the code of Fset_window_configuration and Fselect_window,
>     to have some other way of preventing the latter from storing point
>     in the old selected window, without setting selected_window to
>     nil.
> 
> Any other ideas are welcome.
> 

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

Here is another crash.  Dunno whether this is the same bug.  If it is, and if
the additional info here doesn't help, then ignore.  If it is a different bug,
please create a different bug for this info.  HTH. 

$ ./gdb -p 1580
GNU gdb (GDB) 7.2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Attaching to process 1580
[New Thread 1580.0x1604]
[New Thread 1580.0x1400]
[New Thread 1580.0x1464]
[New Thread 1580.0x1334]
Reading symbols from C:\Emacs-24-2011-01-03\bin\emacs.exe...done.
[Switching to Thread 1580.0x1334]
Warning: c:\drews-lisp-20\bin/../lwlib: No such file or directory.
SIGINT is used by the debugger.
Are you sure you want to change it? (y or n) [answered Y; input not from
terminal]
Environment variable "DISPLAY" not defined.
TERM = cygwin
Breakpoint 1 at 0x1308a1c: file w32fns.c, line 7298.
Temporary breakpoint 2 at 0x11cf0c9: file sysdep.c, line 839.
(gdb) c
Continuing.

Program received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 1580.0x1604]
0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
(gdb) bt
#0  0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
#1  0x01308a53 in w32_abort () at w32fns.c:7312
#2  0x01049f03 in die (msg=0x1592fbc "assertion failed:
WINDOWP(selected_window)",
    file=0x1592e80 "xdisp.c", line=1156) at alloc.c:6129
#3  0x01162d06 in window_text_bottom_y (w=0x3865a00) at xdisp.c:1156
#4  0x011c0737 in erase_phys_cursor (w=0x3865a00) at xdisp.c:23805
#5  0x011c19fb in display_and_set_cursor (w=0x3865a00, on=1, hpos=2, vpos=6,
x=16, y=87)
    at xdisp.c:23939
#6  0x011c1b14 in update_window_cursor (w=0x3865a00, on=1) at xdisp.c:23976
#7  0x011c1cce in update_cursor_in_window_tree (w=0x3865a00, on_p=1) at
xdisp.c:23996
#8  0x011c1e18 in x_update_cursor (f=0x31aee00, on_p=1) at xdisp.c:24010
#9  0x012ea74b in frame_unhighlight (f=0x31aee00) at w32term.c:2750
#10 0x012eac1a in x_frame_rehighlight (dpyinfo=0x16c5928) at w32term.c:2899
#11 0x012eaa66 in w32_frame_rehighlight (frame=0x31aee00) at w32term.c:2873
#12 0x01288a2b in Fredirect_frame_focus (frame=52096517, focus_frame=58688517)
    at frame.c:2082
#13 0x0127f000 in do_switch_frame (frame=58688517, track=1, for_deletion=0,
    norecord=49006618) at frame.c:847
#14 0x0128026b in Fselect_frame (frame=58688517, norecord=49006618) at
frame.c:899
#15 0x01252132 in Fselect_window (window=59212805, norecord=49006618) at
window.c:3586
#16 0x0125e2f5 in Fset_window_configuration (configuration=52819845) at
window.c:6159
#17 0x0103c8aa in unbind_to (count=42, value=64852305) at eval.c:3325
#18 0x010d3ffa in read_minibuf (map=48995734, initial=66477985, prompt=54882353,
    backup_n=20, expflag=0, histvar=49192130, histpos=0, defalt=66477985,
    allow_props=1, inherit_input_method=0) at minibuf.c:805
#19 0x010d4807 in Fread_from_minibuffer (prompt=54882353,
initial_contents=64829062,
    keymap=48995734, sys_read=49006618, hist=49192130, default_value=66477985,
    inherit_input_method=49006618) at minibuf.c:998
#20 0x0103b09e in Ffuncall (nargs=8, args=0x83e920) at eval.c:2969
#21 0x010f32b0 in Fbyte_code (bytestr=59837009, vector=55670533, maxdepth=32)
    at bytecode.c:679
#22 0x0103bae2 in funcall_lambda (fun=52611557, nargs=7, arg_vector=0x83ebe4)
    at eval.c:3129
#23 0x0103b1f9 in Ffuncall (nargs=8, args=0x83ebe0) at eval.c:2992
#24 0x010f32b0 in Fbyte_code (bytestr=60202865, vector=60196869, maxdepth=32)
    at bytecode.c:679
#25 0x0103bae2 in funcall_lambda (fun=50799653, nargs=8, arg_vector=0x83eea4)
    at eval.c:3129
#26 0x0103b1f9 in Ffuncall (nargs=9, args=0x83eea0) at eval.c:2992
#27 0x010f32b0 in Fbyte_code (bytestr=58090801, vector=55460933, maxdepth=36)
    at bytecode.c:679
#28 0x01038de5 in Feval (form=59495286) at eval.c:2358
#29 0x01036494 in internal_catch (tag=60883186, func=0x10382ac <Feval>,
arg=59495286)
    at eval.c:1206
#30 0x010f3ead in Fbyte_code (bytestr=60199057, vector=55375621, maxdepth=40)
    at bytecode.c:854
#31 0x0103bae2 in funcall_lambda (fun=50800133, nargs=7, arg_vector=0x83f440)
    at eval.c:3129
#32 0x0103b508 in apply_lambda (fun=50800133, args=57728030, eval_flag=1) at
eval.c:3055
#33 0x01039269 in Feval (form=57728086) at eval.c:2397
#34 0x01034591 in Fsetq (args=57728094) at eval.c:496
#35 0x010387e8 in Feval (form=57728102) at eval.c:2299
#36 0x01034415 in Fprogn (args=57921326) at eval.c:397
#37 0x01035fd9 in Flet (args=57728110) at eval.c:1054
#38 0x010387e8 in Feval (form=57728246) at eval.c:2299
#39 0x010f69da in Fcall_interactively (function=49554322, record_flag=49006618,
    keys=49027845) at callint.c:345
#40 0x0103adae in Ffuncall (nargs=4, args=0x83fb60) at eval.c:2950
#41 0x0103a228 in call3 (fn=49175466, arg1=49554322, arg2=49006618,
arg3=49006618)
    at eval.c:2775
#42 0x010227ad in Fcommand_execute (cmd=49554322, record_flag=49006618,
keys=49006618,
    special=49006618) at keyboard.c:10393
#43 0x01006bcd in command_loop_1 () at keyboard.c:1726
#44 0x01036a15 in internal_condition_case (bfun=0x100562b <command_loop_1>,
    handlers=49060250, hfun=0x1004d5f <cmd_error>) at eval.c:1462
#45 0x01005231 in command_loop_2 (ignore=49006618) at keyboard.c:1327
#46 0x01036494 in internal_catch (tag=49058346, func=0x100520d <command_loop_2>,
    arg=49006618) at eval.c:1206
#47 0x010051ed in command_loop () at keyboard.c:1306
#48 0x0100446b in recursive_edit_1 () at keyboard.c:929
#49 0x01004985 in Frecursive_edit () at keyboard.c:991
#50 0x010027d7 in main (argc=3, argv=0x33c80) at emacs.c:1716

Lisp Backtrace:
"old-read-from-minibuffer" (0x83e924)
"read-from-minibuffer" (0x83ebe4)
"icicle-lisp-vanilla-completing-read" (0x83eea4)
"byte-code" (0x83f0c4)
"completing-read" (0x83f440)
"setq" (0x83f69c)
"let" (0x83f85c)
"call-interactively" (0x83fb64)
(gdb) frame 2
#2  0x01049f03 in die (msg=0x1592fbc "assertion failed:
WINDOWP(selected_window)",
    file=0x1592e80 "xdisp.c", line=1156) at alloc.c:6129
6129    alloc.c: No such file or directory.
        in alloc.c
(gdb) p selected_window
$1 = 49006618
(gdb) xtype
Lisp_Symbol
(gdb) xsymbol
$2 = (struct Lisp_Symbol *) 0x2ebc818
"nil"
(gdb)

In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2011-01-03 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4) --no-opt --cflags
-Ic:/imagesupport/include'

bug#7728: 24.0.50; GDB backtrace from abort

[Stefan Monnier]

> My conclusion after studying this is that everything that happens
> below Fselect_frame is reasonable: we switch to the frame and redraw
> the cursor in all of its windows.  In particular,
> update_cursor_in_window_tree simply walks the entire window tree of
> the newly selected frame.  I don't see how we can avoid any of this
> when selected_window is nil, because selected_window has nothing to do
> with the windows that are being processed.  None of these functions
> even references selected_window, which is TRT.

Yes, I see that as well, now.

> The first place that does reference selected_window is the
> CURRENT_MODE_LINE_HEIGHT macro used in window_text_bottom_y, and that
> leads to the abort.

There's still one thing I don't understand: why do we call
Fselect_frame?  AFAICT, Fset_window_configuration has no reason to
select a new frame, it all works within the selected-frame.

> So I see 2 ways to prevent this particular problem:
>  1) Handle the case of selected_window == Qnil in
>     CURRENT_MODE_LINE_FACE_ID.

But should it always return the mode-line-inactive face here, or should
it always return the mode-line face?

>  2) Change the code of Fset_window_configuration and Fselect_window,
>     to have some other way of preventing the latter from storing point
>     in the old selected window, without setting selected_window to
>     nil.

That sounds like a better solution.  E.g. move the code of
Fselect_window to another function, add a third argument to it
specifying whether to swap-out point in selected_window, and make
Fset_window_configuration call that new internal function.

But maybe Fselect_frame should simply not be run in this case.


        Stefan

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: drew.adams@oracle.com, 7728@debbugs.gnu.org
> Date: Tue, 11 Jan 2011 15:55:12 -0500
> 
> There's still one thing I don't understand: why do we call
> Fselect_frame?  AFAICT, Fset_window_configuration has no reason to
> select a new frame, it all works within the selected-frame.

Probably because of minibuffer-only frames or something.

Perhaps Drew could publish the relevant parts of the window
configuration that was being restored in that case (or any other
similar case).

> > So I see 2 ways to prevent this particular problem:
> >  1) Handle the case of selected_window == Qnil in
> >     CURRENT_MODE_LINE_FACE_ID.
> 
> But should it always return the mode-line-inactive face here, or should
> it always return the mode-line face?

I don't think it matters much, since if we don't have a window to work
with, we are only guesstimating anyway.

> >  2) Change the code of Fset_window_configuration and Fselect_window,
> >     to have some other way of preventing the latter from storing point
> >     in the old selected window, without setting selected_window to
> >     nil.
> 
> That sounds like a better solution.  E.g. move the code of
> Fselect_window to another function, add a third argument to it
> specifying whether to swap-out point in selected_window, and make
> Fset_window_configuration call that new internal function.

Yes, something like that.

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> > There's still one thing I don't understand: why do we call
> > Fselect_frame?  AFAICT, Fset_window_configuration has no reason to
> > select a new frame, it all works within the selected-frame.
> 
> Probably because of minibuffer-only frames or something.
> 
> Perhaps Drew could publish the relevant parts of the window
> configuration that was being restored in that case (or any other
> similar case).

Sorry, I don't follow this at all at the C level, and even at the Lisp level I'm
not sure I can be much help here.

In the last GDB backtrace I sent (yesterday), I did the following, in my version
of Emacs, in Icicle mode:

C-h f  g r a p h i c  S-TAB

Then click `mouse-2' on completion candidate `display-graphic-p'.  That should
end completion and show *Help* (in a separate frame, in my setup) with the
output of `describe-function'.  Instead, Emacs crashes (or whatever you call it
- no error raised).

That crash (or whatever it is) is reproducible in my setup.  But as there are so
many things going on (I use a standalone minibuffer frame, etc.) it's unclear
how much help that description is, except that it is probably not very helpful.

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: "Drew Adams" <drew.adams@oracle.com>
> Cc: <7728@debbugs.gnu.org>
> Date: Tue, 11 Jan 2011 13:44:34 -0800
> 
> > Perhaps Drew could publish the relevant parts of the window
> > configuration that was being restored in that case (or any other
> > similar case).
> 
> Sorry, I don't follow this at all at the C level, and even at the Lisp level I'm
> not sure I can be much help here.

I think you can.  This is about the crash you originally posted in
this bug, the one that happened because some window configuration was
being restored.  Here's the Lisp backtrace you posted:

  "fit-frame" (0x83ea54)
  "1on1-fit-minibuffer-frame" (0x83ed70)
  "run-hooks" (0x83ee3c)
  "old-read-from-minibuffer" (0x83f404)
  "read-from-minibuffer" (0x83f620)
  "list" (0x83f85c)
  "call-interactively" (0x83fb64)

You also said back then:

> `fit-frame' is one of my commands. It fits a frame to its (typically
> sole) buffer. In this case, it was run on a hook, and it was the
> standalone minibuffer frame that was being fit.
>  
> The hook was no doubt from a run-hooks called in or just after (vanilla)
> `read-from-minibuffer' (which is written in C - I don't have the recent
> source code).

I'm guessing that somewhere inside fit-frame you have code that
restores configuration of windows that was previously saved.  I was
asking for showing the relevant parts of that saved configuration,
that would perhaps explain why set-window-configuration needs to
select a different frame.

> In the last GDB backtrace I sent (yesterday), I did the following, in my version
> of Emacs, in Icicle mode:
> 
> C-h f  g r a p h i c  S-TAB
> 
> Then click `mouse-2' on completion candidate `display-graphic-p'.  That should
> end completion and show *Help* (in a separate frame, in my setup) with the
> output of `describe-function'.  Instead, Emacs crashes (or whatever you call it
> - no error raised).
> 
> That crash (or whatever it is) is reproducible in my setup.

This crash is identical to the first one.  So if you can show the
window configuration data structure used by set-window-configuration
in this case, it would be helpful.  TIA

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> > > Perhaps Drew could publish the relevant parts of the window
> > > configuration that was being restored in that case (or any other
> > > similar case).
> > 
> > Sorry, I don't follow this at all at the C level, and even 
> > at the Lisp level I'm not sure I can be much help here.
> 
> I think you can.  This is about the crash you originally posted in
> this bug, the one that happened because some window configuration was
> being restored.

(C'est toi qui le dis. ;-))  I don't think I was restoring any window config,
but something might have been doing so.

> Here's the Lisp backtrace you posted:
>   "fit-frame" (0x83ea54)
>   "1on1-fit-minibuffer-frame" (0x83ed70)
>   "run-hooks" (0x83ee3c)
>   "old-read-from-minibuffer" (0x83f404)
>   "read-from-minibuffer" (0x83f620)
>   "list" (0x83f85c)
>   "call-interactively" (0x83fb64)
> 
> You also said back then:
> 
> > `fit-frame' is one of my commands. It fits a frame to its (typically
> > sole) buffer. In this case, it was run on a hook, and it was the
> > standalone minibuffer frame that was being fit.
> >  
> > The hook was no doubt from a run-hooks called in or just 
> > after (vanilla) `read-from-minibuffer' (which is written
> > in C - I don't have the recent source code).
> 
> I'm guessing that somewhere inside fit-frame you have code that
> restores configuration of windows that was previously saved.  I was
> asking for showing the relevant parts of that saved configuration,
> that would perhaps explain why set-window-configuration needs to
> select a different frame.

No, `fit-frame' does not restore any window config (unless Emacs does something
like that under the covers somehow), and it does not save any window config.  It
simply calculates the size of the buffer in terms of width and height, and
changes the frame width and height to fit it.  The code is here:
http://www.emacswiki.org/emacs/fit-frame.el

And `1on1-fit-minibuffer-frame' is here:
http://www.emacswiki.org/emacs/oneonone.el

The call to `fit-frame' in `1on1-fit-minibuffer-frame' is this:
(fit-frame frame (frame-width frame))
where frame is the standalone minibuffer frame.

> > In the last GDB backtrace I sent (yesterday), I did the 
> > following, in my version of Emacs, in Icicle mode:
> > 
> > C-h f  g r a p h i c  S-TAB
> > 
> > Then click `mouse-2' on completion candidate `display-graphic-p'.
> > That should end completion and show *Help* (in a separate frame,
> > in my setup) with the output of `describe-function'.  Instead,
> > Emacs crashes (or whatever you call it - no error raised).
> > 
> > That crash (or whatever it is) is reproducible in my setup.
> 
> This crash is identical to the first one.  So if you can show the
> window configuration data structure used by set-window-configuration
> in this case, it would be helpful.  TIA

I do not use `set-window-configuration' in my code at all.  I don't do anything
to or with window configurations in any of my code.


Here's some more info that might (or might not) help, though:

Since the latest crash is easily reproducible, I loaded fit-frame.el (not .elc)
and found that I could _not_ reproduce it - no crash.  IOW, when I load the
byte-compiled file Emacs crashes, but with the source file it does not crash.

What's more, I typically byte-compile in Emacs 20 (unless the library in
question requires a later version). But I've tested this with fit-frame.el that
was compiled using Emacs 20 and using the latest Emacs 24 Windows binary.
Either fit-frame.elc provokes the crash (in Emacs 24 only).

So something in the byte-compiled file leads to the "crash", whereas no error is
raised and no crash occurs if I load fit-frame.el.

HTH.

bug#7728: 24.0.50; GDB backtrace from abort

[martin rudalics]

 >>  2) Change the code of Fset_window_configuration and Fselect_window,
 >>     to have some other way of preventing the latter from storing point
 >>     in the old selected window, without setting selected_window to
 >>     nil.
 >
 > That sounds like a better solution.  E.g. move the code of
 > Fselect_window to another function, add a third argument to it
 > specifying whether to swap-out point in selected_window, and make
 > Fset_window_configuration call that new internal function.

The

       /* Store the current buffer's actual point into the
	 old selected window.  It belongs to that window,
	 and when the window is not selected, must be in the window.  */
       if (!NILP (selected_window))
	{
	  ow = XWINDOW (selected_window);
	  if (! NILP (ow->buffer))
	    ...

part of Fselect_window should be executed _before_ the

       sf = SELECTED_FRAME ();
       if (XFRAME (WINDOW_FRAME (w)) != sf)
	{
	  ...

part, so the selected window would have been already set.  Unfortunately
this would make not_selected_before false when Fselect_window is called
back by Fselect_frame and the remaining parts of Fselect_window starting
with

   Fset_buffer (w->buffer);

   if (NILP (norecord))
   ...

would not get executed in that case.  Probably, there should be a common
subroutine of Fselect_window and Fselect_frame such that the two won't
have to call each other mutually.  (That common subroutine would have to
be robust in the sense that it can't call back neither Fselect_frame nor
Fselect_window.)

 > But maybe Fselect_frame should simply not be run in this case.

If I understand Eli's backtrace correctly, the problem was caused within
the following part of Fset_window_configuration

       selected_window = Qnil;

       /* Arrange *not* to restore point in the buffer that was
	 current when the window configuration was saved.  */
       if (EQ (XWINDOW (data->current_window)->buffer, new_current_buffer))
	set_marker_restricted (XWINDOW (data->current_window)->pointm,
			       make_number (old_point),
			       XWINDOW (data->current_window)->buffer);

       Fselect_window (data->current_window, Qnil);

so apparently the frame of data->current_window is _not_ the selected
frame.  Would you select the window and keep the old frame selected?

martin

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: "Drew Adams" <drew.adams@oracle.com>
> Cc: <monnier@IRO.UMontreal.CA>, <7728@debbugs.gnu.org>
> Date: Tue, 11 Jan 2011 20:59:52 -0800
> 
> I do not use `set-window-configuration' in my code at all.  I don't do anything
> to or with window configurations in any of my code.

But I see that fit-frame uses save-window-excursion.  So it will call
set-window-configuration after the form inside save-window-excursion
is evaluated.

Can you see what window configuration is saved and restored there, and
post that configuration here?

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

>        /* Arrange *not* to restore point in the buffer that was
> 	 current when the window configuration was saved.  */
>        if (EQ (XWINDOW (data->current_window)->buffer, 
> new_current_buffer))
> 	set_marker_restricted (XWINDOW (data->current_window)->pointm,
> 			       make_number (old_point),
> 			       XWINDOW (data->current_window)->buffer);
>        Fselect_window (data->current_window, Qnil);
> 
> so apparently the frame of data->current_window is _not_ the selected
> frame.  Would you select the window and keep the old frame selected?

Believe me, I am not pretending that I actually follow this or have anything
germane to add.  But in the case in question, I can say what I was doing at the
time:

During completion with a standalone minibuffer, I clicked mouse-2 on a
completion candidate in standalone frame *Completions* (which has its input
redirected to the standalone minibuffer).  *Completions* was no doubt _not_ the
selected window.  Probably the minibuffer window was selected at the time.

No idea whether that helps.  If not, ignore.

bug#7728: 24.0.50; GDB backtrace from abort

[Stefan Monnier]

> so apparently the frame of data->current_window is _not_ the selected
> frame.  Would you select the window and keep the old frame selected?

Shouldn't (doesn't?) set-window-configuration simply signal an error if
the selected-frame is different from the frame from which the
window-config comes?


        Stefan

bug#7728: 24.0.50; GDB backtrace from abort

[martin rudalics]

 > Shouldn't (doesn't?) set-window-configuration simply signal an error if
 > the selected-frame is different from the frame from which the
 > window-config comes?

By design `set-window-configuration' does not care about which frame or
window is selected at the time it is called.  IIUC it simply restores
the configuration of the frame of the first saved window in its
CONFIGURATION argument via

   frame = XWINDOW (SAVED_WINDOW_N (saved_windows, 0)->window)->frame;

martin

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  7728@debbugs.gnu.org
> Date: Wed, 12 Jan 2011 10:14:09 -0500
> 
> > so apparently the frame of data->current_window is _not_ the selected
> > frame.  Would you select the window and keep the old frame selected?
> 
> Shouldn't (doesn't?) set-window-configuration simply signal an error if
> the selected-frame is different from the frame from which the
> window-config comes?

How could it?  For example, Drew's code that causes this crash does
this:

    (save-window-excursion (select-frame frame)
                           (or (eq major-mode 'image-mode)
                               image-minor-mode))

Are you saying tha this isn't kosher, because the body of
save-window-excursion is not allowed to select a different frame?

Or am I missing something?

bug#7728: 24.0.50; GDB backtrace from abort

[martin rudalics]

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

 > Are you saying tha this isn't kosher, because the body of
 > save-window-excursion is not allowed to select a different frame?

I doubt it's kosher because if the old selected window is not on the
restored frame and a window on the restored frame gets selected, the
point of the buffer whose window is deselected is not stored in the old
selected window's pointm which is certainly not TRT.

Inherently, the source of all evil is the idea to have (1) these two
variables selected_frame and selected_window and (2) to allow setting
them independently from each other.

To avoid the present crash we could try something like the attached
patch (which does not try to solve anything but that crash).

martin

[-- Attachment #2: window.c.diff --]
[-- Type: text/plain, Size: 2668 bytes --]

*** c:/window.c	2011-01-07 08:25:53.406250000 +0100
--- c:/window.c	2011-01-12 18:25:01.859375000 +0100
***************
*** 158,163 ****
--- 158,167 ----
  
  static int window_initialized;
  
+ /* ...  */
+ 
+ static int inhibit_point_swap;
+ 
  /* Hook to run when window config changes.  */
  
  static Lisp_Object Qwindow_configuration_change_hook;
***************
*** 3594,3600 ****
    /* Store the current buffer's actual point into the
       old selected window.  It belongs to that window,
       and when the window is not selected, must be in the window.  */
!   if (!NILP (selected_window))
      {
        ow = XWINDOW (selected_window);
        if (! NILP (ow->buffer))
--- 3598,3606 ----
    /* Store the current buffer's actual point into the
       old selected window.  It belongs to that window,
       and when the window is not selected, must be in the window.  */
!   if (inhibit_point_swap)
!     inhibit_point_swap = 0;
!   else
      {
        ow = XWINDOW (selected_window);
        if (! NILP (ow->buffer))
***************
*** 3602,3607 ****
--- 3608,3615 ----
  			 BUF_PT (XBUFFER (ow->buffer)),
  			 BUF_PT_BYTE (XBUFFER (ow->buffer)));
      }
+   else
+     inhibit_point_swap = 0;
  
    selected_window = window;
  
***************
*** 5767,5773 ****
      /* This test is needed to make sure PT/PT_BYTE make sense in w->buffer
         when passed below to set_marker_both.  */
      error ("move-to-window-line called from unrelated buffer");
!   
    window = selected_window;
    start = marker_position (w->start);
    if (start < BEGV || start > ZV)
--- 5775,5781 ----
      /* This test is needed to make sure PT/PT_BYTE make sense in w->buffer
         when passed below to set_marker_both.  */
      error ("move-to-window-line called from unrelated buffer");
! 
    window = selected_window;
    start = marker_position (w->start);
    if (start < BEGV || start > ZV)
***************
*** 6147,6153 ****
        /* Prevent "swapping out point" in the old selected window
  	 using the buffer that has been restored into it.
  	 We already swapped out point that from that window's old buffer.  */
!       selected_window = Qnil;
  
        /* Arrange *not* to restore point in the buffer that was
  	 current when the window configuration was saved.  */
--- 6155,6161 ----
        /* Prevent "swapping out point" in the old selected window
  	 using the buffer that has been restored into it.
  	 We already swapped out point that from that window's old buffer.  */
!       inhibit_point_swap = 1;
  
        /* Arrange *not* to restore point in the buffer that was
  	 current when the window configuration was saved.  */

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> Date: Wed, 12 Jan 2011 18:42:55 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: Stefan Monnier <monnier@iro.umontreal.ca>, 7728@debbugs.gnu.org
> 
>  > Are you saying tha this isn't kosher, because the body of
>  > save-window-excursion is not allowed to select a different frame?
> 
> I doubt it's kosher because if the old selected window is not on the
> restored frame and a window on the restored frame gets selected, the
> point of the buffer whose window is deselected is not stored in the old
> selected window's pointm which is certainly not TRT.

But I bet Drew's code works just fine, when it does not crash.

Anyway, if switching away of the frame inside save-window-excursion is
not allowed, we should detect that and signal an error.  That would
solve this bug cleanly, with no need for any low-level hacking or
kludges.

> To avoid the present crash we could try something like the attached
> patch (which does not try to solve anything but that crash).

But if you say that switching a frame inside save-window-excursion is
not supported, why do we need to change code to support it?  What am I
missing?

bug#7728: 24.0.50; GDB backtrace from abort

[martin rudalics]

 >> I doubt it's kosher because if the old selected window is not on the
 >> restored frame and a window on the restored frame gets selected, the
 >> point of the buffer whose window is deselected is not stored in the old
 >> selected window's pointm which is certainly not TRT.
 >
 > But I bet Drew's code works just fine, when it does not crash.

I doubt the scenario sketched above shows up frequently.  And when it
show up I doubt many people would notice it.

 > Anyway, if switching away of the frame inside save-window-excursion is
 > not allowed, we should detect that and signal an error.

It _is_ allowed by design.

 > That would
 > solve this bug cleanly, with no need for any low-level hacking or
 > kludges.

Have you looked into the ChangeLog of this?  It was never clean and
people just managed to make it work, more or less.

As I noticed earlier, one bug is in the design of using two variables
that should never change independently from each other.  Better, there
should be only one variable - either selected_window or selected_frame.
After that has been solved we would have to fix the bug where the
selected window is not the selected window because it's on a frame that
has not been risen.  And, finally we should warn people to avoid things
like `save-window-excursion' in the first place.

 >> To avoid the present crash we could try something like the attached
 >> patch (which does not try to solve anything but that crash).
 >
 > But if you say that switching a frame inside save-window-excursion is
 > not supported, why do we need to change code to support it?  What am I
 > missing?

As I said it is allowed.  But almost all 100 or so occurrences of
`save-window-excursion' in Emacs are harmful.  Take, for example, the
commented-out sections of buff-menu.el.  Richard took them out
apparently because he used that code in practice and noticed that it
didn't DTRT.  Most other instances are still here because people don't
care enough.

martin

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> But I bet Drew's code works just fine, when it does not crash.

It does.

And as I stated, it also works fine if I load the source file `fit-frame.el'
instead of the byte-compiled file `fit-frame.elc' (regardless of whether it is
compiled using Emacs 20 or the latest version).

It seems that in the latest release only, something in the byte-compilation
(from this release and from older releases) causes the crash.

> Anyway, if switching away of the frame inside save-window-excursion is
> not allowed, we should detect that and signal an error.

Please do not even think of such a thing.  AFAIK `save-window-excursion' has
always let you change frames.

(I've always thought of it more or less as `save-windows-and-frames-excursion'
(but I don't claim that is appropriate).)

> That would solve this bug cleanly, with no need for any
> low-level hacking or kludges.
> 
> > To avoid the present crash we could try something like the attached
> > patch (which does not try to solve anything but that crash).
> 
> But if you say that switching a frame inside save-window-excursion is
> not supported, why do we need to change code to support it?  What am I
> missing?

I don't agree that switching a frame inside a `save-window-excursion' is a
no-no.

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> But I see that fit-frame uses save-window-excursion.  So it will call
> set-window-configuration after the form inside save-window-excursion
> is evaluated.
> 
> Can you see what window configuration is saved and restored there, and
> post that configuration here?

When I get some time I'll try to take a look.

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> But I see that fit-frame uses save-window-excursion.  So it will call
> set-window-configuration after the form inside save-window-excursion
> is evaluated.
> 
> Can you see what window configuration is saved and restored there, and
> post that configuration here?

I loaded the source files fit-frame.el and oneonone.el and stepped through
`1on1-fit-minibuffer-frame'.

(Note: It does not crash when the source files are loaded, but you just wanted
to know what windows are involved etc.  The source-file debugging might not tell
us what the problem is with the byte-compiled file, however.  I could try
debugging with the latter, but typically the debugger doesn't show you much
then.  I could add calls to `message' instead of using the debugger, if you wish
- let me know.)

From the start the selected window and selected frame are already the minibuffer
window and minibuffer frame.  The `save-window-excursion' just moves to select
that window and frame (already selected), so the destination and the origin are
the same (`save-selected-window-window' is also the destination).  In this
context, the `save-window-excursion' should amount to a no-op.

I do not see any explicit window configs in the debugger - I see only window and
frame selection.  `save-window-excursion' just seems to save the selected window
and list of frames, and then restore them.

HTH.

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

Just wondering - did I mention this?  I hit this bug all the time in the
development version, but I never saw it in 23.2 or before that.  HTH.

bug#7728: 24.0.50; GDB backtrace from abort

[Stefan Monnier]

>> Shouldn't (doesn't?) set-window-configuration simply signal an error if
>> the selected-frame is different from the frame from which the
>> window-config comes?

> How could it?  For example, Drew's code that causes this crash does this:

>     (save-window-excursion (select-frame frame)
>                            (or (eq major-mode 'image-mode)
>                                image-minor-mode))

I don't think my proposition necessarily prevents this.  It just means that
save-window-excursion should re-select the original frame before doing
the set-window-configuration.

> Are you saying tha this isn't kosher, because the body of
> save-window-excursion is not allowed to select a different frame?

Admittedly, I think the above code is mistaken.  It kind of reminds me
of the infamous (save-excursion (set-buffer) ...), although in worse
since, like Martin, I despise save-window-excursion.


        Stefan

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> > How could it?  For example, Drew's code that causes this 
> > crash does this:
> >     (save-window-excursion (select-frame frame)
> >                            (or (eq major-mode 'image-mode)
> >                                image-minor-mode))

FWIW, that is not the part of the code that gets executed when there is a crash.
However, the branch of the `if' that is executed when there is a crash also has
a similar `save-window-excursion':

(save-window-excursion
  (select-frame frame)
  (setq empty-buf-p  (and (= (point-min) (point-max))
                          (one-window-p (selected-window)))
        specbuf-p    (and empty-buf-p
                          (special-display-p
                            (buffer-name (window-buffer))))))

> > Are you saying tha this isn't kosher, because the body of
> > save-window-excursion is not allowed to select a different frame?
> 
> Admittedly, I think the above code is mistaken.  It kind of reminds me
> of the infamous (save-excursion (set-buffer) ...), although in worse
> since, like Martin, I despise save-window-excursion.

And just what code do you suggest for going off to do something on a different
frame and returning?  AFAIK we do not have a `save-frame-excursion'.  I have
always considered (and seen) `save-window-excursion' to be the way to do this.

bug#7728: 24.0.50; GDB backtrace from abort

[Stefan Monnier]

>> Admittedly, I think the above code is mistaken.  It kind of reminds me
>> of the infamous (save-excursion (set-buffer) ...), although in worse
>> since, like Martin, I despise save-window-excursion.

> And just what code do you suggest for going off to do something on
> a different frame and returning?  AFAIK we do not have
> a `save-frame-excursion'.

Hmm... let's see... how 'bout `with-selected-frame'?  ;-)

> I have always considered (and seen) `save-window-excursion' to be the
> way to do this.

No, as Martin mentioned, save-window-excursion is the macro to use if
you want to write code that breaks in "odd" cases (sidenote:
one-buffer-per-frame is one of those "odd" cases that tend to trigger
bugs in code using save-window-excursion, so you probably "often" suffer
from those things, like me).


        Stefan

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> > And just what code do you suggest for going off to do something on
> > a different frame and returning?  AFAIK we do not have
> > a `save-frame-excursion'.
> 
> Hmm... let's see... how 'bout `with-selected-frame'?  ;-)

Does not exist before Emacs 23, for one thing.  My code needs to work with
multiple Emacs versions.  And please do not suggest that I add such macros to
older versions just to be able to work around a newly introduced Emacs bug.  And
please do not suggest that I split the code to use the macro only for Emacs
23.3+ since this is a new bug (regression).

> > I have always considered (and seen) `save-window-excursion' 
> > to be the way to do this.
> 
> No, as Martin mentioned, save-window-excursion is the macro to use if
> you want to write code that breaks in "odd" cases (sidenote:
> one-buffer-per-frame is one of those "odd" cases that tend to trigger
> bugs in code using save-window-excursion, so you probably 
> "often" suffer from those things, like me).

Sorry, but I have never, ever suffered from "those things".  And
`save-window-excursion' _has_ always been used for this kind of thing in Emacs
AFAIK - revisionism notwithstanding.

In the case in question, the selected window and selected frame happen to be the
_same_ as the destination window and frame (they are the minibuffer window and
standalone frame).

In this case the `save-window-excursion' should amount to a no-op in the end.
The source and target window and frame need not be the same in general, but they
are the same in the crashes I reported.  If Emacs cannot save and restore
without crashing in this case then Houston you really have a problem.

Look - you know that you lost your car keys in the park, but because it's
nighttime you decide to look for them in the living room.  That's silly.

* Let me repeat that the _source code works fine_ - no error, no crash, no bug.

* Let me repeat too that the byte-compiled code (no matter which Emacs version
it was compiled with) works fine in all Emacs versions except the current
development code - no error, no crash, no bug.

This is not a source-code problem.  It is not a byte-code problem.

This is a _regression_ due to some change in the development version that no
longer plays well with the byte-compiled code.  Please investigate that
interaction: dev version + byte code.  Your keys are in the park.

Advice to change my source code so as to not use `save-window-excursion' is
misplaced wrt this bug.  It amounts to blaming the victim.  You are looking
under the lamp in the living room, but your keys are lying in the park
somewhere.

This is a problem with the byte code only - that is, the problem is with how the
byte-code is handled by the latest dev code.  No problem with Emacs 23.2 or even
with dev code prior to when I filed the bug (exact date of regression unknown).

You lost your keys in the park.  Please do not ask me to move the couch in the
living room.  Whether my code could or should be improved in some way is
orthogonal to fixing this Emacs bug.

This is a regression.  Emacs should handle the byte-code correctly, as it has
always done before - and as it still correctly interprets the source code.

bug#7728: 24.0.50; GDB backtrace from abort

[Stefan Monnier]

>> > And just what code do you suggest for going off to do something on
>> > a different frame and returning?  AFAIK we do not have
>> > a `save-frame-excursion'.
>> Hmm... let's see... how 'bout `with-selected-frame'?  ;-)

> Does not exist before Emacs 23, for one thing.  My code needs to work
> with multiple Emacs versions.  And please do not suggest that I add
> such macros to older versions just to be able to work around a newly
> introduced Emacs bug.  And please do not suggest that I split the code
> to use the macro only for Emacs 23.3+ since this is a new bug
> (regression).

I'm just telling you what's the right way to do it.

> Sorry, but I have never, ever suffered from "those things".

Then either you were lucky to only use the code after I fixed it, or you
don't know what I'm talking about.  The typical misuse looks like:

  (save-window-excursion
    (let ((b (find-file "foo")))
      blabla))

instead of

  (let ((b (find-file-noselect "foo")))
    blabla)

I.e. call code that may modify the window-layout whereas what the caller
wants is something else, so he uses save-window-excursion to "undo"
those changes.  But of course, with pop-up-frames and friends, in
many/most cases the code may not only modify the window-layout but also
create a new frame, which can't really be undone because the creation
itself is already user-visible, and save-window-excursion won't even try
to undo it anyway.
    
> And `save-window-excursion' _has_ always been used for this kind of
> thing in Emacs AFAIK - revisionism notwithstanding.

I didn't know that.  Can you point at some examples?

> In this case the `save-window-excursion' should amount to a no-op in
> the end.  The source and target window and frame need not be the same
> in general, but they are the same in the crashes I reported.  If Emacs
> cannot save and restore without crashing in this case then Houston you
> really have a problem.

You know I always consider any crash as a bug in the C code, even if
it's triggered by Elisp code.


        Stefan

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> I'm just telling you what's the right way to do it.

I appreciate that.  But it's not related to what's needed to fix this bug.
That's all I was trying to say.

> > Sorry, but I have never, ever suffered from "those things".
> 
> Then either you were lucky to only use the code after I fixed 
> it, or you don't know what I'm talking about.  The typical
> misuse looks like:
> (save-window-excursion (let ((b (find-file "foo")))  blabla))
> instead of (let ((b (find-file-noselect "foo")))  blabla)
> 
> I.e. call code that may modify the window-layout whereas what 
> the caller wants is something else, so he uses
> save-window-excursion to "undo" those changes.  But of course,
> with pop-up-frames and friends, in many/most cases the code may
> not only modify the window-layout but also create a new frame,
> which can't really be undone because the creation
> itself is already user-visible, and save-window-excursion 
> won't even try to undo it anyway.

It's good to have that pointed out, of course.  But no, I don't believe I've
used `s-w-e' like that.  I tend to use `find-file-noselect' much more than
`find-file' anyway.  I call `find-file' typically only from `find-file' wrapper
commands and such.  But I get your point.

I expect that both you and I have come across places now and then where we
needed to fix code because of interactions with frames.  I don't think it's
common that such source bugs stay hidden from us long, precisely because we use
frames more than most people do.

Problems such as you point out are, I imagine, more common with developers who
test only in an environment where frames are _not_ used very much.  It makes
sense to draw special attention to such gotchas in that case.
  
> > And `save-window-excursion' _has_ always been used for this kind of
> > thing in Emacs AFAIK - revisionism notwithstanding.
> 
> I didn't know that.  Can you point at some examples?

Nope, sorry.  But you can imagine, I think, that before `with-selected-frame'
existed `s-w-e' was used for similar use cases. ;-)

However, grepping for `with-selected-frame' in the Emacs sources shows very few
hits, and all of them correspond to code (e.g. function defs) that was
inexistent in older releases (e.g. Emacs 20).  So no, I don't see an example
offhand where code that used `s-w-e' might have been "upgraded" to code that
uses `with-selected-frame'.

> You know I always consider any crash as a bug in the C code, even if
> it's triggered by Elisp code.

Yes, I hope (and assume) so.  The question is whether the C fix will still allow
the same behavior as previously.  I really hope so.

So let's please just assume here that (a) the source code I mentioned is not
doing something ill-advised, verboten, off-the-wall, or silly wrt this bug, and
(b) it doesn't need to be changed for things to work properly, but rather (c)
the C code should be fixed so that such source code and its byte-compilation
continues to work as before.

I will help to diagnose things at my end if I can be of assistance, but let's
please not concentrate on having the user source code be changed to effect a
workaround.

That `save-window-excursion', like other things in Emacs Lisp, can be misused or
might otherwise not DTRT in some cases is not the question here.

For my use case it DTRT, and has been doing it, for this code, for years.  This
code needs to work not just for my own setup (with a standalone minibuffer etc.
- the case for the reported crash), but more importantly for other setups,
including more common setups.  It works well, I believe, so let's not worry here
and now about improving it.  That's all.

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: "Drew Adams" <drew.adams@oracle.com>
> Cc: "'Eli Zaretskii'" <eliz@gnu.org>, <7728@debbugs.gnu.org>
> Date: Thu, 13 Jan 2011 09:57:11 -0800
> 
> In this case the `save-window-excursion' should amount to a no-op in the end.
> The source and target window and frame need not be the same in general, but they
> are the same in the crashes I reported.

I don't believe this to be true, at least not from Emacs's internals
POV.  The code that crashes clearly executes the branch where the
frame recorded by save-window-excursion is NOT the selected frame by
the time the body of save-window-excursion is done being evaluated.

> * Let me repeat that the _source code works fine_ - no error, no crash, no bug.
> 
> * Let me repeat too that the byte-compiled code (no matter which Emacs version
> it was compiled with) works fine in all Emacs versions except the current
> development code - no error, no crash, no bug.

I don't think this to be relevant, sorry.  I'm inclined to think that
it's some weird side effect of Edebug, or maybe something else.  The
C backtrace is very clear and there's no ambiguity whatsoever in
interpreting it: when save-window-excursion involves switching frames,
the restoration of the original window configuration runs a high risk
to hit this bug, especially when many standard buffers (minibuffer,
*Help*, "Completions*", etc.) use separate frames.

> This is a _regression_ due to some change in the development version that no
> longer plays well with the byte-compiled code.

That's a possibility, but I think it's a remote one.  The offending
code has been in Emacs since v21.1, so the problem is not new in any
way.  It could be that it got exposed in the current development code
due to some other unrelated change.  It could also be that this is the
first time you are running a binary that was built with
ENABLE_CHECKING defined: without that, the abort won't happen at all.

I think you interpret the latest messages incorrectly.  No one is
arguing that your code is the culprit.  The correct way to fix this
bug was pointed out by Stefan several messages ago, and I will do just
that when I have time.  The latest discussions were generally about
the wisdom of switching frames inside save-window-excursion, and
whether we should do something about its problematic semantics, that's
all.

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> > In this case the `save-window-excursion' should amount to a 
> > no-op in the end. The source and target window and frame need
> > not be the same in general, but they are the same in the
> > crashes I reported.
> 
> I don't believe this to be true, at least not from Emacs's internals
> POV.  The code that crashes clearly executes the branch where the
> frame recorded by save-window-excursion is NOT the selected frame by
> the time the body of save-window-excursion is done being evaluated.

As I said, I followed the _source_ code in the debugger.  And the source code
does not cause a crash.  The source code lets us know what _should_ be happening
here, not what is actually happening that provokes a crash.

I asked if you wanted me to instead add some calls to `message' and then
byte-compile.  Let me know if there is some such test you would like me to try.
You asked that I try the debugger to see what window was selected etc., so I did
that (with the source code). 

> > * Let me repeat that the _source code works fine_ - no 
> > error, no crash, no bug.
> > 
> > * Let me repeat too that the byte-compiled code (no matter 
> > which Emacs version it was compiled with) works fine in all
> > Emacs versions except the current development code - no error,
> > no crash, no bug.
> 
> I don't think this to be relevant, sorry.

Why?  The only thing new to the mix is the new Emacs dev version.  The source
code and the byte-compiled code are the same as before.  The regression is not
realized using the source code.  It happens only with the new dev version when
it executes the byte code.  Why isn't that relevant?

> I'm inclined to think that it's some weird side effect of
> Edebug, or maybe something else.

You think _what_ is a weird effect of edebug?  (I used `debug-on-entry', not
edebug, BTW.)  With the debugger there was no crash.  So it certainly cannot be
some weird effect of the debugger that is causing the crash.

> The C backtrace is very clear and there's no ambiguity whatsoever in
> interpreting it: when save-window-excursion involves switching frames,
> the restoration of the original window configuration runs a high risk
> to hit this bug, especially when many standard buffers (minibuffer,
> *Help*, "Completions*", etc.) use separate frames.

The question is why the current Emacs dev version crashes with the byte-compiled
code.  Nothing more.  It's not about some risk from the source code.  The code
works fine in all prior Emacs versions - both the byte code and the source code.
And the source code works fine even in this dev version.

> > This is a _regression_ due to some change in the development
> > version that no longer plays well with the byte-compiled code.
> 
> That's a possibility, but I think it's a remote one.

Seems more like an inescapable conclusion, to me.  Substitute any other Emcs
version and presto: no problem.  Substitute the source code for the byte code
and presto: no problem.

> The offending code

What offending code?

What you see as offending code, if it was already in 21.1, did not present a
problem - it wasn't offending anyone.

> has been in Emacs since v21.1, so the problem is not new in any way.

Of course the problem is new.  It's a _regression_.  There is no such crash in
any prior Emacs version.  You and I have different views of what "the problem"
is, I guess.  For me, the problem is the crash.  That's new.

> It could be that it got exposed in the current development code
> due to some other unrelated change.

Unrelated or related - we don't know yet.  Either way, the problem (crash)
happens only with the new Emacs and the (new or old) byte code.

> It could also be that this is the first time you are running
> a binary that was built with ENABLE_CHECKING defined: without
> that, the abort won't happen at all.

I know nothing about that, so I'm sure you're right about that possibility.  I
really have nothing to say about what the fix should be.  If there was always a
latent bug and it never manifested itself before because of a lack of
ENABLE_CHECKING, then the final fix needs to be at least as good as the effect
of turning off ENABLE_CHECKING. ;-)

> I think you interpret the latest messages incorrectly.  No one is
> arguing that your code is the culprit.  The correct way to fix this
> bug was pointed out by Stefan several messages ago, and I will do just
> that when I have time.

I did not understand that you have a solution.  I didn't get that impression
from your asking me to check the selected window in the debugger etc.  If you
have a solution, fantastic; that's good news.  I'm not trying to hurry you at
all.

> The latest discussions were generally about
> the wisdom of switching frames inside save-window-excursion, and
> whether we should do something about its problematic semantics, that's
> all.

In that case, we can have that discussion outside this bug thread, since you
recognize that it is separate and you apparently already have the solution for
the regression.

This is good news.  Thx.

bug#7728: 24.0.50; GDB backtrace from abort

[Stefan Monnier]

>> Hmm... let's see... how 'bout `with-selected-frame'?  ;-)
> Does not exist before Emacs 23, for one thing.  My code needs to work with
> multiple Emacs versions.

If you can live with Emacs≥22, then you can try (with-selected-window
(frame-selected-window <frame>) ...).


        Stefan

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: "Drew Adams" <drew.adams@oracle.com>
> Cc: <monnier@iro.umontreal.ca>, <7728@debbugs.gnu.org>
> Date: Thu, 13 Jan 2011 17:19:43 -0800
> 
> > > In this case the `save-window-excursion' should amount to a 
> > > no-op in the end. The source and target window and frame need
> > > not be the same in general, but they are the same in the
> > > crashes I reported.
> > 
> > I don't believe this to be true, at least not from Emacs's internals
> > POV.  The code that crashes clearly executes the branch where the
> > frame recorded by save-window-excursion is NOT the selected frame by
> > the time the body of save-window-excursion is done being evaluated.
> 
> As I said, I followed the _source_ code in the debugger.  And the source code
> does not cause a crash.  The source code lets us know what _should_ be happening
> here, not what is actually happening that provokes a crash.

Since you couldn't reproduce the crash under the Lisp debugger, the
evidence you collected during that debugging session is not really
admissible in the court of Emacs bugs ;-)

IOW, the backtrace you posted clearly shows that somehow,
save-window-excursion needed to switch frames, and its code that
restores the original window configuration therefore needed to select
a different frame.  That is a fact revealed by the C backtrace.  If we
want to make sure that this frame switch is real, I would suggest to
look at the values of sf and w->frame in this fragment from
select-window:

  sf = SELECTED_FRAME ();
  if (XFRAME (WINDOW_FRAME (w)) != sf)
    {
      XFRAME (WINDOW_FRAME (w))->selected_window = window;
      /* Use this rather than Fhandle_switch_frame
	 so that FRAME_FOCUS_FRAME is moved appropriately as we
	 move around in the state where a minibuffer in a separate
	 frame is active.  */
      Fselect_frame (WINDOW_FRAME (w), norecord);

For that, you need to reproduce the crash, then go to the call-stack
frame where select-window (Fselect_window) invokes select-frame
(Fselect_frame).  In your original backtrace, this was frame #15:

 #12 0x01288ef3 in Fredirect_frame_focus (frame=93005829, focus_frame=93005829)
     at frame.c:2082
 #13 0x0127f4c8 in do_switch_frame (frame=93005829, track=1, for_deletion=0,
     norecord=49010714) at frame.c:847
 #14 0x01280733 in Fselect_frame (frame=93005829, norecord=49010714) at
 frame.c:899
 #15 0x01252702 in Fselect_window (window=93006853, norecord=49010714) at
 window.c:3581
 #16 0x0125e7c8 in Fset_window_configuration (configuration=99327941) at
 window.c:6148

So in that case, you would need to issue the following GDB commands:

 (gdb) frame 15
 (gdb) p sf->name
 (gdb) xstring
 (gdb) p w->frame
 (gdb) xframe

The 3rd and the 5th command will display the names of the two frames,
the one that's selected at this point, and the one to which the window
w (from the configuration being restored) belongs, respectively.  We
could then try to understand how come Emacs thinks it needs to switch
frames, while your analysis of the Lisp code suggests these two should
have specified the same frame.

(Note that frame #15 could have a different number in a different
crash, so look for the frame whose description is the same as what is
shown about, i.e. a call from Fselect_window to Fselect_frame, and use
the number of that frame.)

> > > * Let me repeat that the _source code works fine_ - no 
> > > error, no crash, no bug.
> > > 
> > > * Let me repeat too that the byte-compiled code (no matter 
> > > which Emacs version it was compiled with) works fine in all
> > > Emacs versions except the current development code - no error,
> > > no crash, no bug.
> > 
> > I don't think this to be relevant, sorry.
> 
> Why?  The only thing new to the mix is the new Emacs dev version.  The source
> code and the byte-compiled code are the same as before.  The regression is not
> realized using the source code.  It happens only with the new dev version when
> it executes the byte code.  Why isn't that relevant?

Because we have the C backtrace (thanks to you).  And that backtrace
speaks for itself.  There's nothing in it that cannot be understood
without invoking some non-trivial bug in the compiled byte code.  So,
while it's certainly possible that byte compilation has some unwanted
effect here, it sounds extremely unlikely, certainly not the first
explanation we should try.

> > I'm inclined to think that it's some weird side effect of
> > Edebug, or maybe something else.
> 
> You think _what_ is a weird effect of edebug?

The fact that uncompiled code seems to avoid the crash.

> With the debugger there was no crash.  So it certainly cannot be
> some weird effect of the debugger that is causing the crash.

The way I see it, you had a crash with byte-compiled code without the
debugger, and you had no crash with uncompiled code under the
debugger.  Which of these two variables caused the difference in
behavior remains to be seen.

> > > This is a _regression_ due to some change in the development
> > > version that no longer plays well with the byte-compiled code.
> > 
> > That's a possibility, but I think it's a remote one.
> 
> Seems more like an inescapable conclusion, to me.  Substitute any other Emcs
> version and presto: no problem.  Substitute the source code for the byte code
> and presto: no problem.

To really convince me in this, you would have to run Emacs under GDB,
using the source Lisp code, step through all the functions involved in
the crash, and show that the crash is indeed avoided, and why.

If you give me a reproducible recipe for the crash, I might try doing
this myself.

> > The offending code
> 
> What offending code?

The one that sets to nil the internal variable which holds the
selected window.  That's what triggers the crash, because way down the
call-stack, Emacs tries to reference the mode-line face of the frame
held in that variable.

> What you see as offending code, if it was already in 21.1, did not present a
> problem - it wasn't offending anyone.

Ever heard of bugs that lurk and rear their ugly head years after they
were introduced?

> > has been in Emacs since v21.1, so the problem is not new in any way.
> 
> Of course the problem is new.  It's a _regression_.

Only if the issue is looked at phenomenologically.  From my POV, this
bug was there for years.

> There is no such crash in any prior Emacs version.

But you have never before used any Emacs binary compiled with
ENABLE_CHECKING, did you?  Only such a version will crash, because it
does extra checking.

> You and I have different views of what "the problem"
> is, I guess.  For me, the problem is the crash.  That's new.

We can never fix the crash unless we understand what code causes it,
and why.  I posted here many messages ago why it crashes, and what
I found does not need to invoke any mysterious changes introduced
by the byte compiler to explain the crash.  It is crystal clear that,
under specific and well-defined circumstances set-window-configuration
and any code that calls it, including save-window-excursion, can crash
in the same way, if the window configuration being restored was
recorded in a different frame.  _That_ is the problem I'm trying to
fix in this bug.

While the crash in your specific use-case could indeed be new (if it
is explained by something other than the fact you are for the first
time using a binary compiled with ENABLE_CHECKING), the defect in the
code that I found and described could cause crashes in any number of
other use-cases, which have nothing to do with byte-compiling.  I'm
trying to find a solution for all those use-cases, not just for yours.

> > I think you interpret the latest messages incorrectly.  No one is
> > arguing that your code is the culprit.  The correct way to fix this
> > bug was pointed out by Stefan several messages ago, and I will do just
> > that when I have time.
> 
> I did not understand that you have a solution.  I didn't get that impression
> from your asking me to check the selected window in the debugger etc.

I asked that to have more evidence to back up my analysis.  It's never
a bad idea to look for more evidence, because sometimes it can
contradict the best hypothesis and change the whole picture.

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> >> Hmm... let's see... how 'bout `with-selected-frame'?  ;-)
> >
> > Does not exist before Emacs 23, for one thing.  My code 
> > needs to work with multiple Emacs versions.
> 
> If you can live with Emacs=22, then you can try (with-selected-window
> (frame-selected-window <frame>) ...).

That too is good to know.
But most of my code works for Emacs 20+.

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> > > > In this case the `save-window-excursion' should amount to a 
> > > > no-op in the end. The source and target window and frame need
> > > > not be the same in general, but they are the same in the
> > > > crashes I reported.
> > > 
> > > I don't believe this to be true, at least not from 
> > > Emacs's internals POV.  The code that crashes clearly
> > > executes the branch where the frame recorded by
> > > save-window-excursion is NOT the selected frame by
> > > the time the body of save-window-excursion is done being 
> > > evaluated.
> > 
> > As I said, I followed the _source_ code in the debugger.  
> > And the source code does not cause a crash.  The source code
> > lets us know what _should_ be happening here, not what is
> > actually happening that provokes a crash.
> 
> Since you couldn't reproduce the crash under the Lisp debugger, the
> evidence you collected during that debugging session is not really
> admissible in the court of Emacs bugs ;-)

I did not _try_ to reproduce the crash under the Lisp debugger.  [But see very
end of this message.]

I'm not trying or being tried in a court case.  I tried ;-) only to find the
context (windows, frame) of the correct behavior in this case, to let you know
which window should have been selected etc.  I had already sent you the info
about which window was _actually_ selected in the crash - as you requested (`p
selected_window' etc.).

> IOW, the backtrace you posted clearly shows that somehow,
> save-window-excursion needed to switch frames, and its code that
> restores the original window configuration therefore needed to select
> a different frame.  That is a fact revealed by the C backtrace.  If we
> want to make sure that this frame switch is real, I would suggest to
> look at the values of sf and w->frame in this fragment from
> select-window:
> 
>   sf = SELECTED_FRAME ();
>   if (XFRAME (WINDOW_FRAME (w)) != sf)
>     {
>       XFRAME (WINDOW_FRAME (w))->selected_window = window;
>       /* Use this rather than Fhandle_switch_frame
> 	 so that FRAME_FOCUS_FRAME is moved appropriately as we
> 	 move around in the state where a minibuffer in a separate
> 	 frame is active.  */
>       Fselect_frame (WINDOW_FRAME (w), norecord);
> 
> For that, you need to reproduce the crash, then go to the call-stack
> frame where select-window (Fselect_window) invokes select-frame
> (Fselect_frame).  In your original backtrace, this was frame #15:
> 
>  #12 0x01288ef3 in Fredirect_frame_focus (frame=93005829, 
> focus_frame=93005829)
>      at frame.c:2082
>  #13 0x0127f4c8 in do_switch_frame (frame=93005829, track=1, 
> for_deletion=0,
>      norecord=49010714) at frame.c:847
>  #14 0x01280733 in Fselect_frame (frame=93005829, 
> norecord=49010714) at
>  frame.c:899
>  #15 0x01252702 in Fselect_window (window=93006853, 
> norecord=49010714) at
>  window.c:3581
>  #16 0x0125e7c8 in Fset_window_configuration 
> (configuration=99327941) at
>  window.c:6148
> 
> So in that case, you would need to issue the following GDB commands:
> 
>  (gdb) frame 15
>  (gdb) p sf->name
>  (gdb) xstring
>  (gdb) p w->frame
>  (gdb) xframe

Here you are:

$ ./gdb -p 448
GNU gdb (GDB) 7.2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "mingw32".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Attaching to process 448
[New Thread 448.0xde8]
[New Thread 448.0x784]
[New Thread 448.0x650]
[New Thread 448.0x1f8]
Reading symbols from C:\Emacs-24-2011-01-03\bin\emacs.exe...done.
[Switching to Thread 448.0x1f8]
Warning: c:\drews-lisp-20\bin/../lwlib: No such file or directory.
SIGINT is used by the debugger.
Are you sure you want to change it? (y or n) [answered Y; input not from
terminal]
Environment variable "DISPLAY" not defined.
TERM = cygwin
Breakpoint 1 at 0x1308a1c: file w32fns.c, line 7298.
Temporary breakpoint 2 at 0x11cf0c9: file sysdep.c, line 839.
(gdb) c
Continuing.

Program received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 448.0xde8]
0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
(gdb) bt
#0  0x7c90120f in ntdll!DbgUiConnectToDbg () from C:\WINDOWS\system32\ntdll.dll
#1  0x01308a53 in w32_abort () at w32fns.c:7312
#2  0x01049f03 in die (msg=0x1592fbc "assertion failed:
WINDOWP(selected_window)",
    file=0x1592e80 "xdisp.c", line=1156) at alloc.c:6129
#3  0x01162d06 in window_text_bottom_y (w=0x3beba00) at xdisp.c:1156
#4  0x011c0737 in erase_phys_cursor (w=0x3beba00) at xdisp.c:23805
#5  0x011c19fb in display_and_set_cursor (w=0x3beba00, on=1, hpos=2, vpos=6,
x=16, y=87)
    at xdisp.c:23939
#6  0x011c1b14 in update_window_cursor (w=0x3beba00, on=1) at xdisp.c:23976
#7  0x011c1cce in update_cursor_in_window_tree (w=0x3beba00, on_p=1) at
xdisp.c:23996
#8  0x011c1e18 in x_update_cursor (f=0x31aee00, on_p=1) at xdisp.c:24010
#9  0x012ea74b in frame_unhighlight (f=0x31aee00) at w32term.c:2750
#10 0x012eac1a in x_frame_rehighlight (dpyinfo=0x16c5928) at w32term.c:2899
#11 0x012eaa66 in w32_frame_rehighlight (frame=0x31aee00) at w32term.c:2873
#12 0x01288a2b in Fredirect_frame_focus (frame=52096517, focus_frame=58733573)
    at frame.c:2082
#13 0x0127f000 in do_switch_frame (frame=58733573, track=1, for_deletion=0,
    norecord=49006618) at frame.c:847
#14 0x0128026b in Fselect_frame (frame=58733573, norecord=49006618) at
frame.c:899
#15 0x01252132 in Fselect_window (window=59380741, norecord=49006618) at
window.c:3586
#16 0x0125e2f5 in Fset_window_configuration (configuration=84414149) at
window.c:6159
#17 0x0103c8aa in unbind_to (count=42, value=64910257) at eval.c:3325
#18 0x010d3ffa in read_minibuf (map=48995734, initial=66396897, prompt=55090145,
    backup_n=20, expflag=0, histvar=49192130, histpos=0, defalt=66396897,
    allow_props=1, inherit_input_method=0) at minibuf.c:805
#19 0x010d4807 in Fread_from_minibuffer (prompt=55090145,
initial_contents=84398958,
    keymap=48995734, sys_read=49006618, hist=49192130, default_value=66396897,
    inherit_input_method=49006618) at minibuf.c:998
#20 0x0103b09e in Ffuncall (nargs=8, args=0x83e920) at eval.c:2969
#21 0x010f32b0 in Fbyte_code (bytestr=59961409, vector=55568133, maxdepth=32)
    at bytecode.c:679
#22 0x0103bae2 in funcall_lambda (fun=50305061, nargs=7, arg_vector=0x83ebe4)
    at eval.c:3129
#23 0x0103b1f9 in Ffuncall (nargs=8, args=0x83ebe0) at eval.c:2992
#24 0x010f32b0 in Fbyte_code (bytestr=60324673, vector=60286981, maxdepth=32)
    at bytecode.c:679
#25 0x0103bae2 in funcall_lambda (fun=52589221, nargs=8, arg_vector=0x83eea4)
    at eval.c:3129
#26 0x0103b1f9 in Ffuncall (nargs=9, args=0x83eea0) at eval.c:2992
#27 0x010f32b0 in Fbyte_code (bytestr=60321889, vector=55379013, maxdepth=36)
    at bytecode.c:679
#28 0x01038de5 in Feval (form=59632374) at eval.c:2358
#29 0x01036494 in internal_catch (tag=60939554, func=0x10382ac <Feval>,
arg=59632374)
    at eval.c:1206
#30 0x010f3ead in Fbyte_code (bytestr=60322641, vector=55367685, maxdepth=40)
    at bytecode.c:854
#31 0x0103bae2 in funcall_lambda (fun=52589573, nargs=7, arg_vector=0x83f440)
    at eval.c:3129
#32 0x0103b508 in apply_lambda (fun=52589573, args=58014910, eval_flag=1) at
eval.c:3055
#33 0x01039269 in Feval (form=58014966) at eval.c:2397
#34 0x01034591 in Fsetq (args=58015062) at eval.c:496
#35 0x010387e8 in Feval (form=58015070) at eval.c:2299
#36 0x01034415 in Fprogn (args=57946062) at eval.c:397
#37 0x01035fd9 in Flet (args=58015078) at eval.c:1054
#38 0x010387e8 in Feval (form=58015214) at eval.c:2299
#39 0x010f69da in Fcall_interactively (function=49554322, record_flag=49006618,
    keys=49027845) at callint.c:345
#40 0x0103adae in Ffuncall (nargs=4, args=0x83fb60) at eval.c:2950
#41 0x0103a228 in call3 (fn=49175466, arg1=49554322, arg2=49006618,
arg3=49006618)
    at eval.c:2775
#42 0x010227ad in Fcommand_execute (cmd=49554322, record_flag=49006618,
keys=49006618,
    special=49006618) at keyboard.c:10393
#43 0x01006bcd in command_loop_1 () at keyboard.c:1726
#44 0x01036a15 in internal_condition_case (bfun=0x100562b <command_loop_1>,
    handlers=49060250, hfun=0x1004d5f <cmd_error>) at eval.c:1462
#45 0x01005231 in command_loop_2 (ignore=49006618) at keyboard.c:1327
#46 0x01036494 in internal_catch (tag=49058346, func=0x100520d <command_loop_2>,
    arg=49006618) at eval.c:1206
#47 0x010051ed in command_loop () at keyboard.c:1306
#48 0x0100446b in recursive_edit_1 () at keyboard.c:929
#49 0x01004985 in Frecursive_edit () at keyboard.c:991
#50 0x010027d7 in main (argc=3, argv=0x33c58) at emacs.c:1716

Lisp Backtrace:
"old-read-from-minibuffer" (0x83e924)
"read-from-minibuffer" (0x83ebe4)
"icicle-lisp-vanilla-completing-read" (0x83eea4)
"byte-code" (0x83f0c4)
"completing-read" (0x83f440)
"setq" (0x83f69c)
"let" (0x83f85c)
"call-interactively" (0x83fb64)
(gdb) frame 15
#15 0x01252132 in Fselect_window (window=59380741, norecord=49006618) at
window.c:3586
3586    window.c: No such file or directory.
        in window.c
(gdb) p sf->name
$1 = 63915633
(gdb) xstring
$2 = (struct Lisp_String *) 0x3cf4670
"drews-lisp-20"
(gdb) p w->frame
$3 = 58733573
(gdb) xframe
$4 = (struct frame *) 0x3803400
"Emacs minibuffer", ' ' <repeats 25 times>, "show/hide: hold CTRL + click in
window"
(gdb)

> The 3rd and the 5th command will display the names of the two frames,
> the one that's selected at this point, and the one to which the window
> w (from the configuration being restored) belongs, respectively.  We
> could then try to understand how come Emacs thinks it needs to switch
> frames, while your analysis of the Lisp code suggests these two should
> have specified the same frame.
> 
> (Note that frame #15 could have a different number in a different
> crash, so look for the frame whose description is the same as what is
> shown about, i.e. a call from Fselect_window to Fselect_frame, and use
> the number of that frame.)

FYI - `drews-lisp-20' was a frame with a Dired listing.  It was the selected
frame when I hit `C-h f'.  `Emacs minibuffer ... click in window' is the name of
the standalone minibuffer frame.  The only other frame was the `*Completions*'
frame, which is where I clicked the completion candidate `display-graphic-p'
with `mouse-2' - that click provoked the crash.  The `*Completions*' frame has
its input redirected to the minibuffer frame (which it should).

HTH.

Let me know if I did what you need or you would like me to do something else.

Note too that when I spoke of the behavior (using the source code) in the
debugger, and I said that the selected window and frame were the same for the
save-window-excursion, I was following the execution of
`1on1-fit-minibuffer-frame' (which calls `fit-frame', where the
`save-window-excursion' occurs that you pointed to).

In the above gdb backtrace, I think we may be looking at something different.
That speaks of the frame that was selected before `C-h f' (`drews-lisp-20'),
i.e. before any call to `1on1-fit-minibuffer-frame', hence before the minibuffer
frame/window would have been selected, if it was at all when running the byte
code.

I mention this just so you make the difference.  The sequence of actions is:

selected frame is `d-l-20'

I hit `C-h f'

minibuffer becomes active

I type `g r a p h i c' and hit `S-TAB' to complete (and open frame
`*Completions*')

I click `mouse-2' on candidate `display-graphic-p' in `*Completions*'

crash

Where does `1on1-fit-minibuffer-frame' come in?  It is on `post-command-hook',
so it is run when the minibuffer is exited.  It is that that calls `fit-frame'
to (re-)fit the minibuffer frame by calling `fit-frame'.  It is you who pointed
to `fit-frame' and its `save-window-excursion', as being the likely place where
the problem arises.

HTH.

> > > > * Let me repeat that the _source code works fine_ - no 
> > > > error, no crash, no bug.
> > > > 
> > > > * Let me repeat too that the byte-compiled code (no matter 
> > > > which Emacs version it was compiled with) works fine in all
> > > > Emacs versions except the current development code - no error,
> > > > no crash, no bug.
> > > 
> > > I don't think this to be relevant, sorry.
> > 
> > Why?  The only thing new to the mix is the new Emacs dev 
> > version.  The source code and the byte-compiled code are the
> > same as before.  The regression is not realized using the
> > source code.  It happens only with the new dev version when
> > it executes the byte code.  Why isn't that relevant?
> 
> Because we have the C backtrace (thanks to you).  And that backtrace
> speaks for itself.  There's nothing in it that cannot be understood
> without invoking some non-trivial bug in the compiled byte code.  So,
> while it's certainly possible that byte compilation has some unwanted
> effect here, it sounds extremely unlikely, certainly not the first
> explanation we should try.
> 
> > > I'm inclined to think that it's some weird side effect of
> > > Edebug, or maybe something else.
> > 
> > You think _what_ is a weird effect of edebug?
> 
> The fact that uncompiled code seems to avoid the crash.

The uncompiled code avoids the crash both with and without the debugger.  No
weird effect of edebug (or `debug') could possibly be the reason that the
uncompiled code avoids the crash when the debugger is not even used.

> > With the debugger there was no crash.  So it certainly cannot be
> > some weird effect of the debugger that is causing the crash.
> 
> The way I see it, you had a crash with byte-compiled code without the
> debugger, and you had no crash with uncompiled code under the
> debugger.  Which of these two variables caused the difference in
> behavior remains to be seen.

I also have no crash (ever) with uncompiled code without the debugger.
Uncompiled code has no crash with or without the debugger.  So I don't see why
you are still asking yourself questions about variables.  The crash is
reproducible only with the byte-compiled code.  I did not try to reproduce the
crash with byte-compiled code while using the debugger.  I can do that if you
like.  I didn't do that because the debugger shows less info with byte-compiled
code and I was trying to find the windows etc. that should have been involved
(when there is no crash).

> > > > This is a _regression_ due to some change in the development
> > > > version that no longer plays well with the byte-compiled code.
> > > 
> > > That's a possibility, but I think it's a remote one.
> > 
> > Seems more like an inescapable conclusion, to me.  
> > Substitute any other Emcs version and presto: no problem.
> > Substitute the source code for the byte code and presto: no problem.
> 
> To really convince me in this, you would have to run Emacs under GDB,
> using the source Lisp code, step through all the functions involved in
> the crash, and show that the crash is indeed avoided, and why.
> 
> If you give me a reproducible recipe for the crash, I might try doing
> this myself.

The latest recipe I've been using uses all of my setup.  I have not tried to
pare things down.

But IIRC I gave you a minimal recipe the first time around, at the beginning of
this thread (or perhaps in mails between us before the thread).  You should be
able to use that recipe.  If you cannot find it and you still need to do this,
let me know and I'll try to find it.

> > > The offending code
> > 
> > What offending code?
> 
> The one that sets to nil the internal variable which holds the
> selected window.  That's what triggers the crash, because way down the
> call-stack, Emacs tries to reference the mode-line face of the frame
> held in that variable.
> 
> > What you see as offending code, if it was already in 21.1, 
> > did not present a problem - it wasn't offending anyone.
> 
> Ever heard of bugs that lurk and rear their ugly head years after they
> were introduced?
> 
> > > has been in Emacs since v21.1, so the problem is not new 
> > > in any way.
> > 
> > Of course the problem is new.  It's a _regression_.
> 
> Only if the issue is looked at phenomenologically.  From my POV, this
> bug was there for years.
> 
> > There is no such crash in any prior Emacs version.
> 
> But you have never before used any Emacs binary compiled with
> ENABLE_CHECKING, did you?  Only such a version will crash, because it
> does extra checking.
> 
> > You and I have different views of what "the problem"
> > is, I guess.  For me, the problem is the crash.  That's new.
> 
> We can never fix the crash unless we understand what code causes it,
> and why.  I posted here many messages ago why it crashes, and what
> I found does not need to invoke any mysterious changes introduced
> by the byte compiler to explain the crash.

I never suggested any changes, mysterious or otherwise, _introduced by the
byte-compiler_.  On the contrary, I've said several times that it does not
matter whether I byte-compile in Emacs 20 or 24.  Any changes in the
byte-compiler since Emacs 20 are irrelevant (unless there are two different
crash causes).

What I said was that the crash occurs only with the byte-compiled code (no
matter how it was compiled - which version) and only when using Emacs 24 (for
the test, not for byte-compiling).  My guess was that it is changes _introduced
in Emacs 24_ which are the problem, not changes in the byte-compiler.

But you are quite right that it might not be _development_ changes introduced in
Emacs 24 but just the simple change to using ENABLE_CHECKING that makes the
difference.  I wasn't aware of that difference.  That's still a difference in
the version, in the sense I was using: it is the Emacs binary that is different,
not the source or byte code.

> It is crystal clear that,
> under specific and well-defined circumstances set-window-configuration
> and any code that calls it, including save-window-excursion, can crash
> in the same way, if the window configuration being restored was
> recorded in a different frame.  _That_ is the problem I'm trying to
> fix in this bug.
> 
> While the crash in your specific use-case could indeed be new (if it
> is explained by something other than the fact you are for the first
> time using a binary compiled with ENABLE_CHECKING), the defect in the
> code that I found and described could cause crashes in any number of
> other use-cases, which have nothing to do with byte-compiling.  I'm
> trying to find a solution for all those use-cases, not just for yours.
> 
> > > I think you interpret the latest messages incorrectly.  No one is
> > > arguing that your code is the culprit.  The correct way 
> > > to fix this bug was pointed out by Stefan several messages ago, and I 
> > > will do just that when I have time.
> > 
> > I did not understand that you have a solution.  I didn't 
> > get that impression from your asking me to check the selected
> > window in the debugger etc.
> 
> I asked that to have more evidence to back up my analysis.  It's never
> a bad idea to look for more evidence, because sometimes it can
> contradict the best hypothesis and change the whole picture.

Let me know if you need something more from me.  I'm hoping that you will not
need me to try to pare down my setup to give you a simple recipe to reproduce.
But if necessary I'll try to find time to do that (but not now).

---

PostScript

Below, I tried to trace `1on1-fit-minibuffer-frame' in the debugger using the
byte-compiled version of `fit-frame.el'.  I just copied (or typed again) most of
the text as I hit `d' etc., so this is not just a copy of the debugger buffer.

First, there was no crash.  Second, I have no idea why that Lisp error was
raised at the end.  (I do have `mouse-choose-completion' defaliased, but to a
definition that is very similar to the original and whose interactive spec is
(interactive "e") - an event with params.

Also, if I load the source file that defines it, I am able to choose a
completion using mouse-2 without a crash and without raising the error.  (I've
never seen that error anyway, except this time with byte-compiled code in the
debugger.)

Now I'm thinking that these recent crashes might not be the same as the crash I
wrote about originally (though you said you think they are the same, so at some
level they must be).

I see now that if, starting with only byte-compiled code, I then load the source
file icicles-mcmd.el, then there is no crash.  That is the file that defines
`icicle-mouse-choose-completion', to which `mouse-choose-completion' is
defaliased (in Icicle mode).  I also traced that in the debugger after loading
that source file, and the event passed to `mouse-choose-completion' is a
legitimate mouse event (not nil nil as in the backtrace below).  Call me
clueless now.

Thinking that maybe the problem with these latest crashes was perhaps, after
all, that I was using an Emacs-20 byte-compiled icicles-mcmd.el, I byte-compiled
that file using Emacs 24 - and it still crashed (same way).  So for this file
too, the particular byte-compiler makes no difference.  So there are now two
ways to avoid the recent crash: load source file fit-frame.el or load source
file icicles-mcmd.el.  

I really don't understand (but then, I also don't understand the Lisp error
raised in the debugger (only) below).

Dunno if any of this makes sense anymore.  Sorry.

------------------

Debugger entered--Lisp error: (error "mouse-choose-completion must be bound to
an event with parameters")
call-interactively(mouse-choose-completion nil nil)
eldoc-pre-command-refresh-echo-area()
delete-selection-pre-hook()
1on1-box-cursor-when-idle-off()
erase-nonempty-inactive-minibuffer()
icicle-add-menu-item-to-cmd-history()
icicle-top-level-prep()
run-hooks(pre-command-hook)

[1on1-fit-minibuffer-frame()]
(normal-top-level)
1on1-set-minibuffer-frame-top/bottom()
set-frame-size(minibuf frame 160 2)
frame-width(minibuf frame) -> 160
1on1-reset-minibuffer-frame()
byte-code...
add-to-list(minor-mode-alist (icicle-mode " Icy"))
icicle-clear-lighter()
icicle-highlight-lighter()
get-buffer-window("*Help*" 0) -> nil
icicle-cancel-Help-redirection()
set-face-background(region "MediumAquamarine")
recursion-depth() -> 1
icicle-restore-region-face()
run-hooks(minibuffer-exit-hook)
run-hooks(activate-menubar-hook)
scroll-down(2)
frame-height(minibuf frame) -> 2
byte-code...
modify-frame-parameters(minibuf frame ((top . -28)))
frame-char-height -> 14
byte-code...
1on1-set-minibuffer-frame-top/bottom()
[fit-frame(minibuf frame) -> nil]
set-frame-size(minibuf frame 160 2)
[fit-frame-max-height -> 7]
fit-frame-thumbnail-factor(minibuf frame) -> 1
frame-char-height(minibuf frame) -> 14
x-display-pixel-height -> 1024
fit-frame-max-height(minibuf frame)
[fit-frame-max-frame-size -> (27 . 1)]
refresh-pretty-control-l()
fit-frame-fringe-width(minibuf frame) -> 0
-> (27 . 1)
frame-parameters(minibuf frame)
window-frame(window 6) -> the minibuf frame
window-buffer() -> minibuf-1
select-window(window 6)
fit-frame-max-window-size(window 6) -> (27 . 1)
selected-window() -> window 6
select-frame(minibuf frame)
byte-code...
fit-frame-max-frame-size(minibuf frame)
refresh-pretty-control-l()
next-window(window 6) -> window 6
selected-window() -> window 6 on minibuf
one-window-p(nil selected-frame)
select-frame(minibuf frame)
byte-code...
frame-parameters(minibuffer frame)
(force-mode-line-update)
pretty-control-l-mode(t)
refresh-pretty-control-l()
select-frame(the minibuf frame)
byte-code...
fboundp(image-mode-fit-frame) -> nil
fit-frame(the minibuffer frame)
frame-width(the minibuf frame) -> 160
minibuffer-prompt-end() -> 20
frame-height(the minibuffer frame) -> 2
(select-window save-selected-window-window) -> window 6
selected-frame() -> the minibuffer frame
select-window(window 6)
minibuffer-window -> window 6
selected-window -> window 6
(select-window save-selected-window-window) -> window 6
next-window(window 6) -> window 6
select-window(window 6)
one-window-p(nil selected-frame)
select-window(window 6)
minibuffer-window() -> window 6 on *Minibuf-1*
selected-window() -> window 6 on *Minibuf-1*
active-minibuffer-window() -> window 6 on *Minibuf-1*
1on1-fit-minibuffer-frame()

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> Also, if I load the source file that defines it, I am able to choose a
> completion using mouse-2 without a crash and without raising 
> the error....
> 
> I see now that if, starting with only byte-compiled code, I 
> then load the source file icicles-mcmd.el, then there is no crash.
> That is the file that defines `icicle-mouse-choose-completion',
> to which `mouse-choose-completion' is defaliased (in Icicle mode).

Nope, that is not really true.  It seems to depend on which frame is selected
when I use C-h f, although even that is not clear.  Sometimes the mouse-2 click
in *Completion* provokes a crash; sometimes it does not.  So maybe this is the
same problem after all, and has nothing to do with `mouse-choose-completion'.

> I also traced that in the debugger after loading
> that source file, and the event passed to `mouse-choose-completion'
> is a legitimate mouse event (not nil nil as in the backtrace 
> below).  Call me clueless now.

bug#7728: 24.0.50; GDB backtrace from abort

[martin rudalics]

 > No, as Martin mentioned, save-window-excursion is the macro to use if
 > you want to write code that breaks in "odd" cases (sidenote:
 > one-buffer-per-frame is one of those "odd" cases that tend to trigger
 > bugs in code using save-window-excursion, so you probably "often" suffer
 > from those things, like me).

`save-window-excursion' is the macro never to use.  It should be used
exclusively for debugging and prototyping.  Application programmers who
prefer to work with multiple frames themselves should know best and
avoid `save-window-excursion' like the plague.  At least in "released"
code.  Anything else is masochism.

martin

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> `save-window-excursion' is the macro never to use.  It should
> be used exclusively for debugging and prototyping.  Application 
> programmers who prefer to work with multiple frames themselves
> should know best and avoid `save-window-excursion' like the
> plague.  At least in "released" code.  Anything else is
> masochism.

Unbelievable language.  What dogmatism.  What hyperbole.  One can only hope that
the same swashbuckling, slash-and-burn attitude is not taken when you modify the
code.

I wonder how that particular plague escaped the attention of Richard et al for
35 years?  It's a wonder that it was ever documented, except perhaps as a mortal
danger.

bug#7728: 24.0.50; GDB backtrace from abort

[Stefan Monnier]

> I wonder how that particular plague escaped the attention of Richard
> et al for 35 years?

Hmm... last I checked, we're only up to 25 years.
As for how... easy: they only use a single frame, and in that setting it
works fine and makes perfect sense.  But it doesn't extend well to
situations with multiple frames.


        Stefan

bug#7728: 24.0.50; GDB backtrace from abort

[Sean Sieger]

Eli Zaretskii <eliz@gnu.org> writes:
    
    It could also be that this is the
    first time you are running a binary that was built with
    ENABLE_CHECKING defined: without that, the abort won't happen at all.

For what it's worth, I've been configuring the binaries found in
http://alpha.gnu.org/gnu/emacs/windows since the end of July.  So say,
since emacs-20100802.

Hope this helps; I've only been able to follow what you are writing in
passing.

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

>     It could also be that this is the
>     first time you are running a binary that was built with
>     ENABLE_CHECKING defined: without that, the abort won't 
>     happen at all.
> 
> For what it's worth, I've been configuring the binaries found in
> http://alpha.gnu.org/gnu/emacs/windows since the end of July.  So say,
> since emacs-20100802.  Hope this helps; I've only been able to follow
> what you are writing in passing.

In case it helps I just checked, using a build I have from 2010-07-19 and
another from 2010-08-02.  The former does not crash; the latter does.  So if the
difference between those two is ENABLE_CHECKING then this would lend support to
Eli's hypothesis wrt ENABLE_CHECKING.

These are those builds:

In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2010-07-19 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4) --no-opt --cflags -Ic:/xpm/include'

In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
 of 2010-08-02 on 3249CTO
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4) --no-opt --cflags -Ic:/xpm/include'

bug#7728: 24.0.50; GDB backtrace from abort

[Sean Sieger]

"Drew Adams" <drew.adams@oracle.com> writes:

    >     It could also be that this is the
    >     first time you are running a binary that was built with
    >     ENABLE_CHECKING defined: without that, the abort won't 
    >     happen at all.
    > 
    > For what it's worth, I've been configuring the binaries found in
    > http://alpha.gnu.org/gnu/emacs/windows since the end of July.  So say,
    > since emacs-20100802.  Hope this helps; I've only been able to follow
    > what you are writing in passing.

    In case it helps I just checked, using a build I have from
    2010-07-19 and another from 2010-08-02.  The former does not crash;
    the latter does.  So if the difference between those two is
    ENABLE_CHECKING then this would lend support to Eli's hypothesis wrt
    ENABLE_CHECKING.

    These are those builds:

    In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
     of 2010-07-19 on 3249CTO
    Windowing system distributor `Microsoft Corp.', version 5.1.2600
    configured using `configure --with-gcc (4.4) --no-opt --cflags
    -Ic:/xpm/include'

    In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
     of 2010-08-02 on 3249CTO
    Windowing system distributor `Microsoft Corp.', version 5.1.2600
    
    configured using `configure --with-gcc (4.4) --no-opt --cflags
    -Ic:/xpm/include'

Right.

And thanks for overlooking the lacuna, `--enable-checking'.

bug#7728: 24.0.50; GDB backtrace from abort

[Eli Zaretskii]

> From: Sean Sieger <sean.sieger@gmail.com>
> Date: Fri, 14 Jan 2011 16:46:48 -0500
> Cc: 
> 
>     In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
>      of 2010-08-02 on 3249CTO
>     Windowing system distributor `Microsoft Corp.', version 5.1.2600
>     
>     configured using `configure --with-gcc (4.4) --no-opt --cflags
>     -Ic:/xpm/include'
> 
> Right.
> 
> And thanks for overlooking the lacuna, `--enable-checking'.

I'm not sure I understand that last comment.  Are you saying you
understand why `--enable-checking' does not appear in the "configured
using" line?  Because I was about to ask that.

bug#7728: 24.0.50; GDB backtrace from abort

[Sean Sieger]

Eli Zaretskii <eliz@gnu.org> writes:

    > From: Sean Sieger <sean.sieger@gmail.com>
    > Date: Fri, 14 Jan 2011 16:46:48 -0500
    > Cc: 
    > 
    >     In GNU Emacs 24.0.50.1 (i386-mingw-nt5.1.2600)
    >      of 2010-08-02 on 3249CTO
    >     Windowing system distributor `Microsoft Corp.', version 5.1.2600
    >     
    >     configured using `configure --with-gcc (4.4) --no-opt --cflags
    >     -Ic:/xpm/include'
    > 
    > Right.
    > 
    > And thanks for overlooking the lacuna, `--enable-checking'.

    I'm not sure I understand that last comment.  Are you saying you
    understand why `--enable-checking' does not appear in the "configured
    using" line?  Because I was about to ask that.

In my followup to this thread I didn't say what I meant to say, which
was that I've been configuring the binaries with --enable-checking since
the end of July---Drew read my mail as though I said it.

bug#7728: 24.0.50; GDB backtrace from abort

[Chong Yidong]

martin rudalics <rudalics@gmx.at> writes:

> I doubt it's kosher because if the old selected window is not on the
> restored frame and a window on the restored frame gets selected, the
> point of the buffer whose window is deselected is not stored in the old
> selected window's pointm which is certainly not TRT.
>
> Inherently, the source of all evil is the idea to have (1) these two
> variables selected_frame and selected_window and (2) to allow setting
> them independently from each other.
>
> To avoid the present crash we could try something like the attached
> patch (which does not try to solve anything but that crash).

Thank you.  I don't see any consensus about what the correct long-term
fix is, but in the meantime I've committed this to the emacs-23 branch.

bug#7728: 24.0.50; GDB backtrace from abort

[martin rudalics]

 > Thank you.  I don't see any consensus about what the correct long-term
 > fix is, but in the meantime I've committed this to the emacs-23 branch.

Sorry, I don't build Emacs 23 here any more and only read the diffs from
emacs-diffs.  Is it possible that you didn't apply the hunk

***************
*** 3602,3607 ****
--- 3608,3615 ----
   			 BUF_PT (XBUFFER (ow->buffer)),
   			 BUF_PT_BYTE (XBUFFER (ow->buffer)));
       }
+   else
+     inhibit_point_swap = 0;

     selected_window = window;

from my patch?  The bug Thierry Volpiatto just reported could be
related.

Thanks, martin

bug#7728: 24.0.50; GDB backtrace from abort

[Drew Adams]

> > I wonder how that particular plague escaped the attention of Richard
> > et al for 35 years?
> 
> Hmm... last I checked, we're only up to 25 years.

We who?  Emacs != Gnu Emacs.  Emacs dev began in the mid-70s.

No, I don't know when `save-window-excursion' was introduced.  And yes, I would
guess that it was introduced before Emacs frames were introduced.  Still, it's
been a very long time now, no matter how you count it.

> As for how... easy: they only use a single frame, and in that 
> setting it works fine and makes perfect sense.  But it doesn't
> extend well to situations with multiple frames.

I won't argue against that.  I'm not knowledgable about it.

If you are sure of what you say, then I suggest that you update the doc that
tells users how to use `save-window-excursion', including together with
`save-excursion'.  `save-window-excursion' is all through the Elisp manual, and
`with-selected-frame' is not mentioned even once!

The new doc should not just cover the case where you switch windows in the same
frame or you switch frames, but the more general case where you do not know
whether some other window and frame will be selected by the wrapped code.

IOW, if you replace or add to the doc about using `save-window-excursion', then
do so in a way that is just as general.

See also the multiple caveats in the doc about `save-window-excursion', to see
whether they too need to be updated to also mention `with-selected-frame' or
`with-selected-window'.

It would be very helpful if in the doc you compared and contrasted the proper
use of `save-selected-window', `with-selected-window', `with-selected-frame, and
`save-window-excursion', pointing out when to use which, what their differences
are, gotchas, etc.  That's not clear, IMO.


In any case, knowing how to use `with-selected-frame', perhaps together with
`with-selected-window', will not help my code, which needs to support Emacs
versions that do not have these macros.  Still, I would like the doc to be
updated to make things clear.