Bug 178534 – Add initial implementation for serviceWorker.postMessage()

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

[Chris Dumez]

Add initial implementation for serviceWorker.postMessage():
- https://w3c.github.io/ServiceWorker/#service-worker-postmessage

[Chris Dumez]

Created attachment 324289 [details]
WIP Patch

[Chris Dumez]

Created attachment 324303 [details]
WIP Patch

[Chris Dumez]

Created attachment 324560 [details]
WIP Patch

[Chris Dumez]

Created attachment 324573 [details]
WIP Patch

[Chris Dumez]

Created attachment 324594 [details]
WIP Patch

[Build Bot]

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


ERROR: Source/WebCore/workers/service/ServiceWorker.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 22 files


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

[Chris Dumez]

Created attachment 324614 [details]
WIP Patch

[Chris Dumez]

Created attachment 324630 [details]
Patch

[Chris Dumez]

Created attachment 324631 [details]
Patch

[Chris Dumez]

Created attachment 324634 [details]
Patch

[Chris Dumez]

Created attachment 324636 [details]
Patch

[Chris Dumez]

Created attachment 324639 [details]
Patch

[Chris Dumez]

Created attachment 324702 [details]
Patch

[youenn fablet]

Comment on attachment 324639 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        * bindings/js/JSExtendableMessageEvent.cpp: Copied from Source/WebCore/workers/service/ServiceWorker.cpp.

Probably added, not copied.

> Source/WebCore/bindings/js/JSExtendableMessageEventCustom.cpp:54
> +    return result;

Is there a way to share some code with JSMessageEvent::data?

> Source/WebCore/dom/ScriptExecutionContext.h:114
> +    virtual String origin() const = 0;

Why not using a SecurityOrigin&/SecurityOrigin*?

> Source/WebCore/workers/service/ExtendableMessageEvent.h:57
> +        Vector<RefPtr<MessagePort>> ports;

Would it be difficult to have a Vector<Ref<MessagePort>> instead?

> Source/WebCore/workers/service/ServiceWorker.cpp:68
> +    if (!scriptExecutionContext())

I am not sure we can get both a valid ExecState and a null scriptExecutionContext.
If so, I would add it as a first check.

Can we get a ScriptExecutionContext& as first parameter and get an ExecState from it?
Maybe add an ASSERT.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:84
> +        m_controller = ServiceWorker::create(*context, context->selectedServiceWorkerIdentifier());

Can we get a controller whose identifier is not context selectedServiceWorkerIdentifier?

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:109
> +        serviceWorkerGlobalScope.dispatchEvent(ExtendableMessageEvent::create(WTFMove(ports), WTFMove(message), sourceOrigin));

I guess sourceOrigin might be different from the origin of the service worker, like in the case of data URL iframes.
If I am wrong, we might not even need to pass the origin.
If the origin cannot be any random origin, maybe we should assert somehow that this is in a restricted set.
I probably need to read the spec.

> Source/WebCore/workers/service/context/ServiceWorkerThread.h:43
> +using MessagePortChannelArray = Vector<std::unique_ptr<MessagePortChannel>, 1>;

Should probably be UniqueRef if possible.

> Source/WebCore/workers/service/context/ServiceWorkerThread.h:56
> +    WEBCORE_EXPORT void postMessageToServiceWorkerGlobalScope(Ref<SerializedScriptValue>&&, std::unique_ptr<MessagePortChannelArray>&&, const String& sourceOrigin);

Here as well.

> Source/WebKit/WebProcess/Storage/ServiceWorkerContextManager.cpp:76
> +    auto* workerThread = m_workerThreadMap.get(serviceWorkerIdentifier);

Should we protect for a serviceWorkerIdentifier being zero?
Even more valid for startFetch above.

[Chris Dumez]

Comment on attachment 324639 [details]
Patch

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

>> Source/WebCore/bindings/js/JSExtendableMessageEventCustom.cpp:54
>> +    return result;
> 
> Is there a way to share some code with JSMessageEvent::data?

I'll see what I can do.

>> Source/WebCore/dom/ScriptExecutionContext.h:114
>> +    virtual String origin() const = 0;
> 
> Why not using a SecurityOrigin&/SecurityOrigin*?

This is a pre-existing method on Document / WorkerGlobalScope is is used for the self.origin Web API. This is why it currently returns a String. Also note that I need the origin as a String currently since it is used to populate the messageEvent.source attribute.

>> Source/WebCore/workers/service/ExtendableMessageEvent.h:57
>> +        Vector<RefPtr<MessagePort>> ports;
> 
> Would it be difficult to have a Vector<Ref<MessagePort>> instead?

Yes. This is a long-standing issue with our bindings converters.

>> Source/WebCore/workers/service/ServiceWorker.cpp:68
>> +    if (!scriptExecutionContext())
> 
> I am not sure we can get both a valid ExecState and a null scriptExecutionContext.
> If so, I would add it as a first check.
> 
> Can we get a ScriptExecutionContext& as first parameter and get an ExecState from it?
> Maybe add an ASSERT.

Ok.

>> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:84
>> +        m_controller = ServiceWorker::create(*context, context->selectedServiceWorkerIdentifier());
> 
> Can we get a controller whose identifier is not context selectedServiceWorkerIdentifier?

I think this ways:
1. Register service worker A
2. Access container.controller
3. Register service worker B
4. Access container.controller

>> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:109
>> +        serviceWorkerGlobalScope.dispatchEvent(ExtendableMessageEvent::create(WTFMove(ports), WTFMove(message), sourceOrigin));
> 
> I guess sourceOrigin might be different from the origin of the service worker, like in the case of data URL iframes.
> If I am wrong, we might not even need to pass the origin.
> If the origin cannot be any random origin, maybe we should assert somehow that this is in a restricted set.
> I probably need to read the spec.

No, the source origin is not necessarily the same as the worker's origin. It is the origin of the calling script.

>> Source/WebCore/workers/service/context/ServiceWorkerThread.h:43
>> +using MessagePortChannelArray = Vector<std::unique_ptr<MessagePortChannel>, 1>;
> 
> Should probably be UniqueRef if possible.

This is a pre-existing WebCore type. I am merely forward declaring.

>> Source/WebCore/workers/service/context/ServiceWorkerThread.h:56
>> +    WEBCORE_EXPORT void postMessageToServiceWorkerGlobalScope(Ref<SerializedScriptValue>&&, std::unique_ptr<MessagePortChannelArray>&&, const String& sourceOrigin);
> 
> Here as well.

No, as mentioned in an earlier patch, channels can be null.

>> Source/WebKit/WebProcess/Storage/ServiceWorkerContextManager.cpp:76
>> +    auto* workerThread = m_workerThreadMap.get(serviceWorkerIdentifier);
> 
> Should we protect for a serviceWorkerIdentifier being zero?
> Even more valid for startFetch above.

It would hit an assertion in HashMap.

[Chris Dumez]

Created attachment 324713 [details]
Patch

[Chris Dumez]

Comment on attachment 324713 [details]
Patch

Clearing flags on attachment: 324713

Committed r223922: <https://trac.webkit.org/changeset/223922>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35568649>