Closed
Bug 757618
Opened 13 years ago
Closed 12 years ago
[10.7] Lion fullscreen: Closing window while fullscreen leaves one on blank desktop for 10-15 seconds
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: smichaud, Assigned: smichaud)
References
Details
Attachments
(2 files)
6.21 KB,
text/plain
|
Details | |
19.90 KB,
patch
|
BenWa
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → smichaud
Comment 1•13 years ago
|
||
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•13 years ago
|
||
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.
Assignee | ||
Comment 4•12 years ago
|
||
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).
Assignee | ||
Comment 5•12 years ago
|
||
> 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.
Assignee | ||
Comment 6•12 years ago
|
||
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)
Assignee | ||
Comment 7•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Assignee | ||
Comment 12•12 years ago
|
||
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.
Attachment #633559 -
Flags: approval-mozilla-beta?
Attachment #633559 -
Flags: approval-mozilla-aurora?
Comment 13•12 years ago
|
||
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.
Attachment #633559 -
Flags: approval-mozilla-beta?
Attachment #633559 -
Flags: approval-mozilla-beta+
Attachment #633559 -
Flags: approval-mozilla-aurora?
Attachment #633559 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 14•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Comment 15•12 years ago
|
||
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•12 years ago
|
||
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.
Description
•