Bug 85191 – MediaStream should not be an ActiveDOMObject

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Bergkvist]

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.

[Adam Bergkvist]

Created attachment 139448 [details]
Proposed patch

[Tommy Widenflycht]

After discussing this with Adam I agree that this is the best solution.

[Adam Barth]

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.

[Adam Bergkvist]

Thank you. I'll update the ChangeLog before committing.

[Tommy Widenflycht]

Will you be committing this patch soon?

[Adam Bergkvist]

I'll commit it today.

[Adam Bergkvist]

Created attachment 140518 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 140518 [details]
Patch for landing

Clearing flags on attachment: 140518

Committed r116311: <http://trac.webkit.org/changeset/116311>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.