Closed Bug 757618 Opened 11 years ago Closed 11 years ago
.7] Lion fullscreen: Closing window while fullscreen leaves one on blank desktop for 10-15 seconds
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.
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.
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.
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).
> 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.
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.
Attachment #633559 - Flags: review?(bgirard)
Here's a tryserver build made from my patch in comment #6: http://email@example.com/try-macosx64/firefox-16.0a1.en-US.mac.dmg There were no non-spurious test failures.
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 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.
Attachment #633559 - Flags: review?(bgirard) → review+
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
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 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 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
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.
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0 Verified on Firefox 15 beta1.
You need to log in before you can comment on or make changes to this bug.