567704 - nsWindow::Destroy race with SetModal(PR_FALSE) is causing window focus/activation to fail?

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Mats Palmgren (inactive)]

nsWindow::Destroy race with SetModal(PR_FALSE) is causing window focus/activation to fail?

In another bug I pushed a change to nsXULWindow::Destroy which added
an explicit mWindow->Destroy().  This caused previously intermittent orange
bug 527099/bug 551538 (Mooth), and bug 542580 (Mo5) to become perma-orange.
The reason is that SetModal(PR_FALSE) [after exiting the modal loop in
nsXULWindow::ShowModal()] fails if nsWindow::Destroy() has been called,
which appears to confuse window focus/activation (it resulted in a lot of
"INFO Error: Unable to restore focus, expect failures and timeouts.").

Specifically, the GetToplevelWidget call got grabWidget==nsnull so we
return early:
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsWindow.cpp#904

So my hope is that fixing this bug will fix some intermittent orange
on Linux.

Comment 1

[(no longer active)]

Could this also have something to do with the recent Linux browser-chrome oranges waiting for the focus to be set to the main test window (bug 542928 et al)?

Comment 2

[Mats Palmgren (inactive)]

Hard to say, we have so many focus related intermittent oranges.

When debugging this locally I see that some tests that open/close
modal windows sometimes causes the focus manager to have
mActiveWindow==null, and the reason for that appears to be that we
blocked an activate event because mContainerBlockFocus was true, here:
http://mxr.mozilla.org/mozilla-central/source/widget/src/gtk2/nsWindow.cpp#3109
and the reason for that is an early return in SetModal().
It's pretty hard to debug so I could be wrong...

I'll have a WIP patch tomorrow for testing...

Comment 3

[Karl Tomlinson (:karlt)]

Thanks so much for picking up on this, Mats, and for your investigation.

Would it be enough for nsWindow::Destroy to check mTransientParent and set parent->mContainerBlockFocus?

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip

Add a gratuitous SetModal(PR_FALSE) in Destroy.
Maybe it's overkill but it should be future proof against SetModal changes.
I looked briefly at the other widget sets and I think this bug could happen
there too, in theory.

Waiting for TryServer results before formally asking for review...

Comment 5

[Mats Palmgren (inactive)]

Ehsan, I got the failure you mentioned from TryServer with an early version
of this patch, so bug 542928 is likely a different problem.

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 447266 [details] [diff] [review]
wip

Unit tests pass on TryServer (trunk), also with the patch in bug 530070.
Still waiting for 1.9.2 results (TryServer seems to be down ATM).

Comment 7

[Karl Tomlinson (:karlt)]

I think SetModal(aModal) should return failure if the window is destroyed, at least for aModal == true, because the caller may wish to know if the request wasn't fulfilled.

It seems that mContainerBlockFocus is not needed any more as nsXULWindow::ShowModal does something similar:
http://hg.mozilla.org/mozilla-central/annotate/ffbc3baf03ae/xpfe/appshell/src/nsXULWindow.cpp#l399

Reverting the patch for bug 290862 doesn't restore the bug on the STR there.
Similarly with a pre-focus-manager build.

I'm trying to work out why the patch was originally required.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to comment #15)
> (In reply to comment #14)
>
> Various Firefox notification UI wants to use more panels but I think mostly
> they want non-noautohide panels which already work ok.

> 1. Non-noautohide panels disappear when clicking outside of them. They do not
> support titlebars but can be either at the 'parent' level or 'top' level.

Yes, these should mostly work fine, though we're getting a bit lucky that bug
568101 doesn't prevent content in iframes from being focused in non-noautohide
panels.

I'm not grasping why anyone would want a 'parent' level non-noautohide panel.
IIUC this is so that the parent could be (partially?) hidden by another
application or browser main window, but there is no way to raise the panel to
make it (completely) visible.  (Clicking to lower the covering window would
initiate hiding of the panel.)

> 2. 'Parent' level noautohide panels appear just above their parent. Their
> position is maintained relative to the owning window if the window is moved.
> Titlebars don't need to be supported on these if it isn't feasible. This type
> is used for the Customize Toolbar dialog on Mac.

I think we can do non-titlebar versions of these with only widget
modifications, probably best with option C, though B is a fallback option that
would require clicking the panel to focus elements.

The window manager may choose whether or not to hide these when the owner is
not active.  Usually we should not try to override the window manager without
good reason, but we may be able to provide some different hints such that the
window manager is less likely to do this, if we think this is necessary.

Comment 9

[Karl Tomlinson (:karlt)]

Sorry, comment 8 was meant for bug 526941.

Comment 10

[Karl Tomlinson (:karlt)]

(In reply to comment #7)
> I'm trying to work out why the patch was originally required.

That was because nsWindow(PRBool aState) was just returning not implemented.
That was fixed in http://hg.mozilla.org/mozilla-central/rev/de267a920411
which is in 1.9.2 but not 1.9.1.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 447266 [details] [diff] [review]
wip

The right thing to do here is just remove mContainerBlockFocus.

Comment 12

[Karl Tomlinson (:karlt)]

On 1.9.2, mContainerBlockFocus is also used for other purposes, so there, just removing the "if (mTransientParent)" block from SetModal is appropriate.

Comment 13

[Mats Palmgren (inactive)]

Attachment: wip7 (for trunk)

Thanks for your help, Karl.  I'm removing mContainerBlockFocus as you
suggested.  There's one issue though - we still need to return early in
nsWindow::OnContainerFocusInEvent when !mEnabled, right?
Otherwise gFocusWindow would be out-of-sync with the focus manager.

mContainerGot/LostFocus are currently unused so I'm removing them too.

The TryServer is a bit flaky right now but it looks like this passed,
also together with the patch in bug 530070.

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 447879 [details] [diff] [review]
wip7 (for trunk)

It seems that the only reason to have mModal (and the SetModal from Destroy)
is to handle a case where SetModal is called on a child window and it gets
destroyed before the toplevel widget gets destroyed.

However, this would be a bit weird anyway because mModal is on the child
window but really it is the toplevel that carries the state.

What do you think about making SetModal fail if not called on a toplevel
nsWindow?  It looks like this would be consistent with the cocoa
port only having an implementation on nsCocoaWindow not nsChildView.

If SetModal is called on a toplevel nsWindow (which is not destroyed) mShell
will be set and is the appropriate grab widget.  When the modal nsWindow gets
destroyed, mShell will be destroyed and gtk will clean up appropriately
(without any need for direct action from Destroy).

(In reply to comment #13)
> There's one issue though - we still need to return early in
> nsWindow::OnContainerFocusInEvent when !mEnabled, right?
> Otherwise gFocusWindow would be out-of-sync with the focus manager.

I don't think it actually makes any difference because gFocusWindow will get
set properly when focus returns to the modal window, and changing gFocusWindow
from NULL to this nsWindow is just setting it to the same nsWindow that
key_press_event_cb will dispatch the event to when gFocusWindow is NULL.
(Key events are only being ignored because get_window_for_gtk_widget is
the toplevel nsIWidget, which doesn't listen for key events.)

> mContainerGot/LostFocus are currently unused so I'm removing them too.

Great, thanks.

Comment 15

[Mats Palmgren (inactive)]

Attachment: wip8 (for trunk)

(In reply to comment #14)
> What do you think about making SetModal fail if not called on a toplevel
> nsWindow?  ...

Yes, this seems logical, however it's a slightly more risky change IMO.

Comment 16

[Mats Palmgren (inactive)]

Attachment: wip8 (for 1.9.1/1.9.2)

Same SetModal() change, but keep the mContainerBlockFocus bit.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 448390 [details] [diff] [review]
wip8 (for trunk)

Thank you for tidying this up, Mats.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 448393 [details] [diff] [review]
wip8 (for 1.9.1/1.9.2)

This is good for 1.9.2, but (on its own) would not be suitable for 1.9.1 because IsEnabled() is broken on 1.9.1.

Comment 19

[Mats Palmgren (inactive)]

(In reply to comment #18)
> because IsEnabled() is broken on 1.9.1.

I don't see what the problem is on 1.9.1.
AFAICT, nsWindow::Enable/IsEnabled and the related code in
nsXULWindow::EnableParent/ShowModal is identical on 1.9.1 and 1.9.2

Comment 20

[Karl Tomlinson (:karlt)]

Sorry, you are right.  IsEnabled() is fixed in 1.9.1.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/de267a920411
So attachment 448393 [details] [diff] [review] is good for 1.9.1 also.

Comment 21

[Mats Palmgren (inactive)]

No problem, I'll ask for approval for 1.9.1/1.9.2 after some baking.

http://hg.mozilla.org/mozilla-central/rev/7ecd591492af

Comment 22

[(no longer active)]

As Mats said in comment 5, this has nothing to do with bug 543278 and friends.