757618 - [10.7] Lion fullscreen: Closing window while fullscreen leaves one on blank desktop for 10-15 seconds

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

STR for current trunk code on Lion:

1) Run Nightly and make it's window fullscreen (e.g. using Cmd+Shift+F).
2) Close the window while fullscreen (e.g. using Cmd+W).

You will see a grey-colored and entirely blank desktop for about 15 seconds.

The proximate cause of this problem is that the toplevel window doesn't actually get destroyed for about 15 seconds (when it's garbage collected).  Only then do you exit back to the "original" desktop.

One solution is to destroy the native window "early" (in nsCocoaWindow::Destroy()).  Later I'll post a patch that does this.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Thanks for tracking this down! It's been on my list for a while to figure out. Let me know if there's anything I can do to help.

Comment 3

[José Jeria]

Is there any chance that this issue will make it for Firefox 14? 

I just ran into this issue while testing the beta and it was indeed quite confusing when it occurred.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

I sincerely hope this will make it into FF 14, as it's a pretty bad bug.  Without a fix for this, we'll probably need to seriously consider backing out for the Lion-style fullscreen.

But I have a reasonable fix for this bug (and have had for a while).  I'll post it and ask for review as soon as I can (probably tomorrow).

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Stack trace of nsCocoaWindow being destroyed on garbage collection

> The proximate cause of this problem is that the toplevel window
> doesn't actually get destroyed for about 15 seconds (when it's
> garbage collected).

Here's a stack trace of this happening.

This only happens to nsCocoaWindow objects that correspond to
top-level windows (whose mWindow is a ToolbarWindow object).  It
happens every time you close a window (by whatever means, whether or
not you're in full-screen mode).  The delay is sometimes longer than
15 seconds.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's a patch that fixes this bug in my tests.  It follows my suggestion from comment #0.

Calling nsCocoaWindow::DestroyNativeWindow() "early" obliges us to null-check nsCocoaWindow::mWindow before it's used anywhere in an nsCocoaWindow object.  We already had some checks in the code.  And in principle it shouldn't hurt to put them everywhere.  But there are a few places where I had to make judgement calls (for example, what should GetTransparencyMode() or ClientToWindowSize() return if mWindow is null?).  I think I made them correctly.  But there's a small chance some of them may need to be revisited.

In any case, I think that's a small price to pay to fix a very serious bug.

Benoit, do you feel comfortable reviewing this?  If not, I'll ask someone else.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made from my patch in comment #6:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8d1a46b8a9a8/try-macosx64/firefox-16.0a1.en-US.mac.dmg

There were no non-spurious test failures.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Just wanted to chime in and say that I tested the patch and it appears to be working. Thanks for looking into this Steven! I completely agree with comment 4 that we should land this on beta if we can or turn off lion fullscreen before releasing.

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 633559 [details] [diff] [review]
Fix

I discussed this patch with Steven. We're not happy with this way of handling shutdown with fullscreen and I'm not too excited about all the null checks. But considering the difficulty in closing the fullscreen view at the right time I'm ok with this patch.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 633559 [details] [diff] [review]
Fix

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de7dcf3d03e3

Like Benoit, I don't much like the idea of destroying the native window "early".  But I wasn't able to find any other reasonable way to fix this bug.

I'm slowly learning more about how Apple implements its fullscreen mode on Lion and above (mostly by reverse-engineering).  So at some point I may find a better way to fix this bug (very possibly using undocumented methods).  In the meantime this is the best we can do.

Comment 11

[Ed Morley [:emorley]]

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

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 633559 [details] [diff] [review]
Fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New support for Lion fullscreen mode
User impact if declined: Many users will see a very serious UI bug
Testing completed (on m-c, etc.): As yet minimal baking on mozilla-central
Risk to taking this patch (and alternatives if risky): Low but non-zero risk
String or UUID changes made by this patch: none

This patch has just started baking on mozilla-central.  But we need this for Firefox 14, and I'd like to get it tested as widely as possible, starting as quickly as possible.  The aurora and beta branches have many more users than mozilla-central, so baking there is more likely to turn up any problems (though in any case this patch is low risk).

If we discover problems this patch can be backed out.  But if it's backed out we should probably also back out our support for the Lion fullscreen mode.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 633559 [details] [diff] [review]
Fix

[Triage Comment]
Let's land on all branches, since the alternative may be to back out the feature for 14.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 633559 [details] [diff] [review]
Fix

Landed on aurora and beta:
http://hg.mozilla.org/releases/mozilla-aurora/rev/bbccfcff3f0b
http://hg.mozilla.org/releases/mozilla-beta/rev/01ec444e6610

Comment 15

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20100101 Firefox/14.0

Verified on Mac OS 10.7 Lion in F14 beta10. There's a 1 second animation now when closing tab while in full screen mode.

Comment 16

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0

Verified on Firefox 15 beta1.