Bug 90180 - MediaStream API: Update MediaStreamTrack to match the specification
Summary: MediaStream API: Update MediaStreamTrack to match the specification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on: 92161
Blocks: 56459
  Show dependency treegraph
 
Reported: 2012-06-28 07:49 PDT by Tommy Widenflycht
Modified: 2012-07-25 09:53 PDT (History)
11 users (show)

See Also:


Attachments
Patch (18.78 KB, patch)
2012-06-28 07:57 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (18.83 KB, patch)
2012-06-29 01:31 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (40.43 KB, patch)
2012-07-20 02:22 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (42.49 KB, patch)
2012-07-23 04:06 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (42.33 KB, patch)
2012-07-24 04:43 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff
Patch (43.13 KB, patch)
2012-07-25 02:09 PDT, Tommy Widenflycht
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2012-06-28 07:49:46 PDT
MediaStreamTracks are now required to show the status of the underlying source, and trigger events when that status changes.
Comment 1 Tommy Widenflycht 2012-06-28 07:57:47 PDT
Created attachment 149955 [details]
Patch
Comment 2 Adam Barth 2012-06-28 08:54:29 PDT
Comment on attachment 149955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149955&action=review

> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:84
> +    ScriptExecutionContext* m_context;

What controls the relation between the lifetime of MediaStreamTrack and ScriptExecutionContext?  They both appear to be refcounted and so likely have unrelated lifetimes, which means this pointer can become stale.
Comment 3 Adam Barth 2012-06-28 08:55:27 PDT
Comment on attachment 149955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149955&action=review

> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:43
> -class MediaStreamTrack : public RefCounted<MediaStreamTrack> {
> +class MediaStreamTrack : public RefCounted<MediaStreamTrack>, public EventTarget, public MediaStreamSource::Observer {

It's likely that MediaStreamTrack needs to be an ActiveDOMObject given that it's going to fire events on itself.
Comment 4 WebKit Review Bot 2012-06-28 10:13:49 PDT
Comment on attachment 149955 [details]
Patch

Attachment 149955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13106154
Comment 5 Tommy Widenflycht 2012-06-29 01:31:53 PDT
Created attachment 150112 [details]
Patch
Comment 6 Tommy Widenflycht 2012-06-29 01:35:40 PDT
Comment on attachment 149955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149955&action=review

>> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:43
>> +class MediaStreamTrack : public RefCounted<MediaStreamTrack>, public EventTarget, public MediaStreamSource::Observer {
> 
> It's likely that MediaStreamTrack needs to be an ActiveDOMObject given that it's going to fire events on itself.

Hmm, why? MediaStreamTrack doesn't fire events on itself, it fires events into the JS world.

>> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:84
>> +    ScriptExecutionContext* m_context;
> 
> What controls the relation between the lifetime of MediaStreamTrack and ScriptExecutionContext?  They both appear to be refcounted and so likely have unrelated lifetimes, which means this pointer can become stale.

ScriptExecutionContext is the document as far as I know and therefore no object can outlive it. Also storing this pointer in a RefPtr can stop a world teardown to take place.n
Comment 7 Tommy Widenflycht 2012-06-29 01:37:29 PDT
Comment on attachment 149955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149955&action=review

>>> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:43
>>> +class MediaStreamTrack : public RefCounted<MediaStreamTrack>, public EventTarget, public MediaStreamSource::Observer {
>> 
>> It's likely that MediaStreamTrack needs to be an ActiveDOMObject given that it's going to fire events on itself.
> 
> Hmm, why? MediaStreamTrack doesn't fire events on itself, it fires events into the JS world.

Btw, I'm happy to make it a ActiveDOMObject, I just want to know the reasoning behind.
Comment 8 Adam Barth 2012-06-29 07:48:23 PDT
Comment on attachment 149955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149955&action=review

>>>> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:43
>>>> +class MediaStreamTrack : public RefCounted<MediaStreamTrack>, public EventTarget, public MediaStreamSource::Observer {
>>> 
>>> It's likely that MediaStreamTrack needs to be an ActiveDOMObject given that it's going to fire events on itself.
>> 
>> Hmm, why? MediaStreamTrack doesn't fire events on itself, it fires events into the JS world.
> 
> Btw, I'm happy to make it a ActiveDOMObject, I just want to know the reasoning behind.

Right, it fires events into the JS world.  We want it to stop firing events when the document is no longer active (i.e., displayed in a frame).  To do that, we subclass ActiveDOMObject and get the stop() callback, after which we shouldn't fire any more events.

>>> Source/WebCore/Modules/mediastream/MediaStreamTrack.h:84
>>> +    ScriptExecutionContext* m_context;
>> 
>> What controls the relation between the lifetime of MediaStreamTrack and ScriptExecutionContext?  They both appear to be refcounted and so likely have unrelated lifetimes, which means this pointer can become stale.
> 
> ScriptExecutionContext is the document as far as I know and therefore no object can outlive it. Also storing this pointer in a RefPtr can stop a world teardown to take place.n

There's nothing that prevents objects from outliving their document.  Imagine a document with an iframe.  The parent documents can get a JS reference to a MediaStreamTrack object from the subframe and then destroy the subframe.  Now this will be a stale pointer.
Comment 9 Adam Barth 2012-06-29 07:50:07 PDT
Comment on attachment 150112 [details]
Patch

This patch still contains use-after-free vulnerabilities, as described in the previous comment.  I'm also concerned that there isn't any testing for this patch.
Comment 10 Tommy Widenflycht 2012-07-20 02:22:32 PDT
Created attachment 153453 [details]
Patch
Comment 11 Tommy Widenflycht 2012-07-20 02:24:55 PDT
I hope the latest patch removes all your concerns. I have added a test and MediaStreamTrack is now a ActiveDOMObject.

The patch is on the large size but it isn't easy to split it sensibly.

(In reply to comment #9)
> (From update of attachment 150112 [details])
> This patch still contains use-after-free vulnerabilities, as described in the previous comment.  I'm also concerned that there isn't any testing for this patch.
Comment 12 WebKit Review Bot 2012-07-20 02:25:50 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 13 Adam Barth 2012-07-20 09:14:29 PDT
Comment on attachment 153453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153453&action=review

> Source/Platform/chromium/public/WebMediaStreamSource.h:56
> +        ReadyStateLive = 0,
> +        ReadyStateMuted = 1,
> +        ReadyStateEnded = 2

Should we COMPILE_ASSERT that these enums match the WebCore versions?

> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:112
> +    return LIVE;

SHould we return MUTED or ENDED here?  That seems like it would be safer.

> Tools/DumpRenderTree/chromium/MockWebKitPlatformSupport.cpp:53
> -WebMediaStreamCenter* MockWebKitPlatformSupport::createMediaStreamCenter(WebMediaStreamCenterClient*)
> +WebMediaStreamCenter* MockWebKitPlatformSupport::createMediaStreamCenter(WebMediaStreamCenterClient* client)

Should this return PassOwnPtr?

> Tools/DumpRenderTree/chromium/MockWebMediaStreamCenter.h:39
> +class MockWebMediaStreamCenter : public WebMediaStreamCenter {

This declaration should not be in the WebKit namespace.
Comment 14 Adam Barth 2012-07-20 09:14:53 PDT
Just minor comments.  Thanks for the patch!
Comment 15 Tommy Widenflycht 2012-07-23 02:39:04 PDT
Comment on attachment 153453 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153453&action=review

>> Source/Platform/chromium/public/WebMediaStreamSource.h:56
>> +        ReadyStateEnded = 2
> 
> Should we COMPILE_ASSERT that these enums match the WebCore versions?

We sure should, thought I already had added them. Fixed.

>> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:112
>> +    return LIVE;
> 
> SHould we return MUTED or ENDED here?  That seems like it would be safer.

Done.

>> Tools/DumpRenderTree/chromium/MockWebKitPlatformSupport.cpp:53
>> +WebMediaStreamCenter* MockWebKitPlatformSupport::createMediaStreamCenter(WebMediaStreamCenterClient* client)
> 
> Should this return PassOwnPtr?

Unfortunately it can't since it is a override and the normal, non-mock, implementation in Chromium wants to use scoped_ptr.

>> Tools/DumpRenderTree/chromium/MockWebMediaStreamCenter.h:39
>> +class MockWebMediaStreamCenter : public WebMediaStreamCenter {
> 
> This declaration should not be in the WebKit namespace.

Done.
Comment 16 Tommy Widenflycht 2012-07-23 04:06:52 PDT
Created attachment 153771 [details]
Patch
Comment 17 Adam Barth 2012-07-23 09:07:50 PDT
Comment on attachment 153771 [details]
Patch

Thanks.  I might have added tests to make sure the JavaScript wrappers interact correctly with the garbage collector (as discussed above).  In the test hardness, there's a gc function you can call.  I think there's a gc.js file that has some instructions about how to use it.  In any case, we can add that tests in a followup patch.
Comment 18 WebKit Review Bot 2012-07-23 09:59:47 PDT
Comment on attachment 153771 [details]
Patch

Rejecting attachment 153771 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ommit-queue/Source/WebKit/chromium/third_party/snappy/src --revision 63 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
45>At revision 63.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13326138
Comment 19 Tommy Widenflycht 2012-07-24 04:43:06 PDT
Created attachment 154018 [details]
Patch
Comment 20 Tommy Widenflycht 2012-07-24 04:43:48 PDT
Fixed merge problem.
Comment 21 Tommy Widenflycht 2012-07-24 05:49:07 PDT
(In reply to comment #17)
> (From update of attachment 153771 [details])
> Thanks.  I might have added tests to make sure the JavaScript wrappers interact correctly with the garbage collector (as discussed above).  In the test hardness, there's a gc function you can call.  I think there's a gc.js file that has some instructions about how to use it.  In any case, we can add that tests in a followup patch.

Will definitely do that.
Comment 22 Adam Barth 2012-07-24 10:32:49 PDT
Comment on attachment 154018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154018&action=review

> Source/WebCore/Modules/mediastream/MediaStreamTrack.idl:29
>          Conditional=MEDIA_STREAM,
> +        EventTarget

I think we need to add the ActiveDOMObject attribute to this class because MediaStreamTrack inherits from ActiveDOMObject.  Please feel free to do that in a followup patch.
Comment 23 WebKit Review Bot 2012-07-24 11:33:40 PDT
Comment on attachment 154018 [details]
Patch

Clearing flags on attachment: 154018

Committed r123499: <http://trac.webkit.org/changeset/123499>
Comment 24 WebKit Review Bot 2012-07-24 11:33:46 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Review Bot 2012-07-24 14:14:57 PDT
Re-opened since this is blocked by 92161
Comment 26 Zhenyao Mo 2012-07-24 14:15:58 PDT
This patch broke Android build (compile failure) and are blocking me from rolling webkit, so I am rolling this out for now.

From sievers@: "it might be that ENABLE(MEDIA_STREAM) is not used consistently and we have it off
the constructor is if'defd for MockWebMediaStreamCenter but the call site is not"
Comment 27 Tommy Widenflycht 2012-07-25 02:09:36 PDT
Created attachment 154298 [details]
Patch
Comment 28 Tommy Widenflycht 2012-07-25 02:10:15 PDT
(In reply to comment #22)
> (From update of attachment 154018 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154018&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaStreamTrack.idl:29
> >          Conditional=MEDIA_STREAM,
> > +        EventTarget
> 
> I think we need to add the ActiveDOMObject attribute to this class because MediaStreamTrack inherits from ActiveDOMObject.  Please feel free to do that in a followup patch.

Done.
Comment 29 Tommy Widenflycht 2012-07-25 02:11:18 PDT
Hopefully fixed the Android build problem, this patch now builds properly with ENABLE_MEDIA_STREAM=0.
Comment 30 Adam Barth 2012-07-25 08:36:25 PDT
Comment on attachment 154298 [details]
Patch

Great.  Let's give it a whirl.
Comment 31 WebKit Review Bot 2012-07-25 09:53:19 PDT
Comment on attachment 154298 [details]
Patch

Clearing flags on attachment: 154298

Committed r123627: <http://trac.webkit.org/changeset/123627>
Comment 32 WebKit Review Bot 2012-07-25 09:53:25 PDT
All reviewed patches have been landed.  Closing bug.