Bug 85191

Summary: MediaStream should not be an ActiveDOMObject
Product: WebKit Reporter: Adam Bergkvist <adam.bergkvist>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Severity: Normal CC: abarth, eric.carlson, feature-media-reviews, ojan, per-erik.brodin, tommyw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
Proposed patch
abarth: review+
Patch for landing none

Description Adam Bergkvist 2012-04-30 07:16:03 PDT
The model with MediaStreamDescriptor and MediaStream (and LocalMediaStream) allows the JavaScript objects (MediaStream and LocalMediaStream) to be cleaned up while the MediaStreamDescriptor lives on to manage the stream in the platform. This happens for example when a URL is created to represent a MediaStream (using createObjectURL). The MediaStreamDescriptor is put into the MediaStreamRegistry and even though the MediaStream object is lost, the URL still works since the descriptor is kept in the registry.

The changes introduced in r113460 turned MediaStream and LocalMediaStream into ActiveDOMObjects. On page reload, LocalMediaStream calls MediaStreamCenter::didStopLocalMediaStream() via its ActiveDOMObject::stop() method. However, when a page reload occurs, the LocalMediaStream object may have been cleaned up already. As a result, the behavior is different since MediaStreamCenter::didStopLocalMediaStream() is called when the LocalMediaStream object exists, but not if only the MediaStreamDescriptor is left. One way to make the behavior consistent would be to call MediaStreamCenter::didStopLocalMediaStream() when the descriptor is cleaned up, cause then we wouldn't be dependent on the LocalMediaStream object being alive. However, calling MediaStreamCenter::didStopLocalMediaStream() might not be the correct thing to do when all references to the descriptor are lost since there can be MediaStream objects constructed from the tracks of the LocalMediaStream that should continue to work. MediaStreamCenter::didStopLocalMediaStream() was intended for LocalMediaStream.stop() which is used to revoke access to devices; that should not necessarily happen when the descriptor of a LocalMediaStream is cleaned up. If it's necessary for some ports to signal to the platform that a MediaStreamDescriptor is cleaned up, then I would suggest adding a new function, willDestroyMediaStreamDescriptor(), to the MediaStreamCenter interface.
Comment 1 Adam Bergkvist 2012-04-30 07:22:26 PDT
Created attachment 139448 [details]
Proposed patch
Comment 2 Tommy Widenflycht 2012-05-02 02:44:17 PDT
After discussing this with Adam I agree that this is the best solution.
Comment 3 Adam Barth 2012-05-02 09:34:22 PDT
Comment on attachment 139448 [details]
Proposed patch

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

I didn't quite follow all the details in Comment #0, but if you and Tommy agree that this is the right thing to do, I'm willing to believe it.  :)

> Source/WebCore/ChangeLog:8
> +        Made MediaStream a ContextDestructionObserver instead.

Consider copying the text from comment #0 in the but to the ChangeLog.  That's really helpful information and it would be nice to have it in the ChangeLog rather than a click away in the bug.
Comment 4 Adam Bergkvist 2012-05-03 04:17:38 PDT
Thank you. I'll update the ChangeLog before committing.
Comment 5 Tommy Widenflycht 2012-05-04 07:54:28 PDT
Will you be committing this patch soon?
Comment 6 Adam Bergkvist 2012-05-07 01:18:51 PDT
I'll commit it today.
Comment 7 Adam Bergkvist 2012-05-07 06:21:25 PDT
Created attachment 140518 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-05-07 08:01:59 PDT
Comment on attachment 140518 [details]
Patch for landing

Clearing flags on attachment: 140518

Committed r116311: <http://trac.webkit.org/changeset/116311>
Comment 9 WebKit Review Bot 2012-05-07 08:02:14 PDT
All reviewed patches have been landed.  Closing bug.