Dorando Reporter
	Description • 7 years ago
regression-window str

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)

	Andrew McCreight [:mccr8]
	Comment 1 • 7 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.

	Dorando Reporter
	Comment 2 • 7 years ago
str 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

	JW Wang [:jwwang] [:jw_wang]
	Comment 3 • 7 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.

	Andrew McCreight [:mccr8]
	Comment 4 • 7 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.

	Andrew McCreight [:mccr8]
	Comment 5 • 7 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?

	Tzuhao Kuo [:kaku] Assignee
	Comment 6 • 7 years ago

Will check it.

	Tzuhao Kuo [:kaku] Assignee
	Comment 7 • 7 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?

	Andrew McCreight [:mccr8]
	Comment 8 • 7 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.

	Tzuhao Kuo [:kaku] Assignee
	Comment 9 • 7 years ago

Thanks for suck detailed information, I can reproduce it now and I will check it!

	Julien Cristau [:jcristau]
	Comment 10 • 7 years ago

Mass wontfix for bugs affecting firefox 52.

	Tzuhao Kuo [:kaku] Assignee
	Comment 15 • 7 years ago

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a07d8cf63dce07714b92f93c6c59dbad588ffb5
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a3c1e48f45208aaadba3d83475438c4d8e16711

	JW Wang [:jwwang] [:jw_wang]
	Comment 16 • 7 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.

	JW Wang [:jwwang] [:jw_wang]
	Comment 17 • 7 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

	JW Wang [:jwwang] [:jw_wang]
	Comment 18 • 7 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

	JW Wang [:jwwang] [:jw_wang]
	Comment 19 • 7 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.

	Tzuhao Kuo [:kaku] Assignee
	Comment 20 • 7 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.

	Tzuhao Kuo [:kaku] Assignee
	Comment 25 • 7 years ago

Try again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1cde224b35764c7f2734c3d461c5326ee8ac9cf
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c46b21f875479d1e8939cf833f6f8734e145a857

	Tzuhao Kuo [:kaku] Assignee
	Comment 26 • 7 years ago

Try looks good, thanks for the review!

	Pulsebot
	Comment 27 • 7 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

	Carsten Book [:Tomcat]
	Comment 28 • 7 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

	Blake Wu [:bwu][:blakewu]
	Comment 29 • 7 years ago

Kaku,
Is this bug a serious problem that we need to request lift?

	Tzuhao Kuo [:kaku] Assignee
	Comment 30 • 7 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.

	Gerry Chang [:gchang]
	Comment 31 • 7 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-.

	Gerry Chang [:gchang]
	Comment 32 • 7 years ago

Change 53 back to affected and leave the decision to release owner of 53.

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 7 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.

	Tzuhao Kuo [:kaku] Assignee
	Comment 34 • 7 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 7 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.

	Gerry Chang [:gchang]
	Comment 36 • 7 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?

	Cornel Ionce [:noni] [Hubs QA]
	Comment 37 • 7 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.

	Gerry Chang [:gchang]
	Comment 38 • 7 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.

	Tzuhao Kuo [:kaku] Assignee
	Comment 39 • 7 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?

	Gerry Chang [:gchang]
	Comment 40 • 7 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 7 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

	Tzuhao Kuo [:kaku] Assignee
	Comment 42 • 7 years ago

Thanks!

	Bogdan Maris, Desktop QA
	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).