The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 14

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smichaud, Assigned: smichaud)

Tracking

Trunk
mozilla14
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox14 verified, firefox15 verified)

Details

Attachments

(2 attachments)

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

5 years ago
Assignee: nobody → smichaud
(Assignee)

Updated

5 years ago
Blocks: 639705
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.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 738995

Comment 3

5 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

5 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

5 years ago
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.
(Assignee)

Comment 6

5 years ago
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.
Attachment #633559 - Flags: review?(bgirard)
(Assignee)

Comment 7

5 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.
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.
https://hg.mozilla.org/mozilla-central/rev/de7dcf3d03e3
Status: NEW → RESOLVED
Last Resolved: 5 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.
Attachment #633559 - Flags: approval-mozilla-beta?
Attachment #633559 - Flags: approval-mozilla-aurora?
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+
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

5 years ago
status-firefox14: --- → fixed
status-firefox15: --- → fixed
Target Milestone: mozilla16 → mozilla14
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.
status-firefox14: fixed → verified
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20100101 Firefox/15.0

Verified on Firefox 15 beta1.
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.