Created attachment 149955 [details] Patch

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 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 on attachment 149955 [details] Patch Attachment 149955 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13106154

Created attachment 150112 [details] Patch

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

Created attachment 153453 [details] Patch

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.

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

Just minor comments. Thanks for the patch!

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.

Created attachment 153771 [details] Patch

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

Created attachment 154018 [details] Patch

Fixed merge problem.

(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 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 on attachment 154018 [details] Patch Clearing flags on attachment: 154018 Committed r123499: <http://trac.webkit.org/changeset/123499>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 92161

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"

Created attachment 154298 [details] Patch

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

Hopefully fixed the Android build problem, this patch now builds properly with ENABLE_MEDIA_STREAM=0.

Comment on attachment 154298 [details] Patch Great. Let's give it a whirl.

Comment on attachment 154298 [details] Patch Clearing flags on attachment: 154298 Committed r123627: <http://trac.webkit.org/changeset/123627>

All reviewed patches have been landed. Closing bug.