Bug 112350 - [chromium] MediaStream API: Modify WebMediaStream::ExtraData to have a owner()
Summary: [chromium] MediaStream API: Modify WebMediaStream::ExtraData to have a owner()
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Tommy Widenflycht
URL:
Keywords:
Depends on:
Blocks: 56459
  Show dependency treegraph
 
Reported: 2013-03-14 08:12 PDT by Tommy Widenflycht
Modified: 2013-04-08 16:50 PDT (History)
11 users (show)

See Also:


Attachments
Patch (5.43 KB, patch)
2013-03-14 08:12 PDT, Tommy Widenflycht
benjamin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tommy Widenflycht 2013-03-14 08:12:00 PDT
[chromium] MediaStream API: Modify WebMediaStream::ExtraData to have a owner()
Comment 1 Tommy Widenflycht 2013-03-14 08:12:45 PDT
Created attachment 193125 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-14 08:16:05 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Tommy Widenflycht 2013-03-14 08:20:11 PDT
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.
Comment 4 Adam Barth 2013-03-15 13:51:05 PDT
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?
Comment 5 Adam Barth 2013-03-15 13:54:02 PDT
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.
Comment 6 Darin Fisher (:fishd, Google) 2013-03-15 20:30:07 PDT
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.
Comment 7 Tommy Widenflycht 2013-03-18 03:25:36 PDT
        +------------+
        |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.
Comment 8 Tommy Widenflycht 2013-03-18 03:29:27 PDT
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...
Comment 9 Tommy Widenflycht 2013-03-21 08:17:36 PDT
Ping...