Bug 90171 – MediaStream API: Update MediaStreamTrackList to match the specification

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 149941 [details]
Patch

[Tommy Widenflycht]

Created attachment 149951 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adam Barth]

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?

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 150116 [details]
Patch

[Tommy Widenflycht]

Created attachment 150171 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

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?

[Tommy Widenflycht]

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.

[Tommy Widenflycht]

Created attachment 150400 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Adam Barth]

> 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.

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 150400 [details]
Patch

Clearing flags on attachment: 150400

Committed r121691: <http://trac.webkit.org/changeset/121691>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.