Bug 99868

Summary:

webkitsourceopen event doesn't always fire

Product:

WebKit

Reporter:

Aaron Colwell <acolwell>

Component:

Media

Assignee:

Aaron Colwell <acolwell>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric.carlson, esprehn, feature-media-reviews, ojan, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Added LayoutTest and stop() method.	none
Fix test to not use setTimeout() & removed unnecessary comment.	none

Aaron Colwell

Reported 2012-10-19 13:56:40 PDT

The following HTML & JavaScript exposes a bug where the webkitsourceopen event doesn't always fire. <html> <body> <video autoplay controls="controls" id='vid'></video> <script type="text/javascript"> function start() { var video = document.getElementById('vid'); var mediaSource = new WebKitMediaSource(); var onSourceOpen = function (e) { document.body.style.background = "silver" document.getElementById('statsArea').textContent = 'ok'; }; document.body.style.background = "red" mediaSource.addEventListener('webkitsourceopen', onSourceOpen); video.src = window.URL.createObjectURL(mediaSource); } start(); </script> <span id="statsArea">loading...</span> </body> </html> The problem is that the JavaScript engine is sometimes garbage collecting the JavaScript MediaSource object because it thinks that it is not active beyond the scope of start(). I talked to abarth@ about this and he said I needed to update MediaSource to derive from ActiveDOMObject. This allows it to tell the JavaScript engine that the MediaSource object is still active even after it goes out of the current scope.

Attachments
Patch (5.72 KB, patch) 2012-10-19 14:03 PDT, Aaron Colwell	no flags	Details Formatted Diff Diff
Added LayoutTest and stop() method. (9.99 KB, patch) 2012-10-19 18:28 PDT, Aaron Colwell	no flags	Details Formatted Diff Diff
Fix test to not use setTimeout() & removed unnecessary comment. (9.96 KB, patch) 2012-10-22 11:09 PDT, Aaron Colwell	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Aaron Colwell

Comment 1 2012-10-19 14:03:58 PDT

Created attachment 169689 [details] Patch

Adam Barth

Comment 2 2012-10-19 14:11:12 PDT

Comment on attachment 169689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169689&action=review > Source/WebCore/ChangeLog:10 > + Bug contains a manual test case. I was unable to reproduce the same garbage collection behavior in a LayoutTest. Did you try including resource/gc.js and calling gc manually? This seems like something we should be able to reproduce in a LayoutTest. > Source/WebCore/Modules/mediasource/MediaSource.cpp:336 > + return m_player || m_asyncEventQueue->hasPendingEvents() > + || ActiveDOMObject::hasPendingActivity(); Is m_player always cleared by the time stop() is called? We don't want to report that we have pending activity forever or else we'll leak memory. Perhaps we should override stop() to ASSERT(!hasPendingActivity()) ? > Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:70 > + // Remove the pending activity added in registerMediaSourceURL(). > + source->unsetPendingActivity(source.get()); What triggers unregisterMediaSourceURL to be called? I'm worried about leaks here again.

Aaron Colwell

Comment 3 2012-10-19 14:33:25 PDT

Comment on attachment 169689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169689&action=review >> Source/WebCore/ChangeLog:10 >> + Bug contains a manual test case. I was unable to reproduce the same garbage collection behavior in a LayoutTest. > > Did you try including resource/gc.js and calling gc manually? This seems like something we should be able to reproduce in a LayoutTest. I'll take a look at this file and try again. I tried sprinkling window.GCController.collect() calls in places that I thought would trigger the problem, but no luck. >> Source/WebCore/Modules/mediasource/MediaSource.cpp:336 >> + || ActiveDOMObject::hasPendingActivity(); > > Is m_player always cleared by the time stop() is called? We don't want to report that we have pending activity forever or else we'll leak memory. > > Perhaps we should override stop() to ASSERT(!hasPendingActivity()) ? Yes. The HTMLMediaElement always calls m_mediaSource->setReadyState(closedKeyword()) when it wants to detach the MediaSource from the HTMLMediaElement. A transition to the "closed" state clears the m_player pointer. When is stop() called? I believe this should be safe assuming that all HTMLMediaElements have been destroyed by the time this call occurs. >> Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:70 >> + source->unsetPendingActivity(source.get()); > > What triggers unregisterMediaSourceURL to be called? I'm worried about leaks here again. There are 2 places. 1. JavaScript can call it via URL.revokeObjectURL() (http://trac.webkit.org/browser/trunk/Source/WebCore/html/DOMURL.cpp#L129) 2. The PublicURLManager can call it when the ScriptExecutionContext gets destroyed. (http://trac.webkit.org/browser/trunk/Source/WebCore/html/PublicURLManager.h#L67, http://trac.webkit.org/browser/trunk/Source/WebCore/dom/ScriptExecutionContext.cpp#L121)

Adam Barth

Comment 4 2012-10-19 15:37:39 PDT

> When is stop() called? I believe this should be safe assuming that all HTMLMediaElements have been destroyed by the time this call occurs. stop() is called when the document is removed from the frame (e.g., when we're navigating the frame to a new document). > >> Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:70 > >> + source->unsetPendingActivity(source.get()); > > > > What triggers unregisterMediaSourceURL to be called? I'm worried about leaks here again. > > There are 2 places. > 1. JavaScript can call it via URL.revokeObjectURL() (http://trac.webkit.org/browser/trunk/Source/WebCore/html/DOMURL.cpp#L129) > 2. The PublicURLManager can call it when the ScriptExecutionContext gets destroyed. (http://trac.webkit.org/browser/trunk/Source/WebCore/html/PublicURLManager.h#L67, http://trac.webkit.org/browser/trunk/Source/WebCore/dom/ScriptExecutionContext.cpp#L121) Great.

Adam Barth

Comment 5 2012-10-19 15:38:05 PDT

I'd feel better if we added some ASSERTs to verify that we're not leaking in these cases.

Aaron Colwell

Comment 6 2012-10-19 16:36:11 PDT

(In reply to comment #5) > I'd feel better if we added some ASSERTs to verify that we're not leaking in these cases. Understood. I'm working on it. I added stop() with the ASSERT you suggested and wouldn't you know it the ASSERT fails. :) I'm trying to untangle what is going on. It looks like the HTMLMediaElement doesn't always transition the MediaSource to closed before MediaSource::stop() is called. I'm looking to see if I can identify a place where HTMLMediaElement is aware of the page closing down before MediaSource::stop() gets called. If not, I'll just clear m_player in stop(). Getting ActiveDOMObject::hasPendingActivity() to be false when stop() is called may be tricky because I believe MediaSource::stop() is getting called before the PublicURLManager has a chance to remove the entries from MediaSourceRegistry. This does eventually get called, but it means that hasPendingActivity() may return true for a little while until the ScriptExecutionContext gets destroyed. Is that ok or would you like me to add a m_stopCalled type boolean to make sure that hasPendingActivity() always returns false after stop()? I was able to create a LayoutTest that exposes the bug so I'll include that in my next patch.

Adam Barth

Comment 7 2012-10-19 16:45:37 PDT

> Is that ok? Yeah, that's fine. As long as we know that PublicURLManager will always balance its calls eventually, then we'll be fine.

Aaron Colwell

Comment 8 2012-10-19 18:28:45 PDT

Created attachment 169737 [details] Added LayoutTest and stop() method.

Adam Barth

Comment 9 2012-10-19 23:43:59 PDT

Comment on attachment 169737 [details] Added LayoutTest and stop() method. View in context: https://bugs.webkit.org/attachment.cgi?id=169737&action=review > Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:53 > + // Add a pending activity to the source so Javascript knows this object is still active. I'd remove this comment. It's just saying what the code is doing. > Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:69 > + // Remove the pending activity added in registerMediaSourceURL(). By contrast, this comment is helpful. :) > LayoutTests/http/tests/media/media-source/video-media-source-garbage-collection-before-sourceopen.html:14 > + if (window.GCController) > + GCController.collect(); Generally we prefer to use gc.js so that we can run the test outside of DumpRenderTree. > LayoutTests/http/tests/media/media-source/video-media-source-garbage-collection-before-sourceopen.html:40 > + setTimeout(runGC, 0); Is there a race here between onSourceOpen and the setTimeout? Maybe we can keep the URL created by createObjectURL around and only set video.src after running GC?

Aaron Colwell

Comment 10 2012-10-22 11:05:27 PDT

Comment on attachment 169737 [details] Added LayoutTest and stop() method. View in context: https://bugs.webkit.org/attachment.cgi?id=169737&action=review >> Source/WebCore/Modules/mediasource/MediaSourceRegistry.cpp:53 >> + // Add a pending activity to the source so Javascript knows this object is still active. > > I'd remove this comment. It's just saying what the code is doing. Done >> LayoutTests/http/tests/media/media-source/video-media-source-garbage-collection-before-sourceopen.html:14 >> + GCController.collect(); > > Generally we prefer to use gc.js so that we can run the test outside of DumpRenderTree. gc.js isn't available, but it looks like an equivalent function is in http/test/resources/js-test-pre.js. I'll update the code to use that. >> LayoutTests/http/tests/media/media-source/video-media-source-garbage-collection-before-sourceopen.html:40 >> + setTimeout(runGC, 0); > > Is there a race here between onSourceOpen and the setTimeout? Maybe we can keep the URL created by createObjectURL around and only set video.src after running GC? Done.

Aaron Colwell

Comment 11 2012-10-22 11:09:55 PDT

Created attachment 169941 [details] Fix test to not use setTimeout() & removed unnecessary comment.

Elliott Sprehn

Comment 12 2012-10-22 11:12:48 PDT

(In reply to comment #11) > Created an attachment (id=169941) [details] > Fix test to not use setTimeout() & removed unnecessary comment. You already got an r+ so if you upload with "webkit-patch land-safely" it'll fill in the reviewer and someone can just cq+.

Aaron Colwell

Comment 13 2012-10-22 11:22:57 PDT

(In reply to comment #12) > (In reply to comment #11) > > Created an attachment (id=169941) [details] [details] > > Fix test to not use setTimeout() & removed unnecessary comment. > > You already got an r+ so if you upload with "webkit-patch land-safely" it'll fill in the reviewer and someone can just cq+. Can I still do this now? The r+ on the previous patch is gone now.

Adam Barth

Comment 14 2012-10-22 11:26:50 PDT

Comment on attachment 169941 [details] Fix test to not use setTimeout() & removed unnecessary comment. Yes. You can always write the reviewer's name directly in the ChangeLog.

Adam Barth

Comment 15 2012-10-22 11:27:11 PDT

land-safely won't do it for you, but you can do it manually.

WebKit Review Bot

Comment 16 2012-10-22 11:52:59 PDT

Comment on attachment 169941 [details] Fix test to not use setTimeout() & removed unnecessary comment. Clearing flags on attachment: 169941 Committed r132115: <http://trac.webkit.org/changeset/132115>

WebKit Review Bot

Comment 17 2012-10-22 11:53:03 PDT

All reviewed patches have been landed. Closing bug.

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