ServiceWorker: Introduce ExtendableMessageEvent to replace MessageEvent

ServiceWorker: Introduce ExtendableMessageEvent to replace MessageEvent.

The message sent by ServiceWorker.postMessage() method will be
ExtendableMessageEvent instead of MessageEvent. The 'source' attribute
of the event could be Client or ServiceWorker or MessagePort.

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#extendablemessage-event-interface

Chrome side CL: https://codereview.chromium.org/1235803003/
Blink side CL: this

BUG=498596

[xiang, 2015-05-26 02:05:03]

xiang.long@intel.com changed reviewers:
+ bashi@chromium.org, falken@chromium.org, haraken@chromium.org

[xiang, 2015-05-26 02:05:04]

This's a working CL to implement ExtendableMessageEvent.

There's a union type binding issue when I'm using (Client or ServiceWorker or
MessagePort) in IDL:
The generated ClientOrServiceWorkerOrMessagePort class has a
Member<ServiceWorkerClient> m_client member, however as ExtendableMessageEvent
is still outside of the Oilpan Heap, seems we should need Persistent<> to keep
reference of it?

[haraken, 2015-05-26 02:10:04]

> There's a union type binding issue when I'm using (Client or ServiceWorker or
> MessagePort) in IDL:
> The generated ClientOrServiceWorkerOrMessagePort class has a
> Member<ServiceWorkerClient> m_client member, however as ExtendableMessageEvent
> is still outside of the Oilpan Heap, seems we should need Persistent<> to keep
> reference of it?

I'm a bit confused.

ExtendableMessageEvent is still off the heap, but
ClientOrServiceWorkerOrMessagePort is on the heap. So
ClientOrServiceWorkerOrMessagePort can just hold a Memeber to
ServiceWorkerClient, can't it?

BTW, it is unfortunate that ExtendableMessageEvent is speced in a way in which
it doesn't inherit from MessageEvent. A lot of code duplication here again...

[haraken, 2015-05-26 02:11:33]

On 2015/05/26 02:10:04, haraken wrote:
> > There's a union type binding issue when I'm using (Client or ServiceWorker
or
> > MessagePort) in IDL:
> > The generated ClientOrServiceWorkerOrMessagePort class has a
> > Member<ServiceWorkerClient> m_client member, however as
ExtendableMessageEvent
> > is still outside of the Oilpan Heap, seems we should need Persistent<> to
keep
> > reference of it?
> 
> I'm a bit confused.
> 
> ExtendableMessageEvent is still off the heap, but
> ClientOrServiceWorkerOrMessagePort is on the heap. So
> ClientOrServiceWorkerOrMessagePort can just hold a Memeber to
> ServiceWorkerClient, can't it?

Regarding oilpan, you can just try to compile with/without oilpan enabled. If it
compiles, it means you're doing a right thing. (Compile-time checks will cover
almost all oilpan programming errors :)

[xiang, 2015-05-26 02:32:12]

On 2015/05/26 02:11:33, haraken wrote:
> On 2015/05/26 02:10:04, haraken wrote:
> > > There's a union type binding issue when I'm using (Client or ServiceWorker
> or
> > > MessagePort) in IDL:
> > > The generated ClientOrServiceWorkerOrMessagePort class has a
> > > Member<ServiceWorkerClient> m_client member, however as
> ExtendableMessageEvent
> > > is still outside of the Oilpan Heap, seems we should need Persistent<> to
> keep
> > > reference of it?
> > 
> > I'm a bit confused.
> > 
> > ExtendableMessageEvent is still off the heap, but
> > ClientOrServiceWorkerOrMessagePort is on the heap. So
> > ClientOrServiceWorkerOrMessagePort can just hold a Memeber to
> > ServiceWorkerClient, can't it?
> 

The generated ClientOrServiceWorkerOrMessagePort is like below:

class MODULES_EXPORT ClientOrServiceWorkerOrMessagePort final {
    ALLOW_ONLY_INLINE_ALLOCATION();
public:
...
    static ClientOrServiceWorkerOrMessagePort fromClient(ServiceWorkerClient*);
...
    DECLARE_TRACE();

private:
...
    Member<ServiceWorkerClient> m_client;
...
};

Seems it should only exists on stack or embedded in other objects.
If the container is out of heap then the "m_client" is out of heap too?

> Regarding oilpan, you can just try to compile with/without oilpan enabled. If
it
> compiles, it means you're doing a right thing. (Compile-time checks will cover
> almost all oilpan programming errors :)

The build can pass without oilpan enabled. I feel if oilpan enabled the problem
should not exist as ExtendableMessageEvent will in heap too.

[haraken, 2015-05-26 02:40:24]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, yutak@chromium.org

[haraken, 2015-05-26 02:40:25]

+yutak

> > On 2015/05/26 02:10:04, haraken wrote:
> > > > There's a union type binding issue when I'm using (Client or
ServiceWorker
> > or
> > > > MessagePort) in IDL:
> > > > The generated ClientOrServiceWorkerOrMessagePort class has a
> > > > Member<ServiceWorkerClient> m_client member, however as
> > ExtendableMessageEvent
> > > > is still outside of the Oilpan Heap, seems we should need Persistent<>
to
> > keep
> > > > reference of it?
> > > 
> > > I'm a bit confused.
> > > 
> > > ExtendableMessageEvent is still off the heap, but
> > > ClientOrServiceWorkerOrMessagePort is on the heap. So
> > > ClientOrServiceWorkerOrMessagePort can just hold a Memeber to
> > > ServiceWorkerClient, can't it?
> > 
> 
> The generated ClientOrServiceWorkerOrMessagePort is like below:
> 
> class MODULES_EXPORT ClientOrServiceWorkerOrMessagePort final {
>     ALLOW_ONLY_INLINE_ALLOCATION();
> public:
> ...
>     static ClientOrServiceWorkerOrMessagePort
fromClient(ServiceWorkerClient*);
> ...
>     DECLARE_TRACE();
> 
> private:
> ...
>     Member<ServiceWorkerClient> m_client;
> ...
> };
> 
> Seems it should only exists on stack or embedded in other objects.
> If the container is out of heap then the "m_client" is out of heap too?

Ah, you're totally right. In non-oilpan build, that needs to be
PersistentWillBeMember<ServiceWorkerClient>.

Hmm -- it looks non-trivial to fix the issue. Let me chat with bashi-san
offline.


> > Regarding oilpan, you can just try to compile with/without oilpan enabled.
If
> it
> > compiles, it means you're doing a right thing. (Compile-time checks will
cover
> > almost all oilpan programming errors :)
> 
> The build can pass without oilpan enabled. I feel if oilpan enabled the
problem
> should not exist as ExtendableMessageEvent will in heap too.

This seems to be a bug of the GC plugin... If there is an untraced Member (on
non-oilpan builds or oilpan builds), the GC plugin must detect the error.
yuta-san?

[bashi, 2015-05-26 05:40:24]

On 2015/05/26 02:32:12, xiang wrote:
> On 2015/05/26 02:11:33, haraken wrote:
> > On 2015/05/26 02:10:04, haraken wrote:
> > > > There's a union type binding issue when I'm using (Client or
ServiceWorker
> > or
> > > > MessagePort) in IDL:
> > > > The generated ClientOrServiceWorkerOrMessagePort class has a
> > > > Member<ServiceWorkerClient> m_client member, however as
> > ExtendableMessageEvent
> > > > is still outside of the Oilpan Heap, seems we should need Persistent<>
to
> > keep
> > > > reference of it?
> > > 
> > > I'm a bit confused.
> > > 
> > > ExtendableMessageEvent is still off the heap, but
> > > ClientOrServiceWorkerOrMessagePort is on the heap. So
> > > ClientOrServiceWorkerOrMessagePort can just hold a Memeber to
> > > ServiceWorkerClient, can't it?
> > 
> 
> The generated ClientOrServiceWorkerOrMessagePort is like below:
> 
> class MODULES_EXPORT ClientOrServiceWorkerOrMessagePort final {
>     ALLOW_ONLY_INLINE_ALLOCATION();
> public:
> ...
>     static ClientOrServiceWorkerOrMessagePort
fromClient(ServiceWorkerClient*);
> ...
>     DECLARE_TRACE();
> 
> private:
> ...
>     Member<ServiceWorkerClient> m_client;
> ...
> };
> 
> Seems it should only exists on stack or embedded in other objects.
> If the container is out of heap then the "m_client" is out of heap too?

Union containers should only be allocated on stack or in vectors. They are kinds
of temporary objects and aren't intended to outlive one JS callback. In other
words, you shouldn't store union containers in DOM impl classes. Instead, please
add each union member separately. In this case, you might want to add Client,
ServiceWorker and MessagePort as fields of ExtendableMessageEvent, and create a
new ClientOrServiceWorkerOrMessagePort instance every time the getter function
(source()) is called.

I understand this looks redundant and unnecessary, but it is required to make
union containers be efficient for certain use cases (some performance sensitive
callbacks use union containers and we want to reduce overhead of union
containers creation as much as possible).

> 
> > Regarding oilpan, you can just try to compile with/without oilpan enabled.
If
> it
> > compiles, it means you're doing a right thing. (Compile-time checks will
cover
> > almost all oilpan programming errors :)
> 
> The build can pass without oilpan enabled. I feel if oilpan enabled the
problem
> should not exist as ExtendableMessageEvent will in heap too.

[xiang, 2015-07-15 04:29:44]

Thanks for the review.
But I have to close this as I don't have enough time to work on it atm, sorry.