Bug 112350 – [chromium] MediaStream API: Modify WebMediaStream::ExtraData to have a owner()

• Component: WebKit API
• Status: RESOLVED WONTFIX

[Tommy Widenflycht]

[chromium] MediaStream API: Modify WebMediaStream::ExtraData to have a owner()

[Tommy Widenflycht]

Created attachment 193125 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Tommy Widenflycht]

Darin and Adam,
How do you feel regarding this modification of the ExtraData pattern?

It would solve a circular reference problem that the chromium port is having; where the ExtraData descendant gets a callback from e.g. the network stack and needs to modify its related object. If it keeps a copy then the objects can never be cleaned away.

 If it's OK I'll modify a few more classes to have the same functionality.

[Adam Barth]

I'm not sure I understand the problem this patch is solving.  Can you give me a more concrete example or point me towards the chromium code that's having the trouble?

[Adam Barth]

Comment on attachment 193125 [details]
Patch

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

> Source/Platform/chromium/public/WebMediaStream.h:57
> +        WebCore::MediaStreamDescriptor* m_owner;

This worries me because there's no connection between the lifetime of the m_owner and the ExtraData instance.  The ExtraDataContainer is RefCounted, which means it could outlive the m_owner.

[Darin Fisher (:fishd, Google)]

Comment on attachment 193125 [details]
Patch

Yeah, I need to understand better why this is necessary.  I'd imagine there may be other solutions, but without more context, it is hard to suggest any.  Maybe a link to the Chromium CL that might use this API change would help.

[Tommy Widenflycht]

        +------------+
        |WebCoreObject|
        +------------+
               |
               |
               *
        +------------+
        |WebKitWrapper|
        +------------+
               *
               |
               |
          +-------+
          |ExtraData|
          +-------+
               ^
               |
==================================== WebKit/chromium boundary
               |
         +----------+
         |SomeListener|
         +----------+

The main issue (for me that is) is that chromiums WebKit platform pattern is tailored for data objects, not "intelligent" objects.

Here's a breakdown:

In the above picture (understandable I hope) --> means inheritance and *-- means ownership.

An event happens and the SomeLister is notified. As an effect of this event the state of the WebCoreObject has to be notified. Here our problems starts because there's no way we can do that because SomeListener can't have the WebKitWrapper as a member. If it were do keep one it would create a circular memory reference and teardown of the world can't take place properly.

What would work is if SomeListener inherits from ExtraData (because there are some data to be stored as well) AND has a way to get its WebKitWrapper when necessary which is what this patch is all about.

For the MediaStream major classes we have another pattern that works really well (Handler/HandlerClient) but it would be a major overhead if all objects need to switch to this pattern. The ideal case would be if all major classes use the Handler/HandlerClient pattern and the minor classes where just a state needs to be changed can use the proposed pattern.

[Tommy Widenflycht]

Comment on attachment 193125 [details]
Patch

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

>> Source/Platform/chromium/public/WebMediaStream.h:57
>> +        WebCore::MediaStreamDescriptor* m_owner;
> 
> This worries me because there's no connection between the lifetime of the m_owner and the ExtraData instance.  The ExtraDataContainer is RefCounted, which means it could outlive the m_owner.

yes, that right but the ExtraDataContainer is only visible in the .cpp file and under complete control which should leave no loopholes. Unless someone later modifies the code and exposes the ExtraDataContainer of course...

[Tommy Widenflycht]

Ping...