Closed Bug 1224790 Opened 9 years ago Closed 9 years ago

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

Categories

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

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: regression)

Attachments

(3 files)

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.
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.
Looks like it's mozilla-central's bug

will post a detail with testcase.
Component: Account Manager → DOM
Product: MailNews Core → Core
Summary: Mouse stops working after closing Feed Subscriptions dialog. → Mouse stops working after closing modal dialog that opened window.
Version: unspecified → Trunk
Attached file Extension with a testcase for Firefox (deleted) —
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.
the issue disappears by partially backing out bug 406375 patch.
Blocks: 406375
Comment on attachment 8687519 [details]
(testing) partially backout bug 406375 for testing.

this is not a fix, just a test.
Attachment #8687519 - Attachment description: partially backout bug 406375 for testing. → (testing) partially backout bug 406375 for testing.
Attachment #8687519 - Attachment is patch: false
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
This seems to be OSX specific issue.
Not reproducible on linux nor Windows.
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
(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 .
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.
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.
try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbfcf6e8bcfc

At lease it fixes the testcase and Thunderbird Feed Subscription dialog.
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.
As fyi, this was reported in Bug 956618, which was duped (perhaps erroneously) to Bug 476541, which precedes Bug 406375 landing though.
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....)
copy-n-pasted wrong bug number :P
I meant bug 476541 for testcases.
Fixing Bug 476541 qualifies as truly heroic!
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
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.
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.
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)
Assignee: nobody → arai.unmht
Attachment #8687618 - Flags: review?(bugs)
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).
Attachment #8687618 - Flags: review?(mstange)
Attachment #8687618 - Flags: review?(bugs)
Attachment #8687618 - Flags: review+
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 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!
Attachment #8687618 - Flags: review?(mstange) → review+
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
https://hg.mozilla.org/mozilla-central/rev/a106a807b7e0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Keywords: regression
Summary: Mouse stops working after closing modal dialog that opened window. → Mouse stops working after closing modal dialog that opened window. [Mac]
Depends on: 1247519
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: