962986 - destroying VIDEO element does not reduce memory usage

Status: RESOLVED WORKSFORME

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

Description

[Hallvord R. M. Steen [:hallvors]]

This is a developer complaint that surfaced in bug 942247. Details in comment https://bugzilla.mozilla.org/show_bug.cgi?id=942247#c3 from that bug:

The reason this Firefox + No Flash catch exists is to avoid a browser-crashing HTML5 video memory leak.

To give you a little background about how the site handles memory/loading, we constantly check the user's position on the site and load only what the user needs to see – everything else gets removed. The idea behind this was to not only achieve the fasted initial load time possible, but ensure a stable performance on devices with smaller memory limits. To check out the dynamic loading, open the network inspector and watch new media load as you scroll down. To see content unload, use the inspector to watch <section> with ID "scrollableSectionContainer".

Anyways, the site initially used only HTML5 video, but in testing we noticed that users would see a video, scroll past it (and therefor unload it), then scroll back up to see it again at a later point (reload it). The result was doubled memory allocation for the same video.

We tried everything we could think of to adequately remove the HTML5 video from memory (terminating all event listeners, defining all video related variables to null, deleting objects, and removing references from the DOM), however nothing proved reliable.

Flash on the other hand played very well with memory. We could instantiate a Flash video player and destroy it later to reclaim memory allocations without any issues.

We experienced this video unload inefficiency across multiple browsers, but Firefox seemed to consume memory more quickly and crash more readily.

Comment 1

[Hallvord R. M. Steen [:hallvors]]

Attachment: videos-scrolling.htm - demo

I can reproduce apparently ever increasing memory usage when scrolling this demo up and down several times. It's written based on the developer's description of the problem.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> I can reproduce apparently ever increasing memory usage when scrolling this demo up and
> down several times.

Does the memory go down if you trigger a GC in about:memory?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

So on linux I see memory consumption going up while scrolling and once GC (after CC) has run, memory
usage is back to normal levels.
Sounds like we should tweak GC or CC triggering.

Comment 4

[Boris Zbarsky [:bzbarsky]]

In particular, maybe we should tell the GC about video memory in use?  We do something like that for canvas...

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Er, there is something else too. It is possible to enter to a state where gc/cc don't release the
memory.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Memory is released when the tab containing the testcase is closed.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Ahaa, CC graph has 
0x7f4e459e6800 [rc=1] root FragmentOrElement (xhtml) video https://bug962986.bugzilla.mozilla.org/attachment.cgi?id=8364217
    is a root of the graph with size 5.


So something is keeping video elements alive too long.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

And the graph for those video elements contains something like 
0x7f4e459e6800 [rc=1] root FragmentOrElement (xhtml) video https://bug962986.bugzilla.mozilla.org/attachment.cgi?id=8364217
0x7f4e1efcd370 [rc=1] TextTrackManager
0x7f4e6fde6a40 [rc=1] nsDOMEventTargetHelper 
0x7f4e6fde6aa0 [rc=1] nsDOMEventTargetHelper 
0x7f4e1a2fb480 [rc=1] TextTrackCueList

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: media_element_leak.diff

The patch fixes the leak, but I don't quite understand the self reference setup.
Apparently we're missing to release somewhere, but where...

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8364567 [details] [diff] [review]
media_element_leak.diff

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

Roc wrote the self ref stuff.

Weird that we leak TextTrack stuff though. AFAICT all the refs in the text track related stuff is reported to the cycle collector.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

TextTrack stuff is reported to CC, but CC can't unlink anything because we're keeping
the media element alive.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8364567 [details] [diff] [review]
media_element_leak.diff

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

I don't think we should be doing this. The whole point of the self-reference setup is to keep the video element alive.

This is the gist of it:

  bool needSelfReference = !mShuttingDown &&
    ownerDoc->IsActive() &&
    (mDelayingLoadEvent ||
     (!mPaused && mDecoder && !mDecoder->IsEnded()) ||
     (!mPaused && mSrcStream && !mSrcStream->IsFinished()) ||
     (mDecoder && mDecoder->IsSeeking()) ||
     CanActivateAutoplay() ||
     mNetworkState == nsIDOMHTMLMediaElement::NETWORK_LOADING);

That must be returning true for some video(s) for some reason. I presume you didn't start playing or seeking any of the videos? These videos aren't autoplaying. Maybe the video is stuck in the "loading" state for some reason?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I wasn't saying the patch is correct. It just hints where the issue is.

Comment 14

[Nicholas Nethercote [inactive]]

roc, who do you think should work on this?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know. How about you? :-)

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Well per comment 12 this seems to require someone who understands this code fairly well ...

Comment 17

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Is this still an issue?

Comment 18

[Hallvord R. M. Steen [:hallvors]]

Testing the demo page I attached here in current Nightly indicates it isn't a problem anymore. \o/ I'm asking the original reporter by E-mail if  he has some live page that hit this problem, it would be nice to verify the fix on an actual site. 

It would also be nice to have some test framework doing memory measurements - do we have that?