Bug 85191 - MediaStream should not be an ActiveDOMObject
: MediaStream should not be an ActiveDOMObject
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
  Show dependency treegraph
Reported: 2012-04-30 07:16 PST by
Modified: 2012-05-07 08:02 PST (History)

Proposed patch (6.13 KB, patch)
2012-04-30 07:22 PST, Adam Bergkvist
abarth: review+
Review Patch | Details | Formatted Diff | Diff
Patch for landing (8.17 KB, patch)
2012-05-07 06:21 PST, Adam Bergkvist
no flags Review Patch | Details | Formatted Diff | Diff


You need to log in before you can comment on or make changes to this bug.

Description From 2012-04-30 07:16:03 PST
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 From 2012-04-30 07:22:26 PST -------
Created an attachment (id=139448) [details]
Proposed patch
------- Comment #2 From 2012-05-02 02:44:17 PST -------
After discussing this with Adam I agree that this is the best solution.
------- Comment #3 From 2012-05-02 09:34:22 PST -------
(From update of attachment 139448 [details])
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 From 2012-05-03 04:17:38 PST -------
Thank you. I'll update the ChangeLog before committing.
------- Comment #5 From 2012-05-04 07:54:28 PST -------
Will you be committing this patch soon?
------- Comment #6 From 2012-05-07 01:18:51 PST -------
I'll commit it today.
------- Comment #7 From 2012-05-07 06:21:25 PST -------
Created an attachment (id=140518) [details]
Patch for landing
------- Comment #8 From 2012-05-07 08:01:59 PST -------
(From update of attachment 140518 [details])
Clearing flags on attachment: 140518

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