436063 - Invisible sheets accessible from Dock's window menu

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Michal 'hramrach' Suchanek]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9) Gecko/2008051202 Firefox/3.0

When there are multiple dialogs they are sometimes ordered incorrectly, and the application seemingly locks up.

Reproducible: Sometimes

Steps to Reproduce:
1. open multiple sites that ask to set a cookie or supply basic auth credentials
2. close firefox
3. restart firefox
Actual Results:  
When the session loads sometimes multiple dialogs are displayed and their controls are unusable

Expected Results:  
the dialogs should work normally

The problem appears only when multiple dialogs are displayed.
The dialogs are modal in some way, and I suspect that while the first dialog is displayed only the last one is functional.

The application menus do not work. However, it is possible to use the dock icon menu to switch to different cookie query windows. These appear as windows without title, and when they are activated this way they are detached from their parent and appear as standalone dialogs. In all but one only the checkbox and the "show details" option work, the confirm or reject buttons do not. They are not grayed, just do nothing. The working dialog is not always the one that is actually displayed.

The issue with basic auth dialog is not as severe. The dialog sometimes loses focus and it is impossible to type in it. However, the focus can be restored to (some) auth dialog by switching to another application and back to Firefox (this does not work for the cookie dialogs, though). I am not sure multiple dialogs are required for this as I never needed to resort to window switching voodoo to fix this one. This one can be also seen on Linux.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

> The problem appears only when multiple dialogs are displayed.

I can't get this to happen (at least with the basic auth dialog, which
so far is the only one I've tested with).

I opened two different sites that require basic auth (in two tabs in
the same window), then quit and selected "Save and quit" (in the "Quit
Firefox" dialog).  When I restarted Firefox both tabs re-appeared, I
only ever saw one basic auth dialog.  I got the same results opening
these sites in two different windows.  The (single) basic auth dialog
was for the tab (or window) that was currently focused.

I tested with Firefox 3 RC 1 on OS X 10.4.11.

Comment 2

[Markus Stange [:mstange]]

I couldn't reproduce it with Michal's STR, but I've found a different way to trigger the bug:

1. Go to http://office.kis.uni-kl.de/
2. Click "Or you can add an exception...", "Add Exception..." and "Get Certificate".
3. Click "View".
4. Choose the "Add Security Exception" window from the Dock menu. (The hidden sheet is now a window.)
5. Click "View" another time.
6. Close the "Certificate Viewer" sheet.
7. Close it again.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Thanks, Markus.

As best I can tell the real problem is that you can access an
invisible sheet from the Dock's window menu.  So (I think) the fix
should be to stop that from happening.

Comment 4

[Tom Dyas]

I haven't tested Firefox 3.6.2 or Trunk to see if this bug still exists.  Having delved into the dock menu code recently, however, the relevant code should be in toolkit/xre/MacMacApplicationDelegate.mm in the -applicationDockMenu method.  The core has to build the windows list manually for some reason by enumerating the windows via the nsIWindowMediator service. I would think it is straightforward to exclude invisible sheets during that enumeration.

Comment 5

[Markus Stange [:mstange]]

Attachment: block invisible windows and sheet parents

Thanks Tom, that worked.

Comment 6

[Josh Aas]

Comment on attachment 439012 [details] [diff] [review]
block invisible windows and sheet parents

+    if (!cocoaWindow || ![cocoaWindow isVisible] || [cocoaWindow attachedSheet])

Does "isVisible" return true or false for a regular dock-minimized window? If minimized windows are not considered visible then I think this is wrong.

Comment 7

[Markus Stange [:mstange]]

Attachment: v2

Your suspicion was correct; minimized windows return false for isVisible.

But there's a slightly simpler way to fix the bug.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 439108 [details] [diff] [review]
v2

Hm, I probably need to look at toolkit/content/macWindowMenu.js, too.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 439108 [details] [diff] [review]
v2

Ah, no, that's not for the Dock menu, but for the Window menu in the menu bar, which I'm not touching here.

Comment 10

[Josh Aas]

Comment on attachment 439108 [details] [diff] [review]
v2

+  NSApplication* app = [NSApplication sharedApplication];

Just use "NSApp".

+- (id)initWithApplication:(NSApplication*)aApp;

Why pass this around, why not just use NSApp wherever we need it.

This doesn't apply on trunk any more, sorry it took me a bit to get to the review. I'm still testing the patch.

Comment 11

[Markus Stange [:mstange]]

Yeah that sounds better.

Comment 12

[Josh Aas]

Comment on attachment 439108 [details] [diff] [review]
v2

I was concerned about how this would interact with our native menu system but I haven't been able to find any issues so if you can fix the stuff I commented about before then I'll r+. Thanks.

Comment 13

[Markus Stange [:mstange]]

Attachment: v3

Comment 14

[Markus Stange [:mstange]]

Attachment: v3

There was a mistake in the last patch.

Comment 15

[Markus Stange [:mstange]]

http://hg.mozilla.org/mozilla-central/rev/d5da063cf883