Bug 83143 – MediaStream API: MediaStreams stops proper cleanup to take place during a page reload.

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

[Tommy Widenflycht]

To fix this I have converted MediaStream and LocalMediaStream to be ActiveDOMObjects.

[Tommy Widenflycht]

Created attachment 135553 [details]
Patch

[Tommy Widenflycht]

I think the main problem was having the ScriptExecutionContext stored in a RefPtr which likely caused a circular reference. However switching to ActiveDOMObjects also gives the automatic calling of the stop() function which is an added bonus.

[Adam Barth]

Comment on attachment 135553 [details]
Patch

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

This looks mostly fine, but I'm going to mark it R- because of the IDL / ActiveDOMObject thing.  I've also left you a couple other things to think about for the next iteration.

> Source/WebCore/Modules/mediastream/LocalMediaStream.h:41
> +    // ActiveDOMObject and idl

Woah.  Should we use [ImplementedAs] to avoid having this one function play both roles?  It's fine if one calls the other, but it's better to do that explicitly rather than having a name collision like this.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:135
> +void MediaStream::stop()
> +{
> +}

If you don't need stop(), you can just use a ContextDestructionObserver rather than ActiveDOMObject.

[Tommy Widenflycht]

Created attachment 135788 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 135553 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/LocalMediaStream.h:41
>> +    // ActiveDOMObject and idl
> 
> Woah.  Should we use [ImplementedAs] to avoid having this one function play both roles?  It's fine if one calls the other, but it's better to do that explicitly rather than having a name collision like this.

Fixed. And I learnt something new today as well :)

>> Source/WebCore/Modules/mediastream/MediaStream.cpp:135
>> +}
> 
> If you don't need stop(), you can just use a ContextDestructionObserver rather than ActiveDOMObject.

MediaStream doesn't but LocalMediaStream does. Removed the empty implementation.

[Adam Barth]

Comment on attachment 135788 [details]
Patch

Thanks.

[WebKit Review Bot]

Comment on attachment 135788 [details]
Patch

Clearing flags on attachment: 135788

Committed r113460: <http://trac.webkit.org/changeset/113460>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.