Closed Bug 1190019 Opened 9 years ago Closed 9 years ago

Ghost-Windows on youtube with e10s disabled

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- unaffected
firefox42 + fixed

People

(Reporter: speciesx, Assigned: jya)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, regression, Whiteboard: [MemShrink:P1])

Attachments

(6 files)

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:42.0) Gecko/20100101 Firefox/42.0
Cset: ca53d4297f02

Tested with new profile and without add-ons.

Steps to reproduce:
1. Open about:memory on the first tab.
2. On the second tab, open and start a youtube video. / Open and start a youtube video in private window.
5. Close the second tab. / Close the private window
6. On the first tab click minimize memory.

or 

1. Open about:memory on the first tab.
2. Open and start a youtube video in private window.
5. Close the private window
6. On the first tab click minimize memory.



Actual result:

6 (100.0%) -- ghost-windows
├──4 (66.67%) ── https://www.youtube.com/.... [4]
└──2 (33.33%) ── https://googleads.g.doubleclick.net/.... [2]
Blocks: GhostWindows
Is this a regression? If so, could you perhaps try to find the regression range?
I can't reproduce on linux.
Whiteboard: [MemShrink]
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0
BuildID: 20150803030207

I can reproduce it with a plain new profile, i.e. with e10s enabled.

Steps to reproduce:
1. In a new profile, open about:home (to prevent Web Content from termination) and about:memory.
2. On a new tab, open any youtube video, e.g. https://www.youtube.com/watch?v=dQw4w9WgXcQ (Rick Roll).
3. After it starts buffering, close the tab.
4. Miminize memory usage a few times in about:memory. (The first time usually only creates an entry top(none)/detached/window)

Actual result:
Ghost windows are created like comment 0. Memory report is attached.

Note:
Ghost windows are created only after the video has started buffering.
If I suppress auto-buffering by an extension (e.g. YouTube Center Developer Build), or visit a site with auto-pause (no pre-buffering) embedded youtube videos (e.g. http://www.isd.gov.hk/drinkingwater/eng/index.html), no ghost windows are created unless I start the buffer manually.

From Mozregression:
Last GOOD Nightly: 2015-07-22
First BAD Nightly: 2015-07-23
Last good revision: 8e5c888d0d89
First bad revision: 1f77b78797d6
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8e5c888d0d89&tochange=1f77b78797d6

Last good revision: f8051d507461
First bad revision: de0a5cf8c4f9
INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f8051d507461&tochange=de0a5cf8c4f9
Is it possible for you to provide verbose GC/CC logs for a session with the ghost window too?  You can get those from about:memory as well.  Unfortunately I couldn't reproduce.

Those logs can contain private data (such as other URLs loaded in the browser) so you'll want to be sure to use a clean profile.
Flags: needinfo?(Fanolian+bugzilla)
Verbose memory report
Flags: needinfo?(Fanolian+bugzilla)
No, that's not what I'm looking for.  Do you see the "Save GC & CC logs" section?  I want that verbose log :)
Flags: needinfo?(Fanolian+bugzilla)
Attached file GC & CC log.7z
Here my
Attached file GC_CC_logs_verbose.7z
The button creates 4 files. Do you need them all? If not, I will upload fewer next time.
Flags: needinfo?(Fanolian+bugzilla)
Comment 8 was exactly what I was looking for, thanks.
From the cycle collector's perspective, the ghost window is rooted by a number of SourceBuffer objects that have one unknown reference.  Two references from SourceBufferLists and one from JS are accounted for, but there's a fourth reference that is unknown.

This is pretty clearly a bug of some sort in the media code, probably in lifetime management surrounding MediaSourceDemuxer.
Blocks: 1171379
Status: UNCONFIRMED → NEW
Component: DOM → Audio/Video: Playback
Ever confirmed: true
Flags: needinfo?(jyavenard)
There's nothing unknown about the references as far as I'm concerned.

From the media perspective we have:

HTMLMediaElement -> MediaSourceDecoder -> MediaSourceDemuxer -> MediaFormatReader

The MediaSourceDemuxer contains an array of TrackBuffersManager.

The TrackBuffersManager has a strong reference to its parent SourceBuffer.

The SourceBuffer is owned by the MediaSource inside a SourceBufferList. The SourceBuffer also keeps a strong reference to the TrackBuffersManager.

When the HTMLMediaElement is shutdown ; the MediaSourceDemuxer will break its cycle reference with the TrackBuffersManagers and the MediaSource object will shutdown, which will also break its cycle reference between the SourceBuffer and the TrackBuffersManager.

Now if the e10s / cycle manager is confused by that ; I'm happy to work with you in getting it resolved.

A SourceBuffer may take up to 100MB of data ; a MediaSource will typically have two SourceBuffer attached, so potentially 200MB of data per HTMLMediaElement. This is required by Youtube (Chrome uses 150MB for the video sourcebuffer and 12MB for the audio sourcebuffer)
Flags: needinfo?(jyavenard)
The TrackBuffersManager is the only object that will take a significant size.

The TrackBuffersManager will be destroyed when the HTMLMediaElement is shutdown.

Chris, any thought on why that couldn't be the case ?

(BTW, any leaks would clearly show in debug build as most of the objects owned by the TrackBuffersManager have MOZ_CTOR/MOZ_DTOR macro, even the refcounted one)
Flags: needinfo?(cpearce)
STR:

1. ./mach mochitest -f plain dom/media/test/test_eme_stream_capture_blocked_case1.html
2. Once the test completes, open a new tab.
3. Close the tab the test ran in.
4. Navigate to about:memory
5. Minimize memory
6. Measure (possibly repeat steps 5 and 6 a couple times)

You will see ghost windows from mochi.test when there should be none.  These windows do get cleaned up when you exit the browser (they don't show up as leaks on tinderbox) so this is a leak until shutdown rather than a leak through shutdown.
Poking around a bit the MediaSourceDecoder, MediaSource, etc are all alive until MediaShutdownManager::Shutdown.  That's definitely a bug.
Also my patch in bug 1179909 turns this into a partial shutdown leak.
Blocks: 1179909
[Tracking Requested - why for this release]: large leaks on a popular site, YouTube.
The MediaDecoder::Shutdown() is called in the HTMLMediaElement destructor.

However, as the TrackBuffersManager hold a reference to the SourceBuffer which is hold by the MediaSource which has a strong reference to the MediaDecoderOwner ; the HTMLMediaElement destructor will never be called.

Need to find a way to break this cycle.
Talked 1:1.
Flags: needinfo?(cpearce)
Instead we use a a ref-counted attribute holder to store those arguments.
Attachment #8642803 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
Comment on attachment 8642803 [details] [diff] [review]
[MSE] P1. Remove cycle between SourceBuffer and TrackBuffersManager.

Review of attachment 8642803 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/mediasource/SourceBuffer.cpp
@@ +323,5 @@
>    MSE_DEBUG("Create mContentManager=%p",
>              mContentManager.get());
>    if (aType.LowerCaseEqualsLiteral("audio/mpeg") ||
>        aType.LowerCaseEqualsLiteral("audio/aac")) {
> +    mAttributes->mGenerateTimestamps = true;

You could make SourceBufferAttributes::mGenerateTimestamps const if you initialized SourceBuffer::mAttributes slightly later in the SourceBuffer ctor here and passed mGenerateTimestamps into SourceBufferAttributes's ctor.
Attachment #8642803 - Flags: review?(cpearce) → review+
Attachment #8642864 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/a736fc66131d
https://hg.mozilla.org/mozilla-central/rev/bd2d8b78d1fc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Whiteboard: [MemShrink] → [MemShrink:P1]
Regressions ought to be tracked even after they are fixed.
Is it in the nightly? Because if it is, it doesn't look fixed.
(In reply to laz2727 from comment #28)
> Is it in the nightly? Because if it is, it doesn't look fixed.

Nope it will be in today's build.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: