Created attachment 149941 [details] Patch

Created attachment 149951 [details] Patch

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

Created attachment 150116 [details] Patch

Created attachment 150171 [details] Patch

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

Created attachment 150400 [details] Patch

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

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

> 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 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 on attachment 150400 [details] Patch Clearing flags on attachment: 150400 Committed r121691: <http://trac.webkit.org/changeset/121691>

All reviewed patches have been landed. Closing bug.