Leak when quitting from full-screen mode

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: jruderman, Assigned: xidorn)

Tracking

(Blocks 1 bug, {memory-leak, testcase})

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(5 attachments)

Reporter

Description

4 years ago
Posted file testcase
1. Load testcase
2. Click button to enter full screen mode
3. Quit Firefox with Cmd+Q

Result: leak nsGlobalWindow, nsDocument, etc.
Whiteboard: [MemShrink]
Is this a regression?
Although this is just a shutdown bug, this might block us from finding other leaks in fullscreen.
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee

Comment 3

4 years ago
How can I see the leak info on Mac?
Flags: needinfo?(jruderman)
Reporter

Comment 4

4 years ago
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
Flags: needinfo?(jruderman)
Assignee

Comment 5

4 years ago
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.
Blocks: 1160014
Assignee

Comment 6

4 years ago
I have found a solution, but probably need to do some cleanup first.
Assignee: nobody → quanxunzhen
Assignee

Comment 9

4 years ago
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.
Attachment #8643518 - Flags: review?(bugs)
Assignee

Comment 10

4 years ago
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.
Reporter

Comment 11

4 years ago
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
Assignee

Comment 12

4 years ago
(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.
Reporter

Comment 13

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

Updated

4 years ago
Depends on: 1191112
Assignee

Comment 14

4 years ago
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.
Attachment #8643674 - Flags: review?(smichaud)
Comment on attachment 8643674 [details] [diff] [review]
patch 4 - stop transtion animation when cocoa window gets destroyed

Looks reasonable to me.
Attachment #8643674 - Flags: review?(smichaud) → review+
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
Attachment #8643516 - Flags: review?(bugs) → review-
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.
Attachment #8643517 - Flags: review?(bugs) → review+
Attachment #8643518 - Flags: review?(bugs) → review+
Assignee

Comment 18

4 years ago
(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
Flags: needinfo?(bugs)
(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...
Flags: needinfo?(bugs)
Attachment #8643516 - Flags: review- → review+
Assignee

Comment 21

4 years ago
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
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.