bug#16967: frame related race condition

bug#16967: frame related race condition

[Juanma Barranquero]

Package: emacs
Version: 24.3.50

emacs -Q

then type this in *scratch*

(let* ((c (selected-frame))
       (f (make-frame)))
  (sit-for 0)
  (select-frame-set-input-focus f)
  (sit-for 0)
  (delete-frame c))

then evaluate the let repeteadly. It's not hard to get "let*: Attempt
to delete the sole visible or iconified frame".

bug#16967: frame related race condition

[martin rudalics]

 > emacs -Q
 >
 > then type this in *scratch*
 >
 > (let* ((c (selected-frame))
 >        (f (make-frame)))
 >   (sit-for 0)
 >   (select-frame-set-input-focus f)
 >   (sit-for 0)
 >   (delete-frame c))
 >
 > then evaluate the let repeteadly. It's not hard to get "let*: Attempt
 > to delete the sole visible or iconified frame".

Confirmed.  But doing

(while t
   (let* ((c (selected-frame))
	 (f (make-frame)))
     (sit-for 0)
     (select-frame-set-input-focus f)
     (sit-for 0)
     (delete-frame c)))

here chokes only the first time around and proceeds without complaints
afterwards.  Is it that what you mean or are there additional problems?

And if possible, can you show the value of f->visible of the other
frame, that is the one that should be retained, at the time Emacs
complains that you want to delete its only visible or iconified frame?

martin

bug#16967: frame related race condition

[martin rudalics]

I checked in a fix in revision 116716.  With that I can't see the
problem any more.  Can you?

martin

bug#16967: frame related race condition

[Juanma Barranquero]

On Mon, Mar 10, 2014 at 11:32 AM, martin rudalics <rudalics@gmx.at> wrote:

> I checked in a fix in revision 116716.  With that I can't see the
> problem any more.  Can you?

No, but it has a much bigger problem:

 emacs -Q --eval "(make-frame '((visibility . nil)))"

and you end with two visible frames.

I think we're rehashing part of bug#14841's thread.

bug#16967: frame related race condition

[martin rudalics]

 >> I checked in a fix in revision 116716.  With that I can't see the
 >> problem any more.  Can you?
 >
 > No, but it has a much bigger problem:
 >
 >  emacs -Q --eval "(make-frame '((visibility . nil)))"
 >
 > and you end with two visible frames.
 >
 > I think we're rehashing part of bug#14841's thread.

When we get a SIZE_RESTORED message we have to make the frame visible.
Do you see another way?

martin

bug#16967: frame related race condition

[Juanma Barranquero]

On Mon, Mar 10, 2014 at 12:45 PM, martin rudalics <rudalics@gmx.at> wrote:

> When we get a SIZE_RESTORED message we have to make the frame visible.

Well, yeah, no. I don't know why an invisible frame gets a
SIZE_RESTORED message, but as long as Emacs thinks it's invisible, no,
it shouldn't be made visible. The WM is at the service of Emacs, not
the other way around.

Or are you saying that you find acceptable being unable to make an
invisible frame?

bug#16967: frame related race condition

[martin rudalics]

 >> When we get a SIZE_RESTORED message we have to make the frame visible.
 >
 > Well, yeah, no. I don't know why an invisible frame gets a
 > SIZE_RESTORED message, but as long as Emacs thinks it's invisible, no,
 > it shouldn't be made visible. The WM is at the service of Emacs, not
 > the other way around.

No.  But the `make-frame' in

(let* ((c (selected-frame))
        (f (make-frame)))
   (sit-for 0)
   (select-frame-set-input-focus f)
   (sit-for 0)
   (delete-frame c))

should create a normally visible frame f.  The fact that this frame has
its visibility set to zero at the time you `delete-frame' c indicates
that we have a pretty awful bug.  The implications of this are
substantial because SET_FRAME_VISIBLE has to redisplay_other_windows and
if that is not done, the consequences are not restricted to the toy
scenario you gave.

Lately I frequently noticed that an Emacs frame that was for some time
hidden by other applications and subsequently became exposed by deleting
their windows was not redrawn and I would like to know whether this was
the reason.  ISTR that others noted the same or a similar misbehavior.

 > Or are you saying that you find acceptable being unable to make an
 > invisible frame?

No.  But we apparently have the problem that Emacs on Windows thinks
that a frame is invisible although it isn't.  And we have to find out
where this notion of invisibility gets introduced - maybe it's easy to
spot it, maybe, likely it's part of my pixelwise changes, and we can
withdraw my "fix" soon.  But till then we have to live with the
situation that on Windows invisible Emacs frames are visible :-(

martin

bug#16967: frame related race condition

[Juanma Barranquero]

On Mon, Mar 10, 2014 at 2:11 PM, martin rudalics <rudalics@gmx.at> wrote:

> should create a normally visible frame f.  The fact that this frame has
> its visibility set to zero at the time you `delete-frame' c indicates
> that we have a pretty awful bug.

Yes

> The implications of this are
> substantial because SET_FRAME_VISIBLE has to redisplay_other_windows and
> if that is not done, the consequences are not restricted to the toy
> scenario you gave.

I don't know what "toy scenario" are you refering to, but certainly

  emacs -Q
  M-: (make-frame '((visibility))) <RET>

is not a toy scenario *at all*. For one, it will prevent
frameset-restore to restore invisible frames (I could work around it,
but it'll be a hack).

> No.  But we apparently have the problem that Emacs on Windows thinks
> that a frame is invisible although it isn't.  And we have to find out
> where this notion of invisibility gets introduced - maybe it's easy to
> spot it, maybe, likely it's part of my pixelwise changes, and we can
> withdraw my "fix" soon.

I think bug#14841 is a clue that the visibility mismatch between Emacs
and the Windows wm predates your pixelwise changes.

>  But till then we have to live with the
> situation that on Windows invisible Emacs frames are visible :-(

I would certainly prefer that you reverted your last change. You're
fixing an occasional problem and introducing a perfectly repeatable
one.

   J

bug#16967: frame related race condition

[martin rudalics]

 >> should create a normally visible frame f.  The fact that this frame has
 >> its visibility set to zero at the time you `delete-frame' c indicates
 >> that we have a pretty awful bug.
 >
 > Yes

Really?  I was just starting to think otherwise.

 >> The implications of this are
 >> substantial because SET_FRAME_VISIBLE has to redisplay_other_windows and
 >> if that is not done, the consequences are not restricted to the toy
 >> scenario you gave.
 >
 > I don't know what "toy scenario" are you refering to,

The one of this bug report which IIUC even you consider "an occasional
problem" ;-)

 > but certainly
 >
 >   emacs -Q
 >   M-: (make-frame '((visibility))) <RET>
 >
 > is not a toy scenario *at all*. For one, it will prevent
 > frameset-restore to restore invisible frames (I could work around it,
 > but it'll be a hack).

OK.  Then I have a motivation to revert it.

 >> No.  But we apparently have the problem that Emacs on Windows thinks
 >> that a frame is invisible although it isn't.  And we have to find out
 >> where this notion of invisibility gets introduced - maybe it's easy to
 >> spot it, maybe, likely it's part of my pixelwise changes, and we can
 >> withdraw my "fix" soon.
 >
 > I think bug#14841 is a clue that the visibility mismatch between Emacs
 > and the Windows wm predates your pixelwise changes.

I think that I misjudged the severity of the problem.  Drew's latest
reports hint at some mysterious behavior which I haven't been able to
understand yet so I'm suspecting potential culprits around every corner.

 > I would certainly prefer that you reverted your last change.

Done.

 > You're
 > fixing an occasional problem and introducing a perfectly repeatable
 > one.

Sorry for the inconvenience.

martin

bug#16967: frame related race condition

[Juanma Barranquero]

On Mon, Mar 10, 2014 at 8:04 PM, martin rudalics <rudalics@gmx.at> wrote:

> Really?  I was just starting to think otherwise.

?

> Done.

Thanks.

> Sorry for the inconvenience.

Sorry if I was whiny, it was entirely unintentional. I'm grateful for
your effort fixing these hard bugs.

bug#16967: frame related race condition

[martin rudalics]

 >> Really?  I was just starting to think otherwise.
 >
 > ?

Earlier in this thread I wrote that ...

 > The fact that this frame has its visibility set to zero at the time
 > you `delete-frame' c indicates that we have a pretty awful bug.

... and you agreed by saying ...

 >  The fact that this frame has
 > > its visibility set to zero at the time you `delete-frame' c indicates
 > > that we have a pretty awful bug.

 > Yes

... to which I replied ...

 > >> The fact that this frame has
 > >> its visibility set to zero at the time you `delete-frame' c indicates
 > >> that we have a pretty awful bug.
 > >
 > > Yes
 >
 > Really?  I was just starting to think otherwise.

... so I was doubting that you really agreed with my judgment of this
bug.

Anyway, the current state of things is IMHO just as bad as it was at the
time you started to write the report for bug 14841.  In particular, the
frame.h hack you mention in that thread

   /* Nonzero if the frame is currently displayed; we check
      it to see if we should bother updating the frame's contents.

      On ttys and on Windows NT/9X, to avoid wasting effort updating
      visible frames that are actually completely obscured by other
      windows on the display, we bend the meaning of visible slightly:
      if equal to 2, then the frame is obscured - we still consider
      it to be "visible" as seen from lisp, but we don't bother
      updating it.  */
   unsigned visible : 2;

is likely responsible for the fact that Emacs doesn't always redisplay a
frame when I remove the window of another application obscuring it.  I'm
still convinced that we should call SET_FRAME_VISIBLE, at least when
f->visible equals 2, in SIZE_RESTORED.  But I'm also convinced that
allowing a value of 2 was bad karma in the first place.  So I see no
other way but reconsidering this design from scratch :-(

BTW: The more I look into this, the more I'm convinced that implementing
frame parameters on top of the old frame infrastructure was one of the
worst design ideas ever.

martin

bug#16967: frame related race condition

[Juanma Barranquero]

On Tue, Mar 11, 2014 at 9:07 AM, martin rudalics <rudalics@gmx.at> wrote:

> ... so I was doubting that you really agreed with my judgment of this
> bug.

It would be hard to deny that there are nasty bugs related to frames
and the Windows wm. So yes, I agree with your judgment.

> BTW: The more I look into this, the more I'm convinced that implementing
> frame parameters on top of the old frame infrastructure was one of the
> worst design ideas ever.

:-)

bug#16967: frame related race condition

[Stefan Monnier]

>   /* Nonzero if the frame is currently displayed; we check
>      it to see if we should bother updating the frame's contents.

>      On ttys and on Windows NT/9X, to avoid wasting effort updating
>      visible frames that are actually completely obscured by other
>      windows on the display, we bend the meaning of visible slightly:
>      if equal to 2, then the frame is obscured - we still consider
>      it to be "visible" as seen from lisp, but we don't bother
>      updating it.  */
>   unsigned visible : 2;

Hmm... I didn't realize this "visible=2" is also used in the Windows GUI.
So maybe the "visible=2" case under Windows is indeed mishandled by the
"redisplay bit" code.  Or by some other part of the code.

At least frame.h does:

   SET_FRAME_VISIBLE (struct frame *f, int v)
   {
     eassert (0 <= v && v <= 2);
     if (v == 1 && f->visible != 1)
       redisplay_other_windows ();
     f->visible = v;
   }

so it should handle the w32 case correctly.

> is likely responsible for the fact that Emacs doesn't always redisplay a
> frame when I remove the window of another application obscuring it.  I'm
> still convinced that we should call SET_FRAME_VISIBLE, at least when
> f->visible equals 2, in SIZE_RESTORED.

I'm not sure what SIZE_RESTORED is for, but indeed when we receive
a size-change notification, the "visible=2" optimization might not be
valid any more so we should set it back to 1.

And we should probably also set it back to 1 when we receive expose
events on that frame.

But I'm generally clueless about GUI code, and even more clueless about
w32, so please don't take my word for it.

> BTW: The more I look into this, the more I'm convinced that implementing
> frame parameters on top of the old frame infrastructure was one of the
> worst design ideas ever.

I have no idea what this is referring to.


        Stefan

bug#16967: frame related race condition

[martin rudalics]

 > Hmm... I didn't realize this "visible=2" is also used in the Windows GUI.
 > So maybe the "visible=2" case under Windows is indeed mishandled by the
 > "redisplay bit" code.  Or by some other part of the code.

If a frame is obscured (has visible=2) and we get a SIZE_RESTORED
message for it, we probably should do some redisplay stuff but I'm not
sure whether either redisplay_other_windows or SET_FRAME_GARBAGED does
accomplish something the other doesn't.  However, soon after I removed
the conditioning on `iconified' I got my usualy "non-redrawing a
previously obscured frame" behavior and decided that my cure wasn't
useful anyway.

 > I'm not sure what SIZE_RESTORED is for, but indeed when we receive
 > a size-change notification, the "visible=2" optimization might not be
 > valid any more so we should set it back to 1.
 >
 > And we should probably also set it back to 1 when we receive expose
 > events on that frame.
 >
 > But I'm generally clueless about GUI code, and even more clueless about
 > w32, so please don't take my word for it.

As explained above, I haven't been able to stop the indecent behavior of
obscured frames I observed with an even more radical approach, so I'm
clueless as well.

 >> BTW: The more I look into this, the more I'm convinced that implementing
 >> frame parameters on top of the old frame infrastructure was one of the
 >> worst design ideas ever.
 >
 > I have no idea what this is referring to.

In the case at hand it refers to the fact that when searching for the
cause of some strange behavior of frame visibility, it's not sufficient
to grep just for `make-frame-visible'.  You also have to spot things
like (set-frame-parameter frame 'visibility t) and maybe all occurrences
of `modify-frame-parameters' and `modify-all-frames-parameters'.
Obviously searching for 'visibility only is more efficient but this will
fail miserably with more mundane words like `right' or `left'.  So the
introduction of frame parametes often multiplies the effort necessary to
find the cause of frame related bugs.

martin

bug#16967: frame related race condition

[Stefan Monnier]

> If a frame is obscured (has visible=2) and we get a SIZE_RESTORED
> message for it, we probably should do some redisplay stuff but I'm not
> sure whether either redisplay_other_windows or SET_FRAME_GARBAGED does
> accomplish something the other doesn't.

redisplay_other_windows only tells Emacs that the next redisplay will
need to loop through all windows to see which ones need to be
redisplayed.  Without it, Emacs will only try to redisplay selected-window.

SET_FRAME_GARBAGED tells Emacs that this frame needs to be redrawn.
It calls redisplay_other_windows unconditionally.

> However, soon after I removed the conditioning on `iconified' I got my
> usually "non-redrawing a previously obscured frame" behavior and
> decided that my cure wasn't useful anyway.

Presumably, un-obscuring a frame shouldn't trigger any SIZE_RESTORED
thingy since the size is unaffected, right?

>> And we should probably also set it back to 1 when we receive expose
>> events on that frame.

Can you try to do this?

>>> BTW: The more I look into this, the more I'm convinced that implementing
>>> frame parameters on top of the old frame infrastructure was one of the
>>> worst design ideas ever.
>> I have no idea what this is referring to.
> In the case at hand it refers to the fact that when searching for the
> cause of some strange behavior of frame visibility, it's not sufficient
> to grep just for `make-frame-visible'.

Oh, that.  Yes, I'm not very happy with the use of frame-parameters to
affect "internal" frame properties.  It has never *really* bothered me,
but it seems like asking for trouble.


        Stefan

bug#16967: frame related race condition

[Stefan Monnier]

> Lately I frequently noticed that an Emacs frame that was for some time
> hidden by other applications and subsequently became exposed by deleting
> their windows was not redrawn and I would like to know whether this was
> the reason.  ISTR that others noted the same or a similar misbehavior.

My "redisplay bit" changes of a few months back introduced such bugs.
I haven't seen such problems for a while now, so I think I've caught all
the problems, but maybe I still missed some.

Part of the change is that previously iconified/invisible frames where
redisplayed right away (i.e. their glyph matrices were kept up-to-date),
whereas now they're not.  Which means that when they're uniconified or
made visible, we have to first set windows_or_buffers_changed to
REDISPLAY_SOME, to make sure that the subsequent redisplay doesn't
forget to look at them.


        Stefan

bug#16967: frame related race condition

[martin rudalics]

 >> Lately I frequently noticed that an Emacs frame that was for some time
 >> hidden by other applications and subsequently became exposed by deleting
 >> their windows was not redrawn and I would like to know whether this was
 >> the reason.  ISTR that others noted the same or a similar misbehavior.
 >
 > My "redisplay bit" changes of a few months back introduced such bugs.
 > I haven't seen such problems for a while now, so I think I've caught all
 > the problems, but maybe I still missed some.
 >
 > Part of the change is that previously iconified/invisible frames where
 > redisplayed right away (i.e. their glyph matrices were kept up-to-date),
 > whereas now they're not.  Which means that when they're uniconified or
 > made visible, we have to first set windows_or_buffers_changed to
 > REDISPLAY_SOME, to make sure that the subsequent redisplay doesn't
 > forget to look at them.

In all cases I observed this, the Emacs frame was neither invisible nor
iconified before, hence there was no uniconifying or making it visible
involved.  And since I don't recall observing this problem on GNU/Linux,
I'm quite convinced that it's merely Emacs mishandling frames obscured
for some time on Windows.

martin

bug#16967: frame related race condition

[Juanma Barranquero]

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

Though the thread was inconclusive,  the bug was likely fixed by subsequent
changes by Martin and does not happen with the current trunk, so I'm
closing this one.

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