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.
Created attachment 169689 [details] Patch
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.
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)
> 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.
I'd feel better if we added some ASSERTs to verify that we're not leaking in these cases.
(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.
> 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.
Created attachment 169737 [details] Added LayoutTest and stop() method.
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?
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.
Created attachment 169941 [details] Fix test to not use setTimeout() & removed unnecessary comment.
(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+.
(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.
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.
land-safely won't do it for you, but you can do it manually.
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>
All reviewed patches have been landed. Closing bug.