Bug 97020

Summary:

HTMLMediaElement isn't garbage collected between document reloads

Product:

WebKit

Reporter:

Andrew Scherkus <scherkus>

Component:

Media

Assignee:

Ami Fischman <fischman>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric.carlson, feature-media-reviews, fischman, haraken, jer.noble, joi, junov, senorblanco, tzik, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Andrew Scherkus

Reported 2012-09-18 07:56:23 PDT

Copied from http://code.google.com/p/chromium/issues/detail?id=148451 Regression introduced in http://trac.webkit.org/changeset/100307 <snip> Summary: It looks like this is not a bug of GC; this is a bug of Audio. (1) In http://www.playescapegoat.com/, two HTMLAudioElements are created, say X and Y. (2) Reload the page. (3) The major GC tries to reclaim X and Y, and calls hasPendingActivity() for X and Y (c.f. http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/v8/V8GCController.cpp&exact_package=chromium&q=v8gccontroller.cpp&type=cs&l=138). (4) X->hasPendingActivity() returns false but Y->hasPendingActivity() returns true. So the GC cannot reclaim Y. (5) Consequently, all Nodes in the DOM tree which Y belongs to are kept alive. The DOM tree contains canvas elements which hold a lot of memory behind it. I think this is the memory leak we are observing. Thus, the problem is that Y->hasPendingActivity() returns true even after the page reload. I investigated why Y->hasPendingActivity() returns true, and observed the following behavior: [In normal cases like WebKit/LayoutTests/media/audio-garbage-collection.html] (1) HTMLMediaElement::potentiallyPlaying() returns true while Y is playing. (2) When the page is reloaded, HTMLMediaElement::potentiallyPlaying() becomes false. Thus, HTMLMediaElement::m_playing becomes false (i.e. http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp&exact_package=chromium&q=htmlmediaelement.cpp&type=cs&l=3579). (3) Because HTMLMediaElement::m_playing is false, Y->hasPendingActivity() returns false. [In http://www.playescapegoat.com/] (1) HTMLMediaElement::potentiallyPlaying() returns true while Y is playing. (2) When the page is reloaded, HTMLMediaElement::potentiallyPlaying() does not become false for some reason. Thus, HTMLMediaElement::m_playing does not become false. (3) Because HTMLMediaElement::m_playing is true, Y->hasPendingActivity() returns true. I think that we need to fix HTMLMediaElement so that HTMLMediaElement::m_playing becomes false when the page is reloaded. (It doesn't make sense that HTMLMediaElement::m_playing keeps being true even after the page reload.) </snip>

Attachments
Patch (1.43 KB, patch) 2012-09-20 15:41 PDT, Ami Fischman	no flags	Details Formatted Diff Diff
Patch (2.91 KB, patch) 2012-09-21 18:28 PDT, Ami Fischman	no flags	Details Formatted Diff Diff
Patch (64.69 KB, patch) 2012-09-24 22:29 PDT, Taiju Tsuiki	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Ami Fischman

Comment 1 2012-09-20 13:30:31 PDT

This isn't V8-specific. I see a memory leak of ~200MB per reload in Safari 6.0.8536.25 on playescapegoat.com (letting the game load to the initial showing a "Play Game" option, no user interaction, let the background music start playing, reload). Instrumenting HTMLMediaElement's ctor & dtor I see the dtor never being called. Sadly I don't have a simpler repro than that full-blown game. Eric/Jer: any idea what's going wrong here?

Ami Fischman

Comment 2 2012-09-20 14:28:03 PDT

Huh; a trivially simple (so simple I suspect I'm missing something) repro: <html> <head> <script> function go() { var a = new Audio(); a.autoplay = "1"; a.src="http://trac.webkit.org/export/129167/trunk/LayoutTests/media/content/test.wav"; } </script> </head> <body onload="go()"> </body> </html> Reloading the page callers m_player->stop() (chromium's ~WebMediaPlayerImpl() ends up being called) but not ~HTMLMediaElement. A DOM-based attempt (using <audio>) fails to repro the problem; the key seems to be generating the HTMLAudioElement using JS.

Ami Fischman

Comment 3 2012-09-20 15:41:19 PDT

Created attachment 164999 [details] Patch

Stephen White

Comment 4 2012-09-20 15:46:17 PDT

Comment on attachment 164999 [details] Patch Leaving for an audio reviewer, but.... awesome. :)

Ami Fischman

Comment 5 2012-09-20 15:52:56 PDT

The attached patch "solves" the problem (leak is gone from both my toy repro and http://www.playescapegoat.com/) though it might be a bit of a hammer; I don't know how the page-reload case is supposed to work. It would also be nice to have a test for this, but I was stymied along two fronts: 1) I don't know how to trigger a page reload in DRT in a way that'd still count as "the same test" 2) I don't know how to observe the leak from JS/lTC, so can't verify its absence. Advice welcome along all fronts :)

Justin Novosad

Comment 6 2012-09-20 16:11:55 PDT

(In reply to comment #5) > The attached patch "solves" the problem (leak is gone from both my toy repro and http://www.playescapegoat.com/) though it might be a bit of a hammer; I don't know how the page-reload case is supposed to work. > > It would also be nice to have a test for this, but I was stymied along two fronts: > 1) I don't know how to trigger a page reload in DRT in a way that'd still count as "the same test" > 2) I don't know how to observe the leak from JS/lTC, so can't verify its absence. > > Advice welcome along all fronts :) It would be easier to test in chrome using the automation controllers (i.e. TabProxy)

Adam Barth

Comment 7 2012-09-20 17:27:05 PDT

> 1) I don't know how to trigger a page reload in DRT in a way that'd still count as "the same test" You can create a popup window using window.open and then call window.reload() in the popup. Search around for other tests that call window.open to see how to turn off the popup blocker. Adam

Adam Barth

Comment 8 2012-09-20 17:28:12 PDT

Comment on attachment 164999 [details] Patch Perhaps setPausedInternal should set m_playing to false instead? I'm not familiar with the details of which functions ought to be responsible for what.

Adam Barth

Comment 9 2012-09-20 17:28:47 PDT

(Doing something like this in stop() is quite likely to be the right solution.)

Eric Carlson

Comment 10 2012-09-20 20:26:12 PDT

Comment on attachment 164999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164999&action=review This does seem like the right way to fix this, but we should have a test. > Source/WebCore/ChangeLog:11 > + No new tests (OOPS!). OOPS indeed!

Ami Fischman

Comment 11 2012-09-20 21:07:47 PDT

Suggestions from Adam & Justin can address the first problem but I don't see how to observe the leak from JS. Anyone got any ideas about how to do that? (note that the fact that m_player->stop() *does* get called means that testing by side-effects caused by the media player, such as network traffic or audio output, is not an option)

Eric Carlson

Comment 12 2012-09-21 12:09:35 PDT

I don't think we have access to enough information from JS to create a layout test. Why don't you create a manual test that someone can use along with another app that allows them to watch memory levels?

Eric Carlson

Comment 13 2012-09-21 12:41:12 PDT

Comment on attachment 164999 [details] Patch r+ but I would prefer to see this land with a manual test rather than adding one later.

Ami Fischman

Comment 14 2012-09-21 18:28:42 PDT

Created attachment 165238 [details] Patch

Ami Fischman

Comment 15 2012-09-21 18:29:26 PDT

Comment on attachment 165238 [details] Patch Oh, yeah, manual tests. I always forget about those. Done.

WebKit Review Bot

Comment 16 2012-09-21 21:44:08 PDT

Comment on attachment 165238 [details] Patch Clearing flags on attachment: 165238 Committed r129296: <http://trac.webkit.org/changeset/129296>

WebKit Review Bot

Comment 17 2012-09-21 21:44:12 PDT

All reviewed patches have been landed. Closing bug.

Taiju Tsuiki

Comment 18 2012-09-24 22:29:49 PDT

Reopening to attach new patch.

Taiju Tsuiki

Comment 19 2012-09-24 22:29:52 PDT

Created attachment 165521 [details] Patch

Adam Barth

Comment 20 2012-09-24 23:01:38 PDT

Looks like your patch got uploaded to the wrong bug.

Taiju Tsuiki

Comment 21 2012-09-24 23:28:20 PDT

Ah, sorry(In reply to comment #20) > Looks like your patch got uploaded to the wrong bug. Ah, sorry. Yes, I missed.

Note You need to log in before you can comment on or make changes to this bug.