Bug 178794 – Add initial support for serviceWorkerClient.postMessage()

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

[Chris Dumez]

Add initial support for serviceWorkerClient.postMessage():
- https://w3c.github.io/ServiceWorker/#client-postmessage

[Chris Dumez]

Created attachment 324830 [details]
WIP Patch

[Chris Dumez]

Created attachment 324833 [details]
WIP Patch

[Build Bot]

Attachment 324833 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:444:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 324889 [details]
WIP Patch

[Chris Dumez]

Created attachment 324899 [details]
WIP Patch

[Build Bot]

Attachment 324899 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/ServiceWorkerClient.h:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/Document.cpp:444:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 324917 [details]
WIP Patch

[Build Bot]

Attachment 324917 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:444:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 324926 [details]
WIP Patch

[Build Bot]

Attachment 324926 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:444:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 324929 [details]
WIP Patch

Ok. Will work on cleaning this up but communication works in both directions now.

[Build Bot]

Attachment 324929 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:444:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 324947 [details]
WIP Patch

[Build Bot]

Attachment 324947 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:444:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 325017 [details]
WIP Patch

[Chris Dumez]

Created attachment 325019 [details]
WIP Patch

[Build Bot]

Attachment 325019 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:444:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 325077 [details]
WIP Patch

[Build Bot]

Attachment 325077 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/Document.cpp:444:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 325097 [details]
WIP Patch

[Chris Dumez]

Created attachment 325099 [details]
Patch

[youenn fablet]

Comment on attachment 325099 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        via postMessage().

Cool!

> Source/WebCore/dom/Document.cpp:445
> +    static NeverDestroyed<HashMap<Identifier, Document*>> documents;

We should be able to do HashMap<Identifier, reference_wrapper> at some point.

> Source/WebCore/dom/Document.h:350
> +    using Identifier = uint64_t;

Does Identifier really help?
If it helps, should we try to ensure that no implicit casting happens between uint64_t and Identifier?

> Source/WebCore/workers/service/ServiceWorkerClient.cpp:71
> +    Vector<RefPtr<MessagePort>> ports;

Might want Vector<Ref> at some point.

> Source/WebCore/workers/service/ServiceWorkerClient.cpp:87
> +    callOnMainThread([message = message.releaseReturnValue(), destinationIdentifier = m_identifier, sourceIdentifier, sourceOrigin = context.origin().isolatedCopy()] () mutable {

Probably fine or even better using callOnMainThread here.
I think the expected abstraction is postMessageToWorkerObject though.
Maybe we should do some refactoring.

> Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:34
> +    uint64_t scriptExecutionContextIdentifier;

We really have a process id and a per-process environment identifier.
It might be good to have something more generic, so that we can reuse this environment identifier in other places like network loads too.
This can probably be improved later on when we have more concrete plans.

> Source/WebCore/workers/service/context/SWContextManager.h:47
> +        virtual void postMessageToServiceWorkerClient(const ServiceWorkerClientIdentifier& destinationIdentifier, Ref<SerializedScriptValue>&& message, uint64_t sourceServiceWorkerIdentifier, const String& sourceOrigin) = 0;

No need for destinationIdentifier name.

> Source/WebCore/workers/service/server/SWClientConnection.cpp:123
> +    if (!container)

Can this be a real regular case?

> Source/WebCore/workers/service/server/SWClientConnection.cpp:131
> +        source = MessageEventSource { RefPtr<ServiceWorker> { ServiceWorker::create(*destinationDocument, sourceServiceWorkerIdentifier) } };

Every time we receive a message, we will create a new ServiceWorker.
We should reuse them according their ids and if we create one, it should be based on a full ServiceWorkerRegistration.
Can we fix that now?

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1906
> +void ArgumentCoder<ServiceWorkerClientIdentifier>::encode(Encoder& encoder, const ServiceWorkerClientIdentifier& identifier)

The current style is to move this code closer to the definition, i.e. ServiceWorkerClientIdentifier.

[Chris Dumez]

Created attachment 325166 [details]
Patch

[Chris Dumez]

(In reply to youenn fablet from comment #22)
> Comment on attachment 325099 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325099&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        via postMessage().
> 
> Cool!
> 
> > Source/WebCore/dom/Document.cpp:445
> > +    static NeverDestroyed<HashMap<Identifier, Document*>> documents;
> 
> We should be able to do HashMap<Identifier, reference_wrapper> at some point.
> 
> > Source/WebCore/dom/Document.h:350
> > +    using Identifier = uint64_t;
> 
> Does Identifier really help?
> If it helps, should we try to ensure that no implicit casting happens
> between uint64_t and Identifier?

Ok, will drop Identifier alias.

> > Source/WebCore/workers/service/ServiceWorkerClient.cpp:71
> > +    Vector<RefPtr<MessagePort>> ports;
> 
> Might want Vector<Ref> at some point.
> 
> > Source/WebCore/workers/service/ServiceWorkerClient.cpp:87
> > +    callOnMainThread([message = message.releaseReturnValue(), destinationIdentifier = m_identifier, sourceIdentifier, sourceOrigin = context.origin().isolatedCopy()] () mutable {
> 
> Probably fine or even better using callOnMainThread here.
> I think the expected abstraction is postMessageToWorkerObject though.
> Maybe we should do some refactoring.
> 
> > Source/WebCore/workers/service/ServiceWorkerClientIdentifier.h:34
> > +    uint64_t scriptExecutionContextIdentifier;
> 
> We really have a process id and a per-process environment identifier.
> It might be good to have something more generic, so that we can reuse this
> environment identifier in other places like network loads too.
> This can probably be improved later on when we have more concrete plans.
> 
> > Source/WebCore/workers/service/context/SWContextManager.h:47
> > +        virtual void postMessageToServiceWorkerClient(const ServiceWorkerClientIdentifier& destinationIdentifier, Ref<SerializedScriptValue>&& message, uint64_t sourceServiceWorkerIdentifier, const String& sourceOrigin) = 0;
> 
> No need for destinationIdentifier name.
> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:123
> > +    if (!container)
> 
> Can this be a real regular case?
> 
> > Source/WebCore/workers/service/server/SWClientConnection.cpp:131
> > +        source = MessageEventSource { RefPtr<ServiceWorker> { ServiceWorker::create(*destinationDocument, sourceServiceWorkerIdentifier) } };
> 
> Every time we receive a message, we will create a new ServiceWorker.
> We should reuse them according their ids and if we create one, it should be
> based on a full ServiceWorkerRegistration.
> Can we fix that now?

This is not an issue yet because we currently always reuse the controller. At some point, it'd be good to keep a list of ServiceWorkers somewhere I guess.

> 
> > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1906
> > +void ArgumentCoder<ServiceWorkerClientIdentifier>::encode(Encoder& encoder, const ServiceWorkerClientIdentifier& identifier)
> 
> The current style is to move this code closer to the definition, i.e.
> ServiceWorkerClientIdentifier.

Can you please clarify this?

[Chris Dumez]

Comment on attachment 325166 [details]
Patch

Clearing flags on attachment: 325166

Committed r224113: <https://trac.webkit.org/changeset/224113>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

> > Every time we receive a message, we will create a new ServiceWorker.
> > We should reuse them according their ids and if we create one, it should be
> > based on a full ServiceWorkerRegistration.
> > Can we fix that now?
> 
> This is not an issue yet because we currently always reuse the controller.
> At some point, it'd be good to keep a list of ServiceWorkers somewhere I
> guess.

Maybe we should add A FIXME and not implement this code path then.

> > 
> > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1906
> > > +void ArgumentCoder<ServiceWorkerClientIdentifier>::encode(Encoder& encoder, const ServiceWorkerClientIdentifier& identifier)
> > 
> > The current style is to move this code closer to the definition, i.e.
> > ServiceWorkerClientIdentifier.
> 
> Can you please clarify this?

Add the encoder/decoder in ServiceWorkerClientIdentifier.h.
See for instance FetchOptions.h

[Chris Dumez]

(In reply to youenn fablet from comment #27)
> > > Every time we receive a message, we will create a new ServiceWorker.
> > > We should reuse them according their ids and if we create one, it should be
> > > based on a full ServiceWorkerRegistration.
> > > Can we fix that now?
> > 
> > This is not an issue yet because we currently always reuse the controller.
> > At some point, it'd be good to keep a list of ServiceWorkers somewhere I
> > guess.
> 
> Maybe we should add A FIXME and not implement this code path then.

I do not agree. It is best to support the feature, even if not as efficient as it could be. Also note that a ServiceWorker is basically just a wrapper around an id and should be cheap to construct.

> 
> > > 
> > > > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1906
> > > > +void ArgumentCoder<ServiceWorkerClientIdentifier>::encode(Encoder& encoder, const ServiceWorkerClientIdentifier& identifier)
> > > 
> > > The current style is to move this code closer to the definition, i.e.
> > > ServiceWorkerClientIdentifier.
> > 
> > Can you please clarify this?
> 
> Add the encoder/decoder in ServiceWorkerClientIdentifier.h.
> See for instance FetchOptions.h

Oh OK, I can do this in a follow-up. I did not know this policy.

[youenn fablet]

> > Maybe we should add A FIXME and not implement this code path then.
> 
> I do not agree. It is best to support the feature, even if not as efficient
> as it could be. Also note that a ServiceWorker is basically just a wrapper
> around an id and should be cheap to construct.

IINM, for every message in that situation, we will have a different JS source object which will be observable from JS. If so, we are not very compliant, right?
Why not simply returning a null source?

If we are testing this code path, it might be good to add test showing the behavior and whether this is expected.

[Chris Dumez]

(In reply to youenn fablet from comment #29)
> > > Maybe we should add A FIXME and not implement this code path then.
> > 
> > I do not agree. It is best to support the feature, even if not as efficient
> > as it could be. Also note that a ServiceWorker is basically just a wrapper
> > around an id and should be cheap to construct.
> 
> IINM, for every message in that situation, we will have a different JS
> source object which will be observable from JS. If so, we are not very
> compliant, right?
> Why not simply returning a null source?
> 
> If we are testing this code path, it might be good to add test showing the
> behavior and whether this is expected.

If the source is null, then you cannot message the source back, which is much worse than getting a different JS wrapper every time.

[Chris Dumez]

(In reply to Chris Dumez from comment #30)
> (In reply to youenn fablet from comment #29)
> > > > Maybe we should add A FIXME and not implement this code path then.
> > > 
> > > I do not agree. It is best to support the feature, even if not as efficient
> > > as it could be. Also note that a ServiceWorker is basically just a wrapper
> > > around an id and should be cheap to construct.
> > 
> > IINM, for every message in that situation, we will have a different JS
> > source object which will be observable from JS. If so, we are not very
> > compliant, right?
> > Why not simply returning a null source?
> > 
> > If we are testing this code path, it might be good to add test showing the
> > behavior and whether this is expected.
> 
> If the source is null, then you cannot message the source back, which is
> much worse than getting a different JS wrapper every time.

Also, if you look at the spec, you'll see that it says to create a new ServiceWorker object. Therefore, it may just be compliant.

[Radar WebKit Bug Importer]

<rdar://problem/35568118>