Implement ServiceWorker::postMessage() [Blink]

Implement ServiceWorker::postMessage()

This allows the registering page, or pages loaded via the worker,
to send arbitrary messages to the worker script.

BUG=350103
R=abarth@chromium.org, adamk@chromium.org, kinuko@chromium.org, marja@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169668

[jsbell, 2014-03-10 21:53:04]

This is ready for an initial look. Layout test demonstrates two-way
communication.

Chromium side is https://codereview.chromium.org/189253002/ - I haven't looked
at how to split it up for landing, yet.

[kinuko, 2014-03-11 07:13:35]

The blink looks good to me, though I also want to take a look at chromium side
(which seems broken on rietveld..)

https://codereview.chromium.org/185643009/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/postmessage-expected.txt (right):

https://codereview.chromium.org/185643009/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-expected.txt:11: PASS
successfullyParsed is true
Can we also have test results for --enable-service-worker case (in virtual/ ?)

https://codereview.chromium.org/185643009/diff/80001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8ServiceWorkerCustom.cpp (right):

https://codereview.chromium.org/185643009/diff/80001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8ServiceWorkerCustom.cpp:35:
exceptionState.throwIfNeeded();
(It's a bit sad that we seem to have mostly same code in V8WorkerCustom)

https://codereview.chromium.org/185643009/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/185643009/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:61: void poke(const
String& message) { WTF_LOG_ERROR("poke: %s", message.utf8().data()); }
Will you remove this from the CL, or are you planning to land this?

[jsbell, 2014-03-11 15:56:40]

I'll re-upload the Chromium side

https://codereview.chromium.org/185643009/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/postmessage-expected.txt (right):

https://codereview.chromium.org/185643009/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-expected.txt:11: PASS
successfullyParsed is true
On 2014/03/11 07:13:36, kinuko wrote:
> Can we also have test results for --enable-service-worker case (in virtual/ ?)

Ah, that's how it works! :) Yes, I'll add them.

https://codereview.chromium.org/185643009/diff/80001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8ServiceWorkerCustom.cpp (right):

https://codereview.chromium.org/185643009/diff/80001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8ServiceWorkerCustom.cpp:35:
exceptionState.throwIfNeeded();
On 2014/03/11 07:13:36, kinuko wrote:
> (It's a bit sad that we seem to have mostly same code in V8WorkerCustom)

Yep. I'll move the common code into V8Binding.cpp

https://codereview.chromium.org/185643009/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/185643009/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:61: void poke(const
String& message) { WTF_LOG_ERROR("poke: %s", message.utf8().data()); }
On 2014/03/11 07:13:36, kinuko wrote:
> Will you remove this from the CL, or are you planning to land this?

I'll remove. Now that I have bi-directional working, I can test w/o this.

[jsbell, 2014-03-11 20:10:51]

New patch, minor tweaks.

https://codereview.chromium.org/185643009/diff/80001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/postmessage-expected.txt (right):

https://codereview.chromium.org/185643009/diff/80001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/postmessage-expected.txt:11: PASS
successfullyParsed is true
On 2014/03/11 15:56:41, jsbell wrote:
> On 2014/03/11 07:13:36, kinuko wrote:
> > Can we also have test results for --enable-service-worker case (in virtual/
?)
> 
> Ah, that's how it works! :) Yes, I'll add them.

Done.

https://codereview.chromium.org/185643009/diff/80001/Source/bindings/v8/custo...
File Source/bindings/v8/custom/V8ServiceWorkerCustom.cpp (right):

https://codereview.chromium.org/185643009/diff/80001/Source/bindings/v8/custo...
Source/bindings/v8/custom/V8ServiceWorkerCustom.cpp:35:
exceptionState.throwIfNeeded();
On 2014/03/11 15:56:41, jsbell wrote:
> On 2014/03/11 07:13:36, kinuko wrote:
> > (It's a bit sad that we seem to have mostly same code in V8WorkerCustom)
> 
> Yep. I'll move the common code into V8Binding.cpp

I gave this a shot, but it ended up not saving many lines. The code is also
nearly duplicated in V8DedicatedWorkerGlobalScopeCustom and V8MessagePortCustom
and (slightly tweaked due to an extra arg) in V8WindowCustom. So unless anyone
objects I will leave it alone for now, and we can look at consolidating all of
the above as a separate patch.

https://codereview.chromium.org/185643009/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.h (right):

https://codereview.chromium.org/185643009/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.h:61: void poke(const
String& message) { WTF_LOG_ERROR("poke: %s", message.utf8().data()); }
On 2014/03/11 15:56:41, jsbell wrote:
> On 2014/03/11 07:13:36, kinuko wrote:
> > Will you remove this from the CL, or are you planning to land this?
> 
> I'll remove. Now that I have bi-directional working, I can test w/o this.

Done.

[jsbell, 2014-03-11 20:56:52]

Good news - Indexed DB "just works" from the ServiceWorker. I added a test to
verify it to this CL - using postMessage for some of the test plumbing.

[michaeln, 2014-03-11 23:29:19]

https://codereview.chromium.org/185643009/diff/110001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/185643009/diff/110001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorker.cpp:53: // FIXME: Any filtering of
MessagePorts or other Transferrables?
good question?

https://codereview.chromium.org/185643009/diff/110001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorker.cpp:62:
m_outerWorker->postMessage(messageString, webChannels);
I'll point out a dead body here and see if we care to fix it. 

If SerializedScriptValue refers to blobs, upon its unref on return, there is no
guarantee those blobs will survive the transit to the SW on the other side.

The other PostMessage impl that crosses process boundaries has this problem too.

I'd vote to fix it in the introduction of this new method.

[jsbell, 2014-03-11 23:47:09]

https://codereview.chromium.org/185643009/diff/110001/public/platform/WebServ...
File public/platform/WebServiceWorker.h (right):

https://codereview.chromium.org/185643009/diff/110001/public/platform/WebServ...
public/platform/WebServiceWorker.h:48: virtual void postMessage(const
WebString&, WebMessagePortChannelArray*) = 0;
Note to self: add a FIXME to do blob ref-counts management as blobs referenced
in the data are passed between processes.

(This is a general issue with all of the postMessage implementations, though
each one will need to be touched to pass the blob data along the way.)

[jsbell, 2014-03-12 16:49:22]

On 2014/03/11 23:29:19, michaeln wrote:
> If SerializedScriptValue refers to blobs, upon its unref on return, there is
no
> guarantee those blobs will survive the transit to the SW on the other side.
> 
> The other PostMessage impl that crosses process boundaries has this problem
too.

I filed https://code.google.com/p/chromium/issues/detail?id=351753 to track it.

> I'd vote to fix it in the introduction of this new method.

Since it's a pre-existing pattern w/ cross-process postMessage, I'd vote to fix
it later since this patch is useful w/o it (e.g. for easier testing).

Happy to be outvoted though!

[michaeln, 2014-03-12 23:51:52]

> Since it's a pre-existing pattern w/ cross-process postMessage, I'd vote to
fix
> it later since this patch is useful w/o it (e.g. for easier testing).

sgtm, so long as later isn't some time after we ship

https://codereview.chromium.org/185643009/diff/130001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/185643009/diff/130001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorker.cpp:53: // FIXME: Any filtering of
MessagePorts or other Transferrables?
link to crbug/351753 too

[michaeln, 2014-03-12 23:56:29]

also, i was just making a drive-by-comment about that dead body, i'll pull
myself from the reviewers list on this one

[jsbell, 2014-03-13 00:06:22]

https://codereview.chromium.org/185643009/diff/130001/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/185643009/diff/130001/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorker.cpp:53: // FIXME: Any filtering of
MessagePorts or other Transferrables?
On 2014/03/12 23:51:52, michaeln wrote:
> link to crbug/351753 too

Actually, I'll delete this FIXME. When I was first implementing this  I didn't
understand as much about Transferrables, and Alec had mentioned something around
MessagePort complexity. I don't think we want to do anything specific for SW's
postMessage.

[kinuko, 2014-03-13 10:44:04]

I think this patch lgtm

[jsbell, 2014-03-14 18:13:30]

marja@ - could you please take a look at the "message port" voodoo here and on
the Chromium side?

[marja, 2014-03-17 09:29:21]

https://codereview.chromium.org/185643009/diff/150001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/indexeddb-expected.txt (right):

https://codereview.chromium.org/185643009/diff/150001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/indexeddb-expected.txt:10: FAIL
DisabledError
Why DisabledError? (I don't really understand the two tests which have this
expectation, like, which part fails and why it's supposed to fail.)

[kinuko, 2014-03-17 09:32:54]

https://codereview.chromium.org/185643009/diff/150001/LayoutTests/http/tests/...
File LayoutTests/http/tests/serviceworker/indexeddb-expected.txt (right):

https://codereview.chromium.org/185643009/diff/150001/LayoutTests/http/tests/...
LayoutTests/http/tests/serviceworker/indexeddb-expected.txt:10: FAIL
DisabledError
On 2014/03/17 09:29:22, marja wrote:
> Why DisabledError? (I don't really understand the two tests which have this
> expectation, like, which part fails and why it's supposed to fail.)

Currently the feature's not enabled by default, so we get DisabledError for
default settings.
(We have enabled version of expected results in another file, 
https://codereview.chromium.org/185643009/diff/150001/LayoutTests/virtual/ser...)

[marja, 2014-03-17 10:30:14]

Alright. The MessagePort passing bits LGTM. I'm not sure if there would be an
opportunity to refactor some message port passing functions on Blink side too,
and have the message port mangling code in one place. Also the test which
demonstrates the working situation LGTM.

[jsbell, 2014-03-18 23:42:57]

abarth@ - could you OWNERS review for public/, Source/web/ and Source/bindings ?


(Do note comment #4 - there's some additional refactoring that could be done
among the various postMessage implementations, but I'd prefer to do that in a
follow-up.)

[abarth-chromium, 2014-03-19 17:04:09]

LGTM but you might want adamk to look at the message port aspects.

[adamk, 2014-03-19 17:48:58]

Suggested a few places where it might be nice to share code instead of copying
it (related to transforming between blink::WebMessagePortChannelArray and
WebCore::MessagePortChannelArray). I don't think you need to hold off this patch
for those, but some FIXMEs would at least be nice.

https://codereview.chromium.org/185643009/diff/160020/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/185643009/diff/160020/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorker.cpp:55: if (channels &&
channels->size()) {
It makes me sad to see another version of this loop (the leakPtr especially is
looks bad). Any chance of sharing this with the code in
MessagePort::postMessage() (and maybe WebDOMMessageEvent::releaseChannels,
though that one is odd since it uses a stack-allocated WebVector)?

https://codereview.chromium.org/185643009/diff/160020/Source/web/ServiceWorke...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/185643009/diff/160020/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:87: for (size_t i = 0; i <
webChannels.size(); ++i)
This loop, too, it would be nice to share, again since the adoptPtr() inside is
important to get right (though it scares me less than the leakPtr).

[jsbell, 2014-03-19 21:06:46]

adamk@ - thanks for prompting. I factored out those loops, although the name and
location of the resulting helpers could be improved. Any suggestions?

https://codereview.chromium.org/185643009/diff/160020/Source/modules/servicew...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/185643009/diff/160020/Source/modules/servicew...
Source/modules/serviceworkers/ServiceWorker.cpp:55: if (channels &&
channels->size()) {
On 2014/03/19 17:48:58, adamk wrote:
> It makes me sad to see another version of this loop (the leakPtr especially is
> looks bad). Any chance of sharing this with the code in
> MessagePort::postMessage() (and maybe WebDOMMessageEvent::releaseChannels,
> though that one is odd since it uses a stack-allocated WebVector)?

This factors out pretty cleanly (I didn't touch releaseChannels); I ended up
putting it in MessagePort for lack of better inspiration.

https://codereview.chromium.org/185643009/diff/160020/Source/web/ServiceWorke...
File Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/185643009/diff/160020/Source/web/ServiceWorke...
Source/web/ServiceWorkerGlobalScopeProxy.cpp:87: for (size_t i = 0; i <
webChannels.size(); ++i)
On 2014/03/19 17:48:58, adamk wrote:
> This loop, too, it would be nice to share, again since the adoptPtr() inside
is
> important to get right (though it scares me less than the leakPtr).

And ditto for this one.

[adamk, 2014-03-19 21:16:52]

lgtm with one more possibility

Thanks for factoring those out, MessagePort seems as good a dumping ground as
any for now.

https://codereview.chromium.org/185643009/diff/200001/Source/core/dom/Message...
File Source/core/dom/MessagePort.h (right):

https://codereview.chromium.org/185643009/diff/200001/Source/core/dom/Message...
Source/core/dom/MessagePort.h:77: static
PassOwnPtr<blink::WebMessagePortChannelArray>
toWebMessagePortChannelArray(MessagePortChannelArray*);
Seems like the argument here could be a PassOwnPtr<MessagePortChannelArray> to
make it clear that it's invalid after this call (the callers already drop it on
the floor immediately after).

[jsbell, 2014-03-19 21:26:07]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-19 21:26:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/185643009/220001

[jsbell, 2014-03-19 21:26:20]

https://codereview.chromium.org/185643009/diff/200001/Source/core/dom/Message...
File Source/core/dom/MessagePort.h (right):

https://codereview.chromium.org/185643009/diff/200001/Source/core/dom/Message...
Source/core/dom/MessagePort.h:77: static
PassOwnPtr<blink::WebMessagePortChannelArray>
toWebMessagePortChannelArray(MessagePortChannelArray*);
On 2014/03/19 21:16:53, adamk wrote:
> Seems like the argument here could be a PassOwnPtr<MessagePortChannelArray> to
> make it clear that it's invalid after this call (the callers already drop it
on
> the floor immediately after).

Ah yes. Done!

[commit-bot: I haz the power, 2014-03-19 21:41:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-19 21:41:45]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jsbell, 2014-03-19 23:25:19]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-19 23:25:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/185643009/220001

[commit-bot: I haz the power, 2014-03-20 00:43:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 00:43:14]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jsbell, 2014-03-20 16:01:20]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-03-20 16:01:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/185643009/220001

[commit-bot: I haz the power, 2014-03-20 17:06:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-20 17:06:38]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[jsbell, 2014-03-20 17:15:52]

Committed patchset #13 manually as r169668 (presubmit successful).