Closed
Bug 1344357
Opened 8 years ago
Closed 8 years ago
Closing a content window with a seeking video leaks the window
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla55
People
(Reporter: bugzilla-mozilla, Assigned: kaku)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(5 files)
(deleted),
text/html
|
Details | |
(deleted),
text/x-review-board-request
|
jwwang
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
(deleted),
text/x-review-board-request
|
jwwang
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jwwang
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jwwang
:
review+
|
Details |
Affected: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0 20170125094131 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0 20170302030206 STR: 0. Fresh profile 1. Ensure max content processes are active as per dom.ipc.processCount (Ctrl+T, Alt+Home; repeat) 2. Load any of the following videos in a new tab: http://download.blender.org/peach/bigbuckbunny_movies/big_buck_bunny_1080p_stereo.ogg http://download.blender.org/peach/bigbuckbunny_movies/BigBuckBunny_640x360.m4v http://download.blender.org/peach/bigbuckbunny_movies/BigBuckBunny_320x180.mp4 http://video.webmfiles.org/big-buck-bunny_trailer.webm http://youtube.com/watch?v=YE7VzlLtp-4 3. Wait for the video to start 4. Click on the progress bar anywhere not already buffered 5. Immediately press Ctrl+W 6. Open about:memory in a new tab β βββββ150,632 B (00.25%) -- top(none)/detached/window([β¦]) 0A7A5640 [Promise] --[mGlobal]--> 06A73000 [nsGlobalWindow # 2147483654 inner [β¦]] --[mDoc]--> 06926000 [nsDocument normal (xhtml) [β¦]] --[mChildren[i]]--> 08E429A0 [FragmentOrElement (xhtml) html [β¦]] --[mAttrsAndChildren[i]]--> 08E53B00 [FragmentOrElement (xhtml) body [β¦]] --[mAttrsAndChildren[i]]--> 0A770000 [FragmentOrElement (xhtml) video [β¦]] Root 0A7A5640 is a ref counted object with 1 unknown edge(s). Regression Range: 2016-06-14-03-02-10-mozilla-central/ ok 2016-06-15-03-02-09-mozilla-central/ broken https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ddb6b3014922&tochange=14c5bf11d37b maybe a366d8ef5e9f Kaku Kuo β Bug 1276272 - part 3 - implement promise-based HTMLMediaElement::seekToNextFrame(); r=jwwang Notes: After the fourth leak, it seems impossible to load any new video Youtube can be leaked more than four times (and possibly gets cleared after enough leaks, but I couldn't reproduce it) Nightly might cause high CPU "http channel Listener OnDataAvailable contract violation" might get logged to Browser Console (couldn't reproduce)
Flags: needinfo?(kaku)
Flags: needinfo?(continuation)
Comment 1•8 years ago
|
||
Interesting. That leak looks similar to what I was seeing in bug 1343353, and I think seeking was triggering it more often. That patch does sound pretty related.
Alternative STR: 0. Fresh profile 1. Ensure max content processes are active as per dom.ipc.processCount (Ctrl+T, Alt+Home; repeat) 2. Open Testcase is a new tab 3. Press the button 4. Close Testcase tab 5. Open about:memory in a new tab
Updated•8 years ago
|
Flags: needinfo?(continuation)
Whiteboard: [MemShrink]
Updated•8 years ago
|
Status: UNCONFIRMED → NEW
status-firefox51:
--- → wontfix
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
Ever confirmed: true
Comment 3•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #1) > Interesting. That leak looks similar to what I was seeing in bug 1343353, > and I think seeking was triggering it more often. That patch does sound > pretty related. I don't think bug 1276272 is responsible. seekToNextFrame() is a special kind of seek which is never activated in this test case.
Comment 4•8 years ago
|
||
I bisected using mozregression with the test case in comment 2 and starting with the range in comment 0 (thanks very much for that, it made this much easier!) and ended up with this regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2a69a6628249dba84af6ad91c371a4bcb662cd8c&tochange=beb4b214b6b74b10f8b75e6231cc7dbe49c77c51 So it is almost certainly bug 1276272.
Blocks: 1276272
Comment 5•8 years ago
|
||
The leak goes away when we're shutting down the content process, so maybe there's some media thing that kills something off for shutdown that also needs to do it for the window close?
Assignee | ||
Comment 6•8 years ago
|
||
Will check it.
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Flags: needinfo?(kaku)
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Comment 7•8 years ago
|
||
Hi Dorando, Sorry that I'm not able to reproduce this issue, I have tried the latest m-c on Windows, Linux, and Mac, but all failed. However, I believe that it should be reproducible since Andrew and you can do it. May I ask for a recording to your reproduction steps?
Flags: needinfo?(bugzilla-mozilla)
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P2]
Updated•8 years ago
|
Blocks: GhostWindows
Comment 8•8 years ago
|
||
Here are more detailed steps to reproduce that I am able to use. 1. Make a debug build of Firefox. Start it with ./mach run 2. Open mozilla.org in 4 separate tabs (to ensure that when we open the test case it will run in an existing content process). 3. Open a new tab, go to https://bugzilla.mozilla.org/attachment.cgi?id=8843452 Then, click the button. Wait for the test case to create a new tab, then close the tab. Wait about a second, then close the tab for the test case. 4. Open a new tab, go to about:memory. Click on minimize memory usage a few times, then click on "measure". You may want to check "verbose" because then you won't have to dig into the various collapsed categories. You should see something like this: βββββ5,959,504 B (05.79%) -- window-objects β βββ5,547,056 B (05.39%) ++ top(https://www.mozilla.org/en-US/, id=4294967301) β βββββ412,448 B (00.40%) -- top(none) β βββ410,400 B (00.40%) -- ghost β β βββ266,480 B (00.26%) ++ window(about:blank) β β βββ143,920 B (00.14%) -- window(https://bug1344357.bmoattachments.org/attachment.cgi?id=8843452) Instead of "ghost", it may have "detached", but after a minute that will turn into a ghost window. Does this help? It would be nice to have this fixed. Some users in bug 1334367 seem to be experiencing a similar issue.
Flags: needinfo?(bugzilla-mozilla) → needinfo?(kaku)
Assignee | ||
Comment 9•8 years ago
|
||
Thanks for suck detailed information, I can reproduce it now and I will check it!
Flags: needinfo?(kaku)
Updated•8 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•8 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a07d8cf63dce07714b92f93c6c59dbad588ffb5 https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a3c1e48f45208aaadba3d83475438c4d8e16711
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8858771 [details] Bug 1344357 P1 - move the MediaDecoder::mSeekDOMPromise to HTMLMediaElement; https://reviewboard.mozilla.org/r/130818/#review133560 ::: dom/html/HTMLMediaElement.cpp:2726 (Diff revision 1) > > // The media backend is responsible for dispatching the timeupdate > // event if it changes the playback position as a result of the seek. > LOG(LogLevel::Debug, ("%p SetCurrentTime(%f) starting seek", this, aTime)); > + mSeekDOMPromise = promise; > nsresult rv = mDecoder->Seek(aTime, aSeekType, promise); We don't have to pass |promise| to mDecoder->Seek(). ::: dom/html/HTMLMediaElement.cpp:7499 (Diff revision 1) > +void > +HTMLMediaElement::AsyncResolveSeekDOMPromiseIfExists() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + if (mSeekDOMPromise) { > + RefPtr<dom::Promise> promise = mSeekDOMPromise; promise = mSeekDOMPromise.forget(); to avoid unnecessary ref-counting. ::: dom/html/HTMLMediaElement.cpp:7513 (Diff revision 1) > +void > +HTMLMediaElement::AsyncRejectSeekDOMPromiseIfExists() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + if (mSeekDOMPromise) { > + RefPtr<dom::Promise> promise = mSeekDOMPromise; Ditto.
Attachment #8858771 -
Flags: review?(jwwang) → review+
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8858772 [details] Bug 1344357 P2 - put HTMLMediaElement::mSeekDOMPromise into cycle collector; https://reviewboard.mozilla.org/r/130820/#review133562
Attachment #8858772 -
Flags: review?(jwwang) → review+
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8858773 [details] Bug 1344357 P3 - dont' pass dom::Promise into MediaDecoder anymore; https://reviewboard.mozilla.org/r/130822/#review133564
Attachment #8858773 -
Flags: review?(jwwang) → review+
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8858774 [details] Bug 1344357 P4 - add a mochitest; https://reviewboard.mozilla.org/r/130824/#review133566 ::: dom/media/test/test_seek_promise_bug1344357.html:14 (Diff revision 1) > +<body> > +<pre id="test"> > + > +<script> > + > +// This test always successes at runtime but should not cause any leakage in the end. 1. succeeds 2. cause any leaks 3. at the end of mochitests.
Attachment #8858774 -
Flags: review?(jwwang) → review+
Assignee | ||
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8858771 [details] Bug 1344357 P1 - move the MediaDecoder::mSeekDOMPromise to HTMLMediaElement; https://reviewboard.mozilla.org/r/130818/#review133680 ::: dom/html/HTMLMediaElement.cpp:2725 (Diff revision 1) > mPlayingBeforeSeek = IsPotentiallyPlaying(); > > // The media backend is responsible for dispatching the timeupdate > // event if it changes the playback position as a result of the seek. > LOG(LogLevel::Debug, ("%p SetCurrentTime(%f) starting seek", this, aTime)); > + mSeekDOMPromise = promise; We can't make this assignment here because the promise might be rejected immediatelly at the following call to MediaDecoder::Seek() -> MediaDecoder::CallSeek() -> MediaDecoder::DiscardOngoingSeekIfExists(). http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/dom/media/MediaDecoder.cpp#786 We should move this assignment to the end of this method.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•8 years ago
|
||
Try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1cde224b35764c7f2734c3d461c5326ee8ac9cf https://treeherder.mozilla.org/#/jobs?repo=try&revision=c46b21f875479d1e8939cf833f6f8734e145a857
Comment 27•8 years ago
|
||
Pushed by tkuo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8dc519a9f713 P1 - move the MediaDecoder::mSeekDOMPromise to HTMLMediaElement; r=jwwang https://hg.mozilla.org/integration/autoland/rev/dd1d45aa8525 P2 - put HTMLMediaElement::mSeekDOMPromise into cycle collector; r=jwwang https://hg.mozilla.org/integration/autoland/rev/9625a8331c27 P3 - dont' pass dom::Promise into MediaDecoder anymore; r=jwwang https://hg.mozilla.org/integration/autoland/rev/03f394bbe821 P4 - add a mochitest; r=jwwang
Comment 28•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8dc519a9f713 https://hg.mozilla.org/mozilla-central/rev/dd1d45aa8525 https://hg.mozilla.org/mozilla-central/rev/9625a8331c27 https://hg.mozilla.org/mozilla-central/rev/03f394bbe821
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 29•8 years ago
|
||
Kaku, Is this bug a serious problem that we need to request lift?
Flags: needinfo?(kaku)
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8858771 [details] Bug 1344357 P1 - move the MediaDecoder::mSeekDOMPromise to HTMLMediaElement; Approval Request Comment [Feature/Bug causing the regression]: bug 1276272 [User impact if declined]: Leaking window object in relatively rare cases which is closing a window with a video element that is under seeking. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes, but by myself only. [Needs manual test from QE? If yes, steps to reproduce]: If possible, yes. The STR is in comment 8. [List of other uplifts needed for the feature/fix]: all patches in this bug. attachment 8858771 [details], attachment 8858772 [details], attachment 8858773 [details], attachment 8858774 [details]. [Is the change risky?]: No. [Why is the change risky/not risky?]: Basically, the code logic is not changed at all, we merely make a variable to participate in the cycle collection. [String changes made/needed]: n/a.
Flags: needinfo?(kaku)
Attachment #8858771 -
Flags: approval-mozilla-release?
Attachment #8858771 -
Flags: approval-mozilla-beta?
Attachment #8858771 -
Flags: approval-mozilla-aurora?
Comment 31•8 years ago
|
||
Comment on attachment 8858771 [details] Bug 1344357 P1 - move the MediaDecoder::mSeekDOMPromise to HTMLMediaElement; It's been there for a while since FF51 and we don't have aurora phase to stabilize this. Let's let it ride the train on 55 and won't fix in 54. Beta54-.
Attachment #8858771 -
Flags: approval-mozilla-beta?
Attachment #8858771 -
Flags: approval-mozilla-beta-
Attachment #8858771 -
Flags: approval-mozilla-aurora?
Updated•8 years ago
|
Updated•8 years ago
|
Comment 33•8 years ago
|
||
Not sure why we'd consider it for a 53 dot release if we don't want it on 54. FWIW, window leaks sound pretty crappy to me. It's a rare case but an ugly one if we hit it.
status-firefox-esr52:
--- → affected
Assignee | ||
Comment 34•8 years ago
|
||
Comment on attachment 8858771 [details] Bug 1344357 P1 - move the MediaDecoder::mSeekDOMPromise to HTMLMediaElement; No, if lifting to 54 is not approved, I would like to cancel the request to 53 or we will be in an inconsistent states.
Attachment #8858771 -
Flags: approval-mozilla-release?
Comment 35•8 years ago
|
||
I think we should reconsider this for 54. Having this code participate in cycle collection isn't very high-risk, and whole window leaks can be really ugly when they happen, both from a memory usage standpoint and CC/GC jank & hang perspective. And per #c0, this also prevents any new videos from loading when it happens.
Flags: needinfo?(gchang)
Comment 36•8 years ago
|
||
Hi Florin, Can you help find someone to verify this issue and also check if any regressions for the audio/video playback before uplifting to Beta54?
Flags: qe-verify+
Flags: needinfo?(gchang)
Flags: needinfo?(florin.mezei)
Comment 37•8 years ago
|
||
Reproduced the issue on Windows 10x64 with Firefox 53 (20170413192749) using the STR from comment 8. Confirming the fix on Mac OS X 10.11, Windows 10x64 and Ubuntu 14.04 x64 with Fx 55 Nightly, build ID: 20170425030221. Exploratory testing was also performed to ensure this fix does not cause any new issues/regressions to A/V.
Comment 38•8 years ago
|
||
Comment on attachment 8858771 [details] Bug 1344357 P1 - move the MediaDecoder::mSeekDOMPromise to HTMLMediaElement; Thanks for verification. Let's take it in Beta54. Should be in 54 beta 3.
Attachment #8858771 -
Flags: approval-mozilla-beta- → approval-mozilla-beta+
Updated•8 years ago
|
Assignee | ||
Comment 39•8 years ago
|
||
(In reply to Gerry Chang [:gchang] from comment #38) > Comment on attachment 8858771 [details] > Bug 1344357 P1 - move the MediaDecoder::mSeekDOMPromise to HTMLMediaElement; > > Thanks for verification. Let's take it in Beta54. Should be in 54 beta 3. Will patch-1 ~ patch-4 be uplifted as a whole? or, should I request approvals for each patch?
Flags: needinfo?(gchang)
Comment 40•8 years ago
|
||
Hi :kaku, I suppose you want to uplift all patches. If that's the case, that should be fine now. Sheriff is aware of this.
Flags: needinfo?(gchang)
Comment 41•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/773644d29d67 https://hg.mozilla.org/releases/mozilla-beta/rev/a8b2d8cd033a https://hg.mozilla.org/releases/mozilla-beta/rev/6647dcb61997 https://hg.mozilla.org/releases/mozilla-beta/rev/a6e7cc841be7
Flags: in-testsuite+
Comment 43•7 years ago
|
||
I also reproduced the initial issue using Firefox 53.0.2 on Windows 10 64bit based on instructions from comment 8. I can see that the issue is fixed using latest Firefox 54 beta 9 across platforms (Windows 10, macOS 10.12.4 and Ubuntu 16.04 32bit).
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•