Issue: Firefox crashes on entering fullscreen mode under some circumstances on OS X. Affects (at least) firefox 15 and firefox 16 release versions. A second monitor seems to be required to reproduce this issue. Steps to reproduce: 1) start a new browser session on your second monitor (i.e. the one without the menu bar) 2) start a nice game of banana bread, e.g. https://developer.mozilla.org/media/uploads/demos/a/z/azakai/3baf4ad7e600cbda06ec46efec5ec3b8/bananabread_1348775105_demo_package/game.html?low,low 3) When prompted, choose either low or high resolution 4) Click 'allow' when prompted to allow fullscreen mode. 5) observe crash.

Comment on attachment 669560 [details] Backtrace of crash from gdb session relates to a debug build of FIREFOX_16_0_RELEASE

Looks like the crash is caused by running out of stack space due to a self-feeding loop of mousemove events, with the cycle: ... #7311 nsViewManager::DispatchEvent (this=0x1098c35c0, aEvent=0x7fff5f6bca98, #7312 HandleEvent #7313 nsChildView::DispatchEvent #7314 nsChildView::DispatchWindowEvent #7315 -[ChildView handleMouseMoved:] #7316 ChildViewMouseTracker::MouseMoved #7317 -[BaseWindow mouseMoved:] #7318 nsChildView::SynthesizeNativeMouseEvent #7319 nsChildView::SynthesizeNativeMouseMove #7320 nsEventStateManager::GenerateMouseEnterExit #7321 nsEventStateManager::PreHandleEvent #7322 PresShell::HandleEventInternal #7323 PresShell::HandlePositionedEvent #7324 PresShell::HandleEvent #7325 nsViewManager::DispatchEvent (this=0x1098c35c0, aEvent=0x7fff5f6bdfe8, ...

Possibly related to bug 788189, particularly to the underlying problem that hasn't yet been fixed. See bug 788189 comment #29 and bug 788189 comment #30.

I can't reproduce this (testing with FF 16.0.1 on OS X 10.7.5). > 1) start a new browser session on your second monitor (i.e. the one without the > menu bar) What does this mean? Do you need to move the (single) browser window to the second monitor, quit Firefox and then restart is? (This is what I've been doing.) > 3) When prompted, choose either low or high resolution At this step, the browser window switches from the second monitor to the first one, and appears to go fullscreen even before I've "allowed" it (in step 4).

Mark, can you clarify the steps to reproduce this crash? (see comment 4). Thanks.

Steps to reproduce for me on today's (2013-02-20) nightly on OSX: 1. Go to http://crypt-webgl.unigine.com/game.html 2. Wait for game to load 3. Choose fullscreen 4. Once we've entered fullscreen, click the "allow" dialog. Don't check the "remember checkbox". Crash report: https://crash-stats.mozilla.com/report/index/bp-63b2a574-0c4f-4cc4-b3f6-13e7c2130221

I'm currently only using a single monitor, i.e. the built-in one on my MacBook Pro. But I do on occasion use an external monitor as well, not sure if that matters.

Bingo. Thanks, Jonas! Your STR only works for me on a Retina MBP, and then only if HiDPI mode is enabled (as it is by default if no external, non-Retina monitors are connected). I tested with a recent mozilla-central nightly on OS X 10.7.5. I'll post a gdb stack trace shortly (the Breakpad stacks I've gotten are useless).

This trace's basic pattern matches Mats' report and analysis from comment #2. This looks like an infinite recursion in Cocoa widgets code, leading to stack exhaustion.

Here's the regression range for this bug: firefox-2012-09-29-03-06-24-mozilla-central firefox-2012-09-30-03-06-10-mozilla-central http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c09a0c022b2e&tochange=a680fd777c3b Which isn't much use, since this is when our HiDPI support (the patches for bug 674373) first landed. I've no idea what the connection is to HiDPI mode. I suspect it's just an accident.

> This looks like an infinite recursion in Cocoa widgets code, leading > to stack exhaustion. Note that this means these crashes will have a huge variety of signatures, and that it will be very difficult to track them by signature.

(In reply to Steven Michaud from comment #11) > Here's the regression range for this bug: > > firefox-2012-09-29-03-06-24-mozilla-central > firefox-2012-09-30-03-06-10-mozilla-central > > http://hg.mozilla.org/mozilla-central/ > pushloghtml?fromchange=c09a0c022b2e&tochange=a680fd777c3b > > Which isn't much use, since this is when our HiDPI support (the patches for > bug 674373) first landed. > > I've no idea what the connection is to HiDPI mode. I suspect it's just an > accident. Probably so, especially given that comment 0 says it affected (at least) Firefox 15 and 16, which predated any HiDPI support (first introducted in FF18). It may be that the larger device-pixel coordinates, surfaces, etc., involved in HiDPI mode make it more likely to happen, though.

I get a crash with this signature: https://crash-stats.mozilla.com/report/index/bp-b696fbbd-6807-4afc-910c-0caf22130228 every time I try to enable fullscreen on the low-res first level of Banana Bread: https://developer.mozilla.org/en/demos/detail/bananabread https://developer.cdn.mozilla.net/media/uploads/demos/a/z/azakai/3baf4ad7e600cbda06ec46efec5ec3b8/bananabread_1348775105_demo_package/game.html?low,low /be

So with a long-reproducible testcase, and my rMBP my only BB demo device, I need this bug fixed pronto. What's the prognosis? /be

Brendan, as a temporary workaround for this bug you can try setting "gfx.hidpi.enabled" to false.

> with a long-reproducible testcase No. This bug has only truly been reproducible since Jonas posted his STR in comment #8. I'm currently working on two complex, difficult bugs that clearly are more urgent than this one (bug 837539 and bug 816976). But given that this bug is an easily reproducible infinite recursion, it shouldn't be too hard to fix. If the circumstances don't change drastically between now and then, I should be able to get to this next week. That is unless someone else beats me to it :-)

(In reply to Steven Michaud from comment #17) > > with a long-reproducible testcase > > No. This bug has only truly been reproducible since Jonas posted his STR in > comment #8. What I meant is that Banana Bread going fullscreen on my rMBP has crashed for months. I remember finding bug 837159 which was dup'ed against this bug in comment 6. > I'm currently working on two complex, difficult bugs that clearly are more > urgent than this one (bug 837539 and bug 816976). Understood, but demos matter too, apparently moreso when I give them. Anyone able to help you? > But given that this bug is an easily reproducible infinite recursion, it > shouldn't be too hard to fix. If the circumstances don't change drastically > between now and then, I should be able to get to this next week. > > That is unless someone else beats me to it :-) Any fix advice? Thanks, /be

I heard from a fellow MWC'er, Fabian, that he saw this crash. I think multiple BB demo'ers with rMBPs have been suffering in silence. /be

> What I meant is that Banana Bread going fullscreen on my rMBP has > crashed for months. Did you open a bug? Did you track down this bug and give clear STR (which have been sorely lacking since this bug was opened, before Jonas posted his)? Us developers can generally only fix a bug once we know how to reproduce it :-) > Any fix advice? No more than I've already given.

> Anyone able to help you? Now that (very thankfully) we've got a couple more full-time Mac devs, there are some possibilities. I know the code well, and my gut tells me that I shouldn't need more than a day to fix it ... once I have that day. I'll talk to Robert Strong about this.

By the way, Brendan, please let us know if setting "gfx.hidpi.enabled" to '0' or '-1' makes this bug go away for you.

Would have been good to get this in before MWC ends but that ship has sailed. Steven will look at this next week so he doesn't have to context switch from bug 837539 and bug 816976. If there is an urgent need to get this fixed this sooner please let me know and I will get someone else to work on this but it will likely take longer for them to come up with a fix. Hopefully changing the gfx.hidpi.enabled pref is sufficient until then.

I can confirm that by turning off HiDPI in about:config, the crash no longer occurs. I had a quick look at this and the crash occurs due to stack exhaustion (as identified in previous comments). When clicking on the "Allow" button, we reposition the cursor to the center of the screen and keep track of it. Unfortunately, the center point is tracked in non-HiDPI pixels (I'm sure there's a better term for this). On subsequent calls, the refPoint of the event would never match the tracked center point, because we would compare a HiDPI point (the event's refPoint) with a non-HiDPI point (the tracked center point). We would proceed to centering the cursor again etc. This patch may not be the right fix (we might want to fix up GetWindowInnerRectCenter() instead). But hopefully, it will illustrate the problem sufficiently for a proper fix. I started a try build with this patch applied. It should appear here once it's done: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/spohl@mozilla.com-74abbc11256e

Please note that my wip only avoids the crash reported in this bug. Unfortunately, it doesn't fix the fact that Banana Bread is unplayable in HiDPI mode. When I move the mouse, the player keeps staring at the ground. There must be another HiDPI bug here, maybe one that's already filed?

Could you open a new issue for that bug? I think you're the first to discover it.

I can confirm the problem with centering the mouse pointer (comment 24) - I tracked down the same issue this evening, and was aiming to fix it by fixing the confusion between display-pixel and device-pixel coords in GetWindowInnerRectCenter. The other problem I've found is that nsChildView::SynthesizeNativeMouseEvent doesn't convert from device to Cocoa pixels before calling Cocoa methods on the view. With these two issues solved, BananaBread seems to run OK on the Retina screen (though I see fairly frequent freezes during the "Starting..." stage, but I think that's unrelated). I'll attach a WIP patch of what I have so far - this is *not* ready for potential review, as it includes an ugly hack to work around what looks like a rounding error in sizing a full-screen window (it appears to be one pixel smaller than expected). But I'm out of time to dig in deeper for tonight.

Tryserver build with my WIP is in progress at https://tbpl.mozilla.org/?tree=Try&rev=4436ed91b1c3, in case anyone wants to try it out.

Comment on attachment 719708 [details] [diff] [review] Wip Retired in favor of jfkthame's approach.

(In reply to Stephen Pohl [:spohl] from comment #24) > Unfortunately, the center point is tracked in non-HiDPI pixels (I'm sure > there's a better term for this). On subsequent calls, the refPoint of the > event would never match the tracked center point, because we would compare a > HiDPI point (the event's refPoint) with a non-HiDPI point (the tracked > center point). We would proceed to centering the cursor again etc. This bug originally manifest itself on a non-retina MBP; could there be other situations where there is refPoint confusion?

Yes, the fact that it occurred on non-retina Macs (and that it occurred back in FF15/16, when there was no HiDPI support) implies that there must be some other scenario that can lead to a similar problem. It may be related to the apparent one-pixel sizing discrepancy in the full-screen window that I ran into with attachment 719738 [details] [diff] [review]. I hacked around it there with an ugly kludge "if (pt.y == 901) pt.y = 900;" to avoid the crash on my retina machine, but obviously that's not an acceptable solution.

So there are several issues that combine to cause the bug here. The first is that GetWindowInnerRectCenter, used when 'locking' the mouse pointer, is incorrect when CSS pixels and device pixels differ. This should fix that problem, by converting innerWidth/Height values to device pixels before the final calculation.

Second problem is that nsChildView::SynthesizeNativeMouseEvent is passed a mouse position in device-pixel coords; it needs to convert this to Cocoa display pixels before calling native Cocoa methods.

And the third problem is that when the window goes into full-screen, we appear to call SetFullScreenInternal on two different nsGlobalWindows. One of these seems to correspond to the "outer" browser XUL window, as I can see the URL bar, search box, etc in its frame tree. The root frame of this window ends up with a rect that exactly matches the screen (great!). So GetInnerScreenRect for this nsGlobalWindow returns {0,0, 86400,54000} (in appUnits) on my screen that is 1440x900 CSS pixels. However, we also call SetFullScreenInternal on another nsGlobalWindow, which I think must correspond to the content of the tab that's being full-screened. Its frame tree starts with a viewport that contains an HTMLScroll element, with the various elements of the BananaBread page within that. But the odd thing is that in this "window", the root frame ends up with a rect that is exactly the -width- of the screen, but is one CSS pixel shorter in -height-. So GetInnerScreenRect returns {0,60, 86400,53940}. This 60-appunit (or 1 CSS px) discrepancy leads to a mismatch in the "center" that we compute for those two windows. On a lo-dpi screen, the discrepancy is 30 appunits in the y-direction, but when that's converted to (lo-dpi) device pixels, the half-pixel is truncated and so the problem vanishes. However, on a hi-dpi screen, 30 appunits is a whole device pixel, and so we get a one-devpix difference in the centers. I haven't pinpointed the cause of this yet, but I have confirmed that it happens in both lo- and hi-dpi modes on OS X, and also on Windows. So it's not specifically a Mac/hi-dpi bug, but because the discrepancy disappears when converting the center position to integer device pixels at low resolution, the error is masked there. The kludge mentioned in my original WIP patch simply hacked around this by "snapping" a y-coordinate of 901 device pixels back to 900, and this lets BananaBread run successfully on a Retina screen. But obviously it was a total hack and only "solves" the problem for one particular screen size/resolution.

I see the 1px height discrepancy is already filed as bug 729011. (Possibly related: bug 804627.) AFAICT, with the two patches here applied, that is the remaining issue that leads to BananaBread failure on Retina screens.

Bug 808136 has a testcase (attachment 678438 [details]) that shows the height discrepancy: when fullscreened on a Retina display, it reports a final size of 1440x899 (in CSS pixels), not the expected 1440x900.

This doesn't -fix- the issue of the fullscreen window being incorrectly sized, but it works around the resulting crash. By calculating the center in integer CSS pixels, the half-px discrepancy that results from the one-px window size error gets lost in integer truncation, instead of becoming a 1-devpx discrepancy. So this patch essentially overrides pt1 above, 'fixing' the use of mismatched units in the opposite direction. Once the underlying problem in bug 729011 is fixed, we could revert this part (though I doubt the slight loss of precision really matters), but for now I think it's an acceptable wallpaper.

Tryserver build in progress with the three patches here: https://tbpl.mozilla.org/?tree=Try&rev=a92469909493

Sigh...tryserver didn't like the typecast there, even though it compiled fine for me locally. (Different compiler versions? Different SDKs?) Trying again with an explicit static_cast<> that ought to make it happier. https://tbpl.mozilla.org/?tree=Try&rev=857d7ed5ce2a.

Comment on attachment 721450 [details] [diff] [review] pt 1 - avoid mixing CSS-pix and device-pix coordinates in GetWindowInnerRectCenter >+ // (so we can legitimately combine them innerX and innerY from above, and "combine them with". r=me

Sorry for the bugspam - this version should finally work, I hope! https://tbpl.mozilla.org/?tree=Try&rev=930e8aac7a04

Comment on attachment 721650 [details] [diff] [review] pt 3 - work around fullscreen window-size error (bug 729011) by calculating center in integer CSS pixels instead of device pixels r=me

Comment on attachment 721702 [details] [diff] [review] pt 2 - convert device pixels to Cocoa display pixels when synthesizing native mouse event Looks fine to me.

https://hg.mozilla.org/integration/mozilla-inbound/rev/99324fe97873 https://hg.mozilla.org/integration/mozilla-inbound/rev/fe3312317288 https://hg.mozilla.org/integration/mozilla-inbound/rev/d5e45282bdcd

It's not clear to me whether there may still be a residual problem here, given that the original reports predated any HiDPI support in Gecko, but the patches here specifically address HiDPI issues. However, what is clear is that it was completely broken on Retina Macs due to a couple of "missing pieces" in the HiDPI support. As such, I think we should consider this for uplift once the patches are safely on mozilla-central. Nominating for tracking-20/21; I'll also post a rollup patch for the aurora and beta trees.

Rollup of the three patches here, for aurora/beta consideration. Carrying over r=bz,smichaud from the individual patches

https://hg.mozilla.org/mozilla-central/rev/99324fe97873 https://hg.mozilla.org/mozilla-central/rev/fe3312317288 https://hg.mozilla.org/mozilla-central/rev/d5e45282bdcd

Comment on attachment 721855 [details] [diff] [review] rollup of parts 1-3 - fix for Retina MacBook crash when entering fullscreen mode (e.g. in BananaBread). [Approval Request Comment] Bug caused by (feature/regressing bug #): fullscreen + hidpi + mouse pointer locked (e.g. for gaming UIs) User impact if declined: BananaBread and similar games (or other kinds of web page that use fullscreen mode and "lock" the mouse pointer) will crash when launched on a Retina screen Testing completed (on m-c, etc.): Tested locally on Retina MacBook, now landed on m-c Risk to taking this patch (and alternatives if risky): Minimal risk - the code changes are small and simple, and the existing code is clearly incorrect for hidpi mode in these places; we're just getting away with it because most pages don't hit those codepaths. I don't see how there can currently be any -working- use of those functions that would be broken by these changes. String or UUID changes made by this patch: None

Comment on attachment 721855 [details] [diff] [review] rollup of parts 1-3 - fix for Retina MacBook crash when entering fullscreen mode (e.g. in BananaBread). Already approving on aurora and requesting QA verification .

Pushed to aurora, marking fixed for firefox-21: https://hg.mozilla.org/releases/mozilla-aurora/rev/2c8a79a534ea Adding qawanted keyword, to draw attention to the request for verification.

Comment on attachment 721855 [details] [diff] [review] rollup of parts 1-3 - fix for Retina MacBook crash when entering fullscreen mode (e.g. in BananaBread). Since Firefox 20 is the first release where we plan to support multi-monitor HiDPI (see bug 814434), and this patch is considered low risk, approving for Beta.

Still crashes on this week's release build of FF20. Does not appear to crash on 21 or 22. Let me know how I can be of more help.

(In reply to Matt Wobensmith from comment #54) > Still crashes on this week's release build of FF20. That's unexpected. :( I wonder if it's exactly the same crash, or something a little different... could you provide a stack and/or crash report ID?

For starters, this: https://crash-stats.mozilla.com/report/index/bp-4c124fe0-75c0-4dc4-8466-9caa52130404 I'm happy to run other tools on this config. I could even cook up an ASan build so that we might know more about the danger of the crash itself. Let me know.

Not much of a stack in that report :( ... if you run it under gdb, can you get more of a backtrace when it crashes? The original crash was triggered by infinite recursion, I believe - it'd be nice to determine whether that's the case for yours as well.

Could you give full details of your exact configuration, please? I just tried Firefox 20 (about:buildconfig says "Built from http://hg.mozilla.org/releases/mozilla-release/rev/c90d44bfa96c") here, and I'm not seeing the crash with either http://crypt-webgl.unigine.com/game.html or BananaBread demo, either on the internal Retina screen or an external (non-hidpi) display.

I'm using the same build of 20 that you are, although I built it as a local ASan build, to see if I could get a better stack. For whatever reason, this crash does not produce that, only the OS-level one. One thing I did see in the terminal was this: WARNING: shutting down early because of crash!: file /Users/mwobensmith/asan_current_build/dom/plugins/ipc/PluginModuleChild.cpp, line 701 WARNING: plugin process _exit()ing: file /Users/mwobensmith/asan_current_build/dom/plugins/ipc/PluginModuleChild.cpp, line 666 In any case, I'm happy to give you full details of my config; just tell me what details you need most. Thanks.

(In reply to Matt Wobensmith from comment #60) > In any case, I'm happy to give you full details of my config; just tell me > what details you need most. Thanks. Adding need-info to Jonathan Kew.

(In reply to Matt Wobensmith from comment #60) > I'm using the same build of 20 that you are, although I built it as a local > ASan build, to see if I could get a better stack. For whatever reason, this > crash does not produce that, only the OS-level one. Your crash report (comment 59) still shows stack overflow apparently due to infinite recursion, presumably triggered by the use of the pointer-lock API generating a synthetic mouse-move event. So that looks very much like the original crash reports; but I haven't been able to reproduce it on my Retina MacBook here, which seems odd. > In any case, I'm happy to give you full details of my config; just tell me > what details you need most. Thanks. Are you seeing this on a Retina MacBook with just the built-in screen, or do you have an external display as well? If so, what resolution, and which is the primary display (with menu bar)? Oh, and what resolution do you have selected in System Preferences / Display for the Retina display? I normally run mine at "Best for Retina Display"; if you're using a different scaling, that could be a factor.

This doesn't appear to happen when just using the built-in screen on the Retina MacBook. It only appears to happen when playing the game on the 2nd monitor, as per original instructions. The Display system preferences are set for "Best for Retina Display". More data from system profiler: Displays: Color LCD: Display Type: LCD Resolution: 2880 X 1800 Retina: Yes Pixel Depth: 32-Bit Color (ARGB8888) Main Display: Yes Mirror: Off Online: Yes Built-In: Yes Connection Type: DisplayPort DELL U2410: Resolution: 1920 x 1200 @ 60 Hz Pixel Depth: 32-Bit Color (ARGB8888) Display Serial Number: C592M1848E4L Mirror: Off Online: Yes Rotation: Supported Connection Type: DisplayPort Television: Yes

I've got a Retina MacBook Pro and a 23" Cinema Display (1920x1200) here, so that should be pretty much equivalent, but I'm still not able to persuade it to crash, regardless of which display I run it on. How are your displays arranged (in the Displays pref panel, Arrangement tab) - side by side (which way? aligned at the top or bottom?), above/below (aligned left or right?), and which is the primary display with the system menu bar? (I guess that must be the external monitor, as it seems the fullscreen game always appears on the primary display, regardless of where the browser window was). Where is your browser window from which you're starting the game, and exactly which of the demos are you trying? There's clearly still a problem of some kind in FF20 (which worries me, as it might also be present in current code if the circumstances are just right), but it's proving elusive here - maybe if I can duplicate more precisely what you're doing, I can get it to happen.

Working from a different office today, so no access to my external monitor. Will get back to you Thursday morning with more information, when I'm able to do so.

Displays are arranged in default order - main Retina screen as primary display on left, external monitor on the right. I will move the browser to the external monitor, which at that point has no system menu bar. Upon launching the game, I'll be asked to choose low/high resolution. Making that choice causes the Retina display to clear its screen, and the full screen dialog + system file menu to appear on the external monitor, waiting for input. Clicking OK on the full screen dialog (on external monitor) causes a crash, and also restores the screen and its contents to the default Retina screen. As far as which demo, I can see this on all of the Banana Bread demos that I can load. Some of them intermittently don't load completely at various times, causing me to have to switch from demo to demo, but try the first or second ones on that page and you should see it. Let me know what else you need.

I'm going to assume based on lack of response to comment 66 that we are all good here. Please re-add the verifyme keyword if there is more testing needed.