Bug 90171

Summary: MediaStream API: Update MediaStreamTrackList to match the specification
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: WebCore Misc.Assignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric.carlson, feature-media-reviews, fishd, gns, jamesr, ojan, philn, rakuco, tkent+wkapi, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 56459    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-03 none

Description Tommy Widenflycht 2012-06-28 06:39:04 PDT
The latest update to the specification added add and remove methods with corresponding callbacks. The callbacks can be triggered both from JS and from the platform layer.
Comment 1 Tommy Widenflycht 2012-06-28 06:52:09 PDT
Created attachment 149941 [details]
Patch
Comment 2 Tommy Widenflycht 2012-06-28 07:26:26 PDT
Created attachment 149951 [details]
Patch
Comment 3 WebKit Review Bot 2012-06-28 08:43:47 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 4 Adam Barth 2012-06-28 09:38:08 PDT
Comment on attachment 149951 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaStreamTrackList.h:67
> +    MediaStream* m_owner;

What controls the relation between the lifetimes of MediaStreamTrackList and MediaStream?
Comment 5 Tommy Widenflycht 2012-06-29 01:30:59 PDT
Comment on attachment 149951 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/MediaStreamTrackList.h:67
>> +    MediaStream* m_owner;
> 
> What controls the relation between the lifetimes of MediaStreamTrackList and MediaStream?

A MediaStream owns two track lists and the lifetime should be identical. Writing this I realize that that isn't necessarily true. Will fix.
Comment 6 Tommy Widenflycht 2012-06-29 02:09:55 PDT
Created attachment 150116 [details]
Patch
Comment 7 Tommy Widenflycht 2012-06-29 07:23:26 PDT
Created attachment 150171 [details]
Patch
Comment 8 Adam Barth 2012-06-29 08:00:23 PDT
Comment on attachment 150171 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaStreamTrackEvent.h:46
> +    virtual const AtomicString& interfaceName() const;

nit: We usually use the OVERRIDE keyword here.

> Source/WebCore/platform/mediastream/MediaStreamCenter.h:72
> +    void doAddMediaStreamTrack(MediaStreamDescriptor*, MediaStreamComponent*);
> +    void doRemoveMediaStreamTrack(MediaStreamDescriptor*, MediaStreamComponent*);

We should probably skip the "do" prefix.  That doesn't really add anything to the name.  addMediaStreamTrack means that we should do that.
Comment 9 Adam Barth 2012-06-29 08:31:01 PDT
Comment on attachment 150171 [details]
Patch

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

Please add a test that shows that the use-after-free doesn't exist and that shows that the audio and video tracks get passivated at a predictable time.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:113
>  MediaStream::~MediaStream()
>  {
>      m_descriptor->setOwner(0);
> +    m_audioTracks->detachOwner();
> +    m_videoTracks->detachOwner();
>  }

Is MediaStream::~MediaStream called at a predictable time?  If JavaScript has a reference to m_audioTracks (for example), then JavaScript can detect whether detachOwner() has been called on the audio track.  That's a problem if MediaStream::~MediaStream is called at the whim of the JavaScript garbage collector.  We don't want to expose details of the garbage collector to JavaScript because that will make it hard for us to improve the garbage collector in the future (as we might break web sites).  Instead, we need to call these functions at a predictable time.  For example, does MediaStream have a state machine that reaches some sort of "closed" or "stopped" state?  If so, that's likely the appropriate time to close/stop/passivate these objects too.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:150
> +void MediaStream::doAddTrack(MediaStreamComponent* component)

doAddTrack -> addTrack

> Source/WebCore/Modules/mediastream/MediaStream.cpp:165
> +void MediaStream::doRemoveTrack(MediaStreamComponent* component)

doRemoveTrack -> removeTrack

> Source/WebCore/Modules/mediastream/MediaStream.cpp:185
> +    for (unsigned i = 0; i < trackList->length(); ++i) {
> +        RefPtr<MediaStreamTrack> track = trackList->item(i);
> +        if (track->component() == component) {
> +            ExceptionCode ec = 0;
> +            trackList->remove(track, ec);
> +            ASSERT(!ec);
> +            break;
> +        }
> +    }

Should MediaStreamTrack have a remove(MediaStreamComponent*) method that does this work?

> Source/WebCore/Modules/mediastream/MediaStreamTrackList.h:35
> -class MediaStreamTrackList : public RefCounted<MediaStreamTrackList> {
> +class MediaStreamTrackList : public RefCounted<MediaStreamTrackList>, public EventTarget {

MediaStreamTrackList needs to be an ActiveDOMObject and should stop firing events after it gets a stop() call.  As written, this patch has a use-after-free vulnerability because m_context can become stale.

> Source/WebCore/Modules/mediastream/MediaStreamTrackList.h:69
> +    MediaStream* m_owner;

I would add a comment here that says that this pointer can be 0.

> Source/WebCore/Modules/mediastream/MediaStreamTrackList.idl:27
>      interface [

Should this be an ArrayClass?
Comment 10 Tommy Widenflycht 2012-07-02 04:32:26 PDT
Comment on attachment 150171 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:113
>>  }
> 
> Is MediaStream::~MediaStream called at a predictable time?  If JavaScript has a reference to m_audioTracks (for example), then JavaScript can detect whether detachOwner() has been called on the audio track.  That's a problem if MediaStream::~MediaStream is called at the whim of the JavaScript garbage collector.  We don't want to expose details of the garbage collector to JavaScript because that will make it hard for us to improve the garbage collector in the future (as we might break web sites).  Instead, we need to call these functions at a predictable time.  For example, does MediaStream have a state machine that reaches some sort of "closed" or "stopped" state?  If so, that's likely the appropriate time to close/stop/passivate these objects too.

Yes, MediaStreams can be stopped and I have passified the track lists there as well as requested.

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:150
>> +void MediaStream::doAddTrack(MediaStreamComponent* component)
> 
> doAddTrack -> addTrack

Done.

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:165
>> +void MediaStream::doRemoveTrack(MediaStreamComponent* component)
> 
> doRemoveTrack -> removeTrack

Done.

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:185
>> +    }
> 
> Should MediaStreamTrack have a remove(MediaStreamComponent*) method that does this work?

MediaStreamTrackList should. Fixed.

>> Source/WebCore/Modules/mediastream/MediaStreamTrackEvent.h:46
>> +    virtual const AtomicString& interfaceName() const;
> 
> nit: We usually use the OVERRIDE keyword here.

Done.

>> Source/WebCore/Modules/mediastream/MediaStreamTrackList.h:35
>> +class MediaStreamTrackList : public RefCounted<MediaStreamTrackList>, public EventTarget {
> 
> MediaStreamTrackList needs to be an ActiveDOMObject and should stop firing events after it gets a stop() call.  As written, this patch has a use-after-free vulnerability because m_context can become stale.

Done.

>> Source/WebCore/Modules/mediastream/MediaStreamTrackList.h:69
>> +    MediaStream* m_owner;
> 
> I would add a comment here that says that this pointer can be 0.

Done.

>> Source/WebCore/Modules/mediastream/MediaStreamTrackList.idl:27
>>      interface [
> 
> Should this be an ArrayClass?

Don't know what that means, and I couldn't find any usage of that keyword.

>> Source/WebCore/platform/mediastream/MediaStreamCenter.h:72
>> +    void doRemoveMediaStreamTrack(MediaStreamDescriptor*, MediaStreamComponent*);
> 
> We should probably skip the "do" prefix.  That doesn't really add anything to the name.  addMediaStreamTrack means that we should do that.

Done.
Comment 11 Tommy Widenflycht 2012-07-02 05:08:59 PDT
Created attachment 150400 [details]
Patch
Comment 12 WebKit Review Bot 2012-07-02 06:18:34 PDT
Comment on attachment 150400 [details]
Patch

Attachment 150400 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13130147

New failing tests:
fast/loader/loadInProgress.html
Comment 13 WebKit Review Bot 2012-07-02 06:18:41 PDT
Created attachment 150413 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 14 Adam Barth 2012-07-02 09:27:47 PDT
> fast/loader/loadInProgress.html

Sorry about this failure.  This is a result of us migrating the bot to a new hosting provider and not related to your patch.
Comment 15 Adam Barth 2012-07-02 09:31:18 PDT
Comment on attachment 150400 [details]
Patch

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

Thanks!

> Source/WebCore/Modules/mediastream/MediaStreamTrackList.cpp:128
> +    size_t index = notFound;
> +    for (unsigned i = 0; i < m_trackVector.size(); ++i) {
> +        if (m_trackVector[i]->component() == component) {
> +            index = i;
> +            break;
> +        }
> +    }

I would have put this code in m_trackVector, but I'm not sure it matters.
Comment 16 WebKit Review Bot 2012-07-02 10:24:41 PDT
Comment on attachment 150400 [details]
Patch

Clearing flags on attachment: 150400

Committed r121691: <http://trac.webkit.org/changeset/121691>
Comment 17 WebKit Review Bot 2012-07-02 10:24:47 PDT
All reviewed patches have been landed.  Closing bug.