Closed Bug 1344357 Opened 3 years ago Closed 3 years ago

Closing a content window with a seeking video leaks the window

Categories

(Core :: Audio/Video: Playback, defect, P1)

50 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- affected
firefox53 --- wontfix
firefox54 --- verified
firefox55 --- verified

People

(Reporter: bugzilla-mozilla, Assigned: kaku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(5 files)

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)
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.
Attached file Testcase
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
Flags: needinfo?(continuation)
Whiteboard: [MemShrink]
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
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
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?
Will check it.
Assignee: nobody → kaku
Status: NEW → ASSIGNED
Flags: needinfo?(kaku)
Priority: -- → P1
Blocks: 1343353
See Also: 1343353
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)
Whiteboard: [MemShrink] → [MemShrink:P2]
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)
Thanks for suck detailed information, I can reproduce it now and I will check it!
Flags: needinfo?(kaku)
Mass wontfix for bugs affecting firefox 52.
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 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 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 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+
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.
Try looks good, thanks for the review!
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
Kaku,
Is this bug a serious problem that we need to request lift?
Flags: needinfo?(kaku)
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 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?
Change 53 back to affected and leave the decision to release owner of 53.
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.
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?
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)
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)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(florin.mezei)
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+
(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)
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)
Thanks!
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.