Last Comment Bug 757618 - [10.7] Lion fullscreen: Closing window while fullscreen leaves one on blank desktop for 10-15 seconds
: [10.7] Lion fullscreen: Closing window while fullscreen leaves one on blank d...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Steven Michaud [:smichaud] (Retired)
:
Mentors:
: 738995 (view as bug list)
Depends on:
Blocks: 639705
  Show dependency treegraph
 
Reported: 2012-05-22 14:35 PDT by Steven Michaud [:smichaud] (Retired)
Modified: 2012-07-19 07:18 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified


Attachments
Stack trace of nsCocoaWindow being destroyed on garbage collection (6.21 KB, text/plain)
2012-06-15 09:22 PDT, Steven Michaud [:smichaud] (Retired)
no flags Details
Fix (19.90 KB, patch)
2012-06-15 09:32 PDT, Steven Michaud [:smichaud] (Retired)
bgirard: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review

Description Steven Michaud [:smichaud] (Retired) 2012-05-22 14:35:31 PDT
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 reading much bugmail, email directly) 2012-05-22 14:45:50 PDT
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 2 Steven Michaud [:smichaud] (Retired) 2012-05-24 10:55:41 PDT
*** Bug 738995 has been marked as a duplicate of this bug. ***
Comment 3 José Jeria 2012-06-09 02:14:51 PDT
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) 2012-06-13 14:30:45 PDT
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) 2012-06-15 09:22:08 PDT
Created attachment 633555 [details]
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) 2012-06-15 09:32:56 PDT
Created attachment 633559 [details] [diff] [review]
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) 2012-06-15 09:35:38 PDT
Here's a tryserver build made from my patch in comment #6:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-8d1a46b8a9a8/try-macosx64/firefox-16.0a1.en-US.mac.dmg

There were no non-spurious test failures.
Comment 8 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-06-16 09:50:44 PDT
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) 2012-06-18 07:48:57 PDT
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) 2012-06-18 08:25:56 PDT
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] 2012-06-19 01:20:44 PDT
https://hg.mozilla.org/mozilla-central/rev/de7dcf3d03e3
Comment 12 Steven Michaud [:smichaud] (Retired) 2012-06-19 13:00:36 PDT
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] 2012-06-19 20:08:22 PDT
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 15 Virgil Dicu [:virgil] [QA] 2012-07-03 05:33:42 PDT
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] 2012-07-19 07:18:50 PDT
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0

Verified on Firefox 15 beta1.

Note You need to log in before you can comment on or make changes to this bug.