Bug 27337 – Add a getter in MessagePortChannel for the PlatformMessagePortChannel

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Abd-El-Malek]

Platform code might need to get access to its PlatformMessagePortChannel object from a MessagePortChannel.  i.e. Chromium's port needs it when sending a message port, so it can get the unique identifier for that port.

[John Abd-El-Malek]

Created attachment 32843 [details]
Proposed patch

[David Levin]

Comment on attachment 32843 [details]
Proposed patch

It would be nice to mention in the bug why this is useful.

> Index: WebCore/dom/MessagePortChannel.h
> +        // Getter for m_channel.

I'd remove this comment as it doesn't really seem to add any information.

> +        PassRefPtr<PlatformMessagePortChannel> channel();

Since this method isn't really passing the ref count to the caller (it retains the ref counted pointer), just return PlatformMessagePortChannel* (instead of a PassRefPtr).

Also, why not just define this inline?
     return m_channel.get();

[Andrew Wilson]

I'm also curious about why this is necessary. The whole point behind MessagePortChannel is to create a wrapper around PlatformMessagePortChannel to make sure that you never have a raw PlatformMessagePortChannel floating around that might not get close()'d.

Exposing a getter for PlatformMessagePortChannel seems dangerous (it's too easy to inadvertently create leaks) and violates the encapsulation that MessagePortChannel is intended to provide.

[John Abd-El-Malek]

(In reply to comment #2)
> (From update of attachment 32843 [details])
> It would be nice to mention in the bug why this is useful.

I wrote a small description above (i..e re unique identifier), is that not enough?

> 
> > Index: WebCore/dom/MessagePortChannel.h
> > +        // Getter for m_channel.
> 
> I'd remove this comment as it doesn't really seem to add any information.

ah, I thought it was necessary if the implementation was in a different file (which was necessary if I was returning a PassRefPtr, but as you point out below, that's not necessary).

> 
> > +        PassRefPtr<PlatformMessagePortChannel> channel();
> 
> Since this method isn't really passing the ref count to the caller (it retains
> the ref counted pointer), just return PlatformMessagePortChannel* (instead of a
> PassRefPtr).

ah, I still get confused sometimes with all the RefPtr objects.  Thanks, I only need a pointer.  I wasn't sure if it's ok to pass around raw pointers of ref counted objects.
> 
> Also, why not just define this inline?
>      return m_channel.get();

this is now possible once there are PassRefPtr (since the PlatformMessagePortChannel header isn't included here).

[John Abd-El-Malek]

(In reply to comment #3)
> I'm also curious about why this is necessary. The whole point behind
> MessagePortChannel is to create a wrapper around PlatformMessagePortChannel to
> make sure that you never have a raw PlatformMessagePortChannel floating around
> that might not get close()'d.
> 
> Exposing a getter for PlatformMessagePortChannel seems dangerous (it's too easy
> to inadvertently create leaks) and violates the encapsulation that
> MessagePortChannel is intended to provide.

I'm going to upload the chromium port changes soon, which will make things clearer.

The reason this is needed, which I hinted to above, is that when sending a message port in a postMessage call, PlatformMessagePortChannel   just gets a MessagePortChannel::EventData that contains a MessagePortChannel object.  In the single process case, that's enough.  However for Chrome, when the object needs to be recreated in a different process, more information from the platform implementation is needed (namely, the unique identifier for this message port channel that will be sent to a different process).

Checking in this patch isn't time critical (thanks for the quick review though David!), so committing it can wait until you guys see the chromium platform implementation.

[John Abd-El-Malek]

Created attachment 32886 [details]
new patch

[Andrew Wilson]

Ah, that makes sense. 

Sorry I missed your earlier comment in the bug - I was rushing off to an interview and wanted to get my two cents in before going.

And on further consideration, it's not that dangerous to have a bare reference to a PMPC, since the PMPC will get closed whenever the MPC is freed so there's no danger of lingering refs.

[David Levin]

Comment on attachment 32886 [details]
new patch

I missed your earlier comment too.

> ah, I still get confused sometimes with all the RefPtr objects.  Thanks, I only
> need a pointer.  I wasn't sure if it's ok to pass around raw pointers of ref
> counted objects.

Actually, I wasn't aware of this guideline until recently, but it is stated well here: http://webkit.org/coding/RefPtr.html

"If a function’s result is an object, but ownership is not being transferred, the result should be a raw pointer. This includes most getter functions."

[David Levin]

Assigned to levin for landing.

[David Levin]

Committed as http://trac.webkit.org/changeset/45996

[John Abd-El-Malek]

(In reply to comment #10)
> Committed as http://trac.webkit.org/changeset/45996

Thanks!