1224790 - Mouse stops working after closing modal dialog that opened window. [Mac]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tooru Fujisawa [:arai]]

originally reported to http://forums.mozillazine.jp/viewtopic.php?f=3&t=15771

Steps to reproduce:
  1. Run Thunderbird with clean profile
  2. Create Feed Account
    2.1. Open "Account Setting"
    2.2. Click "Account Actions"-"Add Feed Account..."
    2.3. Click "Continue"
    2.4. Click "Done"
  3. Click "Manage Subscriptions..." in "Account Setting" dialog
  4. Click "Close" to close "Feed Subscriptions" dialog
  5. Click "OK" to close "Account Setting" dialog

Expected result:

Mouse click keeps working

Actual result:

Mouse (or any pointing device) click stops working

Keyboard still works in some case. Menu bar also works (maybe only on OSX?) but newly opened windows also doesn't respond to mouse events.
Following modal dialogs keeps working:
  * Account Manager (re-opened after Step 5)
  * New Account Setup (Click "New Message")

Last good build: https://ftp.mozilla.org/pub/thunderbird/nightly/2010/12/2010-12-08-03-comm-central/
First bad build: https://ftp.mozilla.org/pub/thunderbird/nightly/2010/12/2010-12-09-13-comm-central/
Regression windows
  comm-central: http://hg.mozilla.org/comm-central/pushloghtml?fromchange=1dc9fb072bf4&tochange=61c697891f6c
  mozilla-central (just a guess from date): https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=95452499f3d6&tochange=58dcad7165be

might be related to bug 1032619.

Comment 1

[Tooru Fujisawa [:arai]]

This issue disappears by removing "dialog=no" from following call.
https://dxr.mozilla.org/comm-central/rev/c460d51c3548885903f40c61ecdd5cd737134766/mailnews/extensions/newsblog/content/newsblogOverlay.js#267
>     window.openDialog("chrome://messenger-newsblog/content/feed-subscriptions.xul",
>                       "", "centerscreen,chrome,dialog=no,resizable",
>                       { folder: aFolder});

Could be a bug related to modal dialog.

Comment 2

[Tooru Fujisawa [:arai]]

Looks like it's mozilla-central's bug

will post a detail with testcase.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Extension with a testcase for Firefox

Steps to reproduce:
  1. Set xpinstall.signatures.required to false in about:config for Step 2
  2. Install attached "test" extension and restart
  3. Open "Add-ons Manager" - "Extensions" tab
  4. Click "Preferences" in test extension
  5. Click "Step 5: Open modal dialog" button in prefernces window
  6. Click "Step 6: Open child window" button in modal dialog
  7. Click "Step 7: Close child window" button in child window
  8. Click "Step 8: Close modal dialog" button in modal dialog

Expected result:

Mouse keeps working

Actual result:

Mouse (or any pointing device) click stops working.

Attaches extension does following:

1. open modal dialog
  window.openDialog('chrome://test/content/modal.xul', '', 'modal');
2. open window with 'dialog=no' inside modal dialog
  window.openDialog('chrome://test/content/window.xul', '', 'dialog=no');

Regression window (comment #0 was wrong):
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=11e328a49e0a&tochange=a4544a4b3224
bug 406375 touches modal dialog.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: (testing) partially backout bug 406375 for testing.

the issue disappears by partially backing out bug 406375 patch.

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8687519 [details]
(testing) partially backout bug 406375 for testing.

this is not a fix, just a test.

Comment 6

[Tooru Fujisawa [:arai]]

This seems to be OSX specific issue.
Not reproducible on linux nor Windows.

Comment 7

[Tooru Fujisawa [:arai]]

Looks like |SetModal(false)| or equivalent code is missing for following call, and that makes parent windows in wrong state:
https://dxr.mozilla.org/mozilla-central/rev/faf815a0fa9b052a38bce00c0c2aa1e2c9610936/embedding/components/windowwatcher/nsWindowWatcher.cpp#1056
>           parentWidget->SetModal(true);

We could call |SetModal(false)| in nsCocoaWindow::Destroy() or somewhere:
https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaWindow.mm#502

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Tooru Fujisawa [:arai] from comment #7)
> Looks like |SetModal(false)| or equivalent code is missing for following
> call, and that makes parent windows in wrong state:
> https://dxr.mozilla.org/mozilla-central/rev/
> faf815a0fa9b052a38bce00c0c2aa1e2c9610936/embedding/components/windowwatcher/
> nsWindowWatcher.cpp#1056
> >           parentWidget->SetModal(true);
Not sure what you're saying here. Which widget should get SetModal(false)?



> 
> We could call |SetModal(false)| in nsCocoaWindow::Destroy() or somewhere:
> https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaWindow.
> mm#502

Ah, that might work. Loks like Cocoa backend has rather complicated SetModal implementation comparing to the
other platforms .

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Though, need to be careful. That cocoa backend seems to rely on setModal(true) to always have corresponding setModal(false), and other way round.
We may need to add some setFakeModal to nsIWidget just to deal with this.
By default it would just call setModal, but on cocoa backend it would set some flag on 
nsCocoaWindow to tell whether setModal(false) is needed.

Comment 10

[Tooru Fujisawa [:arai]]

Thanks, yes, |SetModal(true)| and |SetModal(false)| should be called exactly same time, possibly to same widget.

I think we could use mModal flag, as it's set to true in |SetModal(true)|, and set back to false only in |SetModal(false)|.  (as long as |SetModal(true)| is never called twice to same widget)

Then, If I put |if (mModal) SetModal(false)| in nsCocoaWindow::Destroy(), it works with following steps:
  * Click "Preferences" in test extension
  * Click "Step 5: Open modal dialog" button in preferences window
  * Click "Step 6: Open child window" button in modal dialog
  * Click "Step 7: Close child window" button in child window
  * Click "Step 8: Close modal dialog" button in modal dialog

but doesn't work with following steps:
  * Click "Preferences" in test extension
  * Click "Step 5: Open modal dialog" button in preferences window
  * Click "Step 6: Open child window" button in modal dialog
  * Click "Step 8: Close modal dialog" button in modal dialog
  * Focus browser window (not sure if this step is necessary...)
  * Click "Step 7: Close child window" button in child window

with latter steps, "preferences window" keeps unresponsive, as "child window"'s |SetModal(false)| doesn't traverse to "preferences window" in following loop:
https://dxr.mozilla.org/mozilla-central/rev/faf815a0fa9b052a38bce00c0c2aa1e2c9610936/widget/cocoa/nsCocoaWindow.mm#642
>       while (aParent) {
>         if (--aParent->mNumModalDescendents == 0) {
>           NSWindow *aWindow = aParent->GetCocoaWindow();
>           if (aParent->mWindowType != eWindowType_invisible) {
>             [[aWindow standardWindowButton:NSWindowCloseButton] setEnabled:YES];
>             [[aWindow standardWindowButton:NSWindowMiniaturizeButton] setEnabled:YES];
>             [[aWindow standardWindowButton:NSWindowZoomButton] setEnabled:YES];
>           }
>         }
>         NS_ASSERTION(aParent->mNumModalDescendents >= 0, "Widget hierarchy changed while modal!");
>         aParent = static_cast<nsCocoaWindow*>(aParent->mParent);
>       }

We may need yet another link than mParent, that can traverse all ancestor windows that is still alive.

Comment 11

[Tooru Fujisawa [:arai]]

try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbfcf6e8bcfc

At lease it fixes the testcase and Thunderbird Feed Subscription dialog.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

So the normal case when SetModal(true) is followed by SetModal(false) shouldn't be changed.
Talking about http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsXULWindow.cpp#390
Though, I think SetModal(false) is there guaranteed to be called before widget's Destroy, based on 
http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/nsXULWindow.cpp#474

It is only this weird modal-window trying to open non-modal case we need and want to change.

Comment 13

[alta88]

As fyi, this was reported in Bug 956618, which was duped (perhaps erroneously) to Bug 476541, which precedes Bug 406375 landing though.

Comment 14

[Tooru Fujisawa [:arai]]

Thanks again :)

(In reply to Olli Pettay [:smaug] from comment #12)
> It is only this weird modal-window trying to open non-modal case we need and
> want to change.
okay, SetFakeModal and a separated flag will fix it.


(In reply to alta88 from comment #13)
> As fyi, this was reported in Bug 956618, which was duped (perhaps
> erroneously) to Bug 476541, which precedes Bug 406375 landing though.
Now running try for fixed patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4184b66d440
and, at least, it fixes following cases in bug 406375:
  bug 476541 comment #0
    1) Load https://bugzilla.mozilla.org/attachment.cgi?id=351645
    2) Hit Cmd+S before the alert comes up
    3) Wait...
  bug 476541 comment #16
    data:text/html,<iframe src="javascript:print()"></iframe><iframe src="javascript:alert(0)"></iframe>
Not yet confirmed other testcases (there are so much cases, including duplicated bugs....)

Comment 15

[Tooru Fujisawa [:arai]]

copy-n-pasted wrong bug number :P
I meant bug 476541 for testcases.

Comment 16

[alta88]

Fixing Bug 476541 qualifies as truly heroic!

Comment 17

[Tooru Fujisawa [:arai]]

sorry I was wrong, I misinterpreted those testcases
  * those are buggy only on non-e10s
  * the alert can be closed before save/print dialogs (maybe something has been changed from the report)
    but save/print dialogs cannot be closed before the alert
so, they're not fixed with this patch, but now those testcases don't make Firefox entirely freeze.

Sorry for false hope :P

Comment 18

[Tooru Fujisawa [:arai]]

To be clear there are 2 issues related to modal dialog:

steps to reproduce, and window states for each steps:

  1. open 2 non-modal windows (A, B)
  2. open modal dialog (C) from A
     A gets unresponsive (expected, because C is window-modal)
     B is still responsive (expected)
  3. open non-modal window (D) from C
     A is still unresponsive (expected, because C is still opened)
     B gets unresponsive (NOT EXPECTED, because there is no app-modal) [*2]
     C is still responsive (expected, because D is non-modal)
  4. close D
     A is still unresponsive (expected, because C is still opened)
     B is still unresponsive (still NOT EXPECTED) [*1]
     C is still responsive (still expected)
  5. close C
     A is still unresponsive (NOT EXPECTED, because there is no modal) [*1]
     B is still unresponsive (still NOT EXPECTED) [*1]

another steps (Close modal before non-modal child):

  1. open 2 non-modal windows (A, B)
  2. open modal dialog (C) from A
     A gets unresponsive (expected, because C is window-modal)
     B is still responsive (expected)
  3. open non-modal window (D) from C
     A is still unresponsive (expected, because C is still opened)
     B gets unresponsive (NOT EXPECTED, because there is no app-modal) [*2]
     C is still responsive (expected, because D is non-modal)
  4. close C
     A is still unresponsive (NOT EXPECTED, because there is no modal) [*2]
     B is still unresponsive (still NOT EXPECTED) [*2]
     D is still responsive (expected, because there is no modal)
  5. close D
     A is still unresponsive (still NOT EXPECTED) [*1]
     B is still unresponsive (still NOT EXPECTED) [*1]

With this patch they become following:

  1. open 2 non-modal windows (A, B)
  2. open modal dialog (C) from A
     A gets unresponsive (expected, because C is window-modal)
     B is still responsive (expected)
  3. open non-modal window (D) from C
     A is still unresponsive (expected, because C is still opened)
     B gets unresponsive (NOT EXPECTED, because there is no app-modal) [*2]
     C is still responsive (expected, because D is non-modal)
  4. close D
     A is still unresponsive (expected, because C is still opened)
     B comes back to responsive (expected, because there is no app-modal) [*1]
     C is still responsive (still expected)
  5. close C
     A comes back to responsive (expected, because there is no modal) [*1]
     B is still responsive (expected) [*1]

another steps (Close modal before non-modal child):

  1. open 2 non-modal windows (A, B)
  2. open modal dialog (C) from A
     A gets unresponsive (expected, because C is window-modal)
     B is still responsive (expected)
  3. open non-modal window (D) from C
     A is still unresponsive (expected, because C is still opened)
     B gets unresponsive (NOT EXPECTED, because there is no app-modal) [*2]
     C is still responsive (expected, because D is non-modal)
  4. close C
     A is still unresponsive (NOT EXPECTED, because there is no modal) [*2]
     B is still unresponsive (still NOT EXPECTED) [*2]
     D is still responsive (expected, because there is no modal)
  5. close D
     A comes back to responsive (expected, because there is no modal) [*1]
     B comes back to responsive (expected, because there is no app-modal) [*1]

So, in above steps, states marked as [*1] are fixed by this patch, but [*2] are not fixed.
[*1] is a bug that other windows don't come back to responsive.
[*2] is a bug that non-modal window opened by window-modal dialog is treated as a *strange* app-modal.

Some of issues in bug 476541 are saying about [*2], so this bug is not fully dupe of bug 476541 itself, but some other issues in bug 476541 and some duplicating bugs are dupe of this bug.

Comment 19

[Tooru Fujisawa [:arai]]

in addition to that, an modal (alert/confirm) opened by content seems to work differently than an modal opened by chrome.
comment #18 (and test extension, Thunderbird Feed Subscription) is saying about modal opened by chrome, but some issues in bug 476541 is saying about modal opened by content.

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Use SetFakeModal instead of SetModal for non-modal window opened by modal window.

Changed following:
  * Added nsCocoaWindow.mAncestorLink, that is almost same as mParent, but it gets re-linked to grandparent window in parent window's destructor
  * Call |SetFakeModal(true)| instead of |SetModal(true)| for non-modal window opened by modal dialog
  * SetFakeModal calls SetModal with same argument
  * on Cocoa, SetFakeModal sets mFakeModal to passed argument
  * Call |SetFakeModal(false)| in nsCocoaWindow::Destroy, when |mFakeModal| is true
  * Added 2 testcases that basically does comment #3

In the testcase, I used ctypes to emulate native mouse event (synthesizeNativeClick), because synthesizeMouseAtCenter *works* even if the bug happens.  Do you know better solution?

Green on try runs:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4184b66d440 (forgot to remove known-failure in dom/tests/mochitest/bugs/test_bug406375.html)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af6ca98bd40 (fixed dom/tests/mochitest/bugs/test_bug406375.html)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2cd929e76c (added one more testcase for mAncestorLink re-link)

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8687618 [details] [diff] [review]
Use SetFakeModal instead of SetModal for non-modal window opened by modal window.



>+/*
>+ * Synthesize a native mouse click event at a particular point in screen.
>+ * This function should be used only for testing native event loop.
>+ * Use synthesizeMouse instead for most case.
>+ *
>+ * This works only on OS X.  Throws an error on other OS.  Also throws an error
>+ * when the library or any of function are not found, or something goes wrong
>+ * in native functions.
>+ */
>+function synthesizeNativeClick(x, y)
Perhaps call this method then synthesizeNativeOSXClick
and maybe even ifdef so that it is visible only on OSX


>+++ b/widget/nsIWidget.h
>@@ -586,16 +586,24 @@ class nsIWidget : public nsISupports {
update IID of nsIWidget

...
>+     * Make the non-modal window opened by modal window fake-modal, that will
>+     * call SetFakeModal(false) on destory on Cocoa.
destroy

>+     */
>+    NS_IMETHOD SetFakeModal(bool aModal) {
nit, { to its own line

C++/ObjC changes looks rather simple. r+
but mstange should review this too (I very rarely work on OSX specific issues).

Comment 22

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Olli Pettay [:smaug] from comment #21)
> >+/*
> >+ * Synthesize a native mouse click event at a particular point in screen.
> >+ * This function should be used only for testing native event loop.
> >+ * Use synthesizeMouse instead for most case.
> >+ *
> >+ * This works only on OS X.  Throws an error on other OS.  Also throws an error
> >+ * when the library or any of function are not found, or something goes wrong
> >+ * in native functions.
> >+ */
> >+function synthesizeNativeClick(x, y)
> Perhaps call this method then synthesizeNativeOSXClick
> and maybe even ifdef so that it is visible only on OSX

looks like there's no preprocessing variant for TEST_HARNESS_FILES.
I'd like to go with function-name-change only, for now.  is it okay?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

fine

Comment 24

[Markus Stange [:mstange]]

Comment on attachment 8687618 [details] [diff] [review]
Use SetFakeModal instead of SetModal for non-modal window opened by modal window.

Review of attachment 8687618 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, except for what Olli already mentioned. I really like the test!

Comment 25

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a106a807b7e0f78b5b934a12dc259c7d9d29f77f
Bug 1224790 - Use SetFakeModal instead of SetModal for non-modal window opened by modal window. r=smaug, mstange

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a106a807b7e0

Comment 27

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f74ef730ba520b0e8cdc8048b30a18989527df3a
Bug 1224790 followup - Make the test actually run. r=me

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f74ef730ba52