1. Load testcase 2. Click button to enter full screen mode 3. Quit Firefox with Cmd+Q Result: leak nsGlobalWindow, nsDocument, etc.

Is this a regression?

Although this is just a shutdown bug, this might block us from finding other leaks in fullscreen.

How can I see the leak info on Mac?

By setting XPCOM_MEM_LEAK_LOG=2 and running a debug build. If you want to have exactly the same setup as me: mkdir -p ~/px/z curl https://raw.githubusercontent.com/MozillaSecurity/funfuzz/master/dom/automation/constant-prefs.js > ~/px/z/prefs.js XPCOM_MEM_LEAK_LOG=2 firefox -profile ~/px/z/ ~/Desktop/q.html

So this also seems to be a regression of bug 1160014. The leak happens because we somehow starts the fullscreen transition, and the transition task object holds a reference to various objects including the nsDocument and nsGlobalWindow. This is indeed a shutdown bug because this won't really leak in normal cases. That object will eventually be released after it finishes.

I have found a solution, but probably need to do some cleanup first.

Setting window fullscreen in ExitFullscreenInDocTree takes effect only when we don't have chance to perform the transition in advance. In that case, the window content changes immediately anyway, so it doesn't make sense to perform transition anymore.

We will still detect this leak anyway, if we manually exit the browser during the transition. These patches just avoid performing the transition if we are exiting the browser, so that the related object won't be created in that case, and thus won't leak.

I'll need that case fixed too in order to fuzz-test the feature properly. Or at least put in something so my tools can distinguish that case from new unknown leak bugs, like: #ifdef NS_FREE_PERMANENT_DATA fprintf(stderr, "Quitting during full-screen transition; intentionally leaking\n"); #endif

(In reply to Jesse Ruderman from comment #11) > I'll need that case fixed too in order to fuzz-test the feature properly. Or > at least put in something so my tools can distinguish that case from new > unknown leak bugs, like: > > #ifdef NS_FREE_PERMANENT_DATA > fprintf(stderr, "Quitting during full-screen transition; intentionally > leaking\n"); > #endif You can set the pref "full-screen-api.transition-duration.enter" and "full-screen-api.transition-duration.leave" to "0 0" to completely suppress the fullscreen transition in your fuzz-test.

I could, but then I wouldn't be testing what happens when a page does weird things during a transition!

This patch independently fixes this bug. With this patch (and patch 2 & 3 of bug 1191112), the fulscreen transition objects shouldn't leak anymore on OS X. (Windows and Linux may still need fix) Although this is patch independently fixes this bug, the three patches before are still useful.

Comment on attachment 8643674 [details] [diff] [review] patch 4 - stop transtion animation when cocoa window gets destroyed Looks reasonable to me.

Comment on attachment 8643516 [details] [diff] [review] patch 1 - remove SetWindowFullScreen function Please add MOZ_ASSERT(nsContentUtils::IsSafeToRunScript()); to SetFullscreenInternal >@@ -11628,17 +11601,18 @@ nsDocument::RequestFullScreen(UniquePtr< > // If we are not the top level process, dispatch an event to make > // our parent process go fullscreen first. > nsContentUtils::DispatchEventOnlyToChrome( > this, ToSupports(this), NS_LITERAL_STRING("MozDOMFullscreen:Request"), > /* Bubbles */ true, /* Cancelable */ false, /* DefaultAction */ nullptr); > } else { > // Make the window fullscreen. > FullscreenRequest* lastRequest = sPendingFullscreenRequests.getLast(); >- SetWindowFullScreen(this, true, lastRequest->mVRHMDDevice); >+ rootWin->SetFullscreenInternal(nsPIDOMWindow::eForFullscreenAPI, true, >+ lastRequest->mVRHMDDevice); I don't understand this change. Before this patch you end up calling GetWindow()->SetFullscreenInternal(... But with the patch you go up to the chrome window in non-e10s or top level content window in e10s can call SetFullscreenInternal on that Window object. Please explain or fix

Comment on attachment 8643517 [details] [diff] [review] patch 2 - ensure script safe in ExitFullscreenInDocTree /me hates Move(). It doesn't move anything and requires reviewer to check whether the relevant code can actually deal with rvalues in a useful way. But fine.

(In reply to Olli Pettay [:smaug] from comment #16) > Comment on attachment 8643516 [details] [diff] [review] > patch 1 - remove SetWindowFullScreen function > > Please add MOZ_ASSERT(nsContentUtils::IsSafeToRunScript()); to > SetFullscreenInternal This is probably unsafe to add it in this patch. May I do it in patch 2 instead? > >@@ -11628,17 +11601,18 @@ nsDocument::RequestFullScreen(UniquePtr< > > // If we are not the top level process, dispatch an event to make > > // our parent process go fullscreen first. > > nsContentUtils::DispatchEventOnlyToChrome( > > this, ToSupports(this), NS_LITERAL_STRING("MozDOMFullscreen:Request"), > > /* Bubbles */ true, /* Cancelable */ false, /* DefaultAction */ nullptr); > > } else { > > // Make the window fullscreen. > > FullscreenRequest* lastRequest = sPendingFullscreenRequests.getLast(); > >- SetWindowFullScreen(this, true, lastRequest->mVRHMDDevice); > >+ rootWin->SetFullscreenInternal(nsPIDOMWindow::eForFullscreenAPI, true, > >+ lastRequest->mVRHMDDevice); > I don't understand this change. Before this patch you end up calling > GetWindow()->SetFullscreenInternal(... > But with the patch you go up to the chrome window in non-e10s or top level > content window in e10s can call > SetFullscreenInternal on that Window object. > Please explain or fix This branch only works for non-content process, so it always goes up to the chrome window. Fullscreen change doesn't make sense to non-chrome window. We redirect this call to the root window anyway in SetFullscreenInternal [1], hence this change effectively doesn't change any behavior. And I don't think the code here on its own needs further in-code description. It's only worth an explanation on the change as it is not a direct unfolding. Given this, could you re-review this patch? [1] https://dxr.mozilla.org/mozilla-central/rev/5cf4d2f7f2f2b3df2f1edd31b8bdce7882f3875c/dom/base/nsGlobalWindow.cpp#6486-6495

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #18) > This is probably unsafe to add it in this patch. May I do it in patch 2 > instead? sure, fine. > Given this, could you re-review this patch? Thanks for the explanation. reviewing...

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d09363a1516e74bcab7514643f8b5c77b91e65a changeset: 2d09363a1516e74bcab7514643f8b5c77b91e65a user: Xidorn Quan <quanxunzhen@gmail.com> date: Thu Aug 06 15:37:48 2015 +1000 description: Bug 1190669 part 1 - Remove helper function SetWindowFullScreen and its helper runnable. r=smaug All of the callsites of this helper function has a synchronous event dispatch which indicates the AddScriptRunner in that helper function makes no sense. Also it seems that most of the callsites are actually safe to run script, except ExitFullscreenInDocTree() which may be called from Element::UnbindFromTree(). It'll be fixed in the following patch. url: https://hg.mozilla.org/integration/mozilla-inbound/rev/f278dbd163fae58402630e36109fd4cc7cec9132 changeset: f278dbd163fae58402630e36109fd4cc7cec9132 user: Xidorn Quan <quanxunzhen@gmail.com> date: Thu Aug 06 15:37:48 2015 +1000 description: Bug 1190669 part 2 - Move the part which runs script from ExitFullscreenInDocTree to a runnable, and protect it with AddScriptRunner. r=smaug url: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e4870fd88547dcce694d0ad09debe8a9409045 changeset: e3e4870fd88547dcce694d0ad09debe8a9409045 user: Xidorn Quan <quanxunzhen@gmail.com> date: Thu Aug 06 15:37:48 2015 +1000 description: Bug 1190669 part 3 - Introduce new fullscreen reason which exits fullscreen without fullscreen transition, and use it for ExitFullscreenInDocTree. r=smaug url: https://hg.mozilla.org/integration/mozilla-inbound/rev/70a60348507ef8af0e6eee965cfc183b8ed0e2bf changeset: 70a60348507ef8af0e6eee965cfc183b8ed0e2bf user: Xidorn Quan <quanxunzhen@gmail.com> date: Thu Aug 06 15:37:48 2015 +1000 description: Bug 1190669 part 4 - Force stop transition animation when the cocoa window gets destroyed. r=smichaud

https://hg.mozilla.org/mozilla-central/rev/2d09363a1516 https://hg.mozilla.org/mozilla-central/rev/f278dbd163fa https://hg.mozilla.org/mozilla-central/rev/e3e4870fd885 https://hg.mozilla.org/mozilla-central/rev/70a60348507e