Bug 99868 - webkitsourceopen event doesn't always fire
Summary: webkitsourceopen event doesn't always fire
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aaron Colwell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-19 13:56 PDT by Aaron Colwell
Modified: 2012-10-22 11:53 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Colwell 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.
Comment 1 Aaron Colwell 2012-10-19 14:03:58 PDT
Created attachment 169689 [details]
Patch
Comment 2 Adam Barth 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.
Comment 3 Aaron Colwell 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)
Comment 4 Adam Barth 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.
Comment 5 Adam Barth 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.
Comment 6 Aaron Colwell 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.
Comment 7 Adam Barth 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.
Comment 8 Aaron Colwell 2012-10-19 18:28:45 PDT
Created attachment 169737 [details]
Added LayoutTest and stop() method.
Comment 9 Adam Barth 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?
Comment 10 Aaron Colwell 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.
Comment 11 Aaron Colwell 2012-10-22 11:09:55 PDT
Created attachment 169941 [details]
Fix test to not use setTimeout() & removed unnecessary comment.
Comment 12 Elliott Sprehn 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+.
Comment 13 Aaron Colwell 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.
Comment 14 Adam Barth 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.
Comment 15 Adam Barth 2012-10-22 11:27:11 PDT
land-safely won't do it for you, but you can do it manually.
Comment 16 WebKit Review Bot 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>
Comment 17 WebKit Review Bot 2012-10-22 11:53:03 PDT
All reviewed patches have been landed.  Closing bug.