[webkit-reviews] review denied: [Bug 90171] MediaStream API: Update MediaStreamTrackList to match the specification : [Attachment 150171] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 90171: MediaStream API: Update MediaStreamTrackList to match the
specification
https://bugs.webkit.org/show_bug.cgi?id=90171

Attachment 150171: Patch
https://bugs.webkit.org/attachment.cgi?id=150171&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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?