Allow the browser to Send messages to an embedded worker once the script as been loaded without hav…

Allow the browser to Send messages to an embedded worker once the script as been loaded without having to wait for the initial eval to complete.

BUG=405701
Committed: https://crrev.com/d604aea83210f191044b9c58ad56c4cf081dd543
Cr-Commit-Position: refs/heads/master@{#292008}

[michaeln, 2014-08-20 22:15:06]

ptal

[jsbell, 2014-08-21 00:15:30]

On 2014/08/20 22:15:06, michaeln wrote:
> ptal

FYI, with this fix applied, most of the crashes go away. Still hitting a crash
in asanka's cache-add.html but I think it's due to unimplemented bits of the
cache API causing the script to fail and unregister while async cache
initialization is still in process.

[dominicc (has gone to gerrit), 2014-08-21 04:01:04]

On 2014/08/21 00:15:30, jsbell wrote:
> On 2014/08/20 22:15:06, michaeln wrote:
> > ptal
> 
> FYI, with this fix applied, most of the crashes go away. Still hitting a crash
> in asanka's cache-add.html but I think it's due to unimplemented bits of the
> cache API causing the script to fail and unregister while async cache
> initialization is still in process.

Are we suuuuuuuure about this? On the one hand, it sucks that every user of Send
has to handle the case when the worker is starting but not running.

On the other hand, if we start sending messages before the worker starts and the
worker fails to start, those messages have been blackholed.

Why did we do it this way in the first place? That code was quite explicit with
DCHECKS and stuff?

[dominicc (has gone to gerrit), 2014-08-21 04:04:23]

Was it simply because we didn't have a thread ID on hand to post to?

The change itself looks good. Did you audit the callers? Were there any that
checked status before sending that we need to/can change?

https://codereview.chromium.org/489253002/diff/40001/content/browser/service_...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/489253002/diff/40001/content/browser/service_...
content/browser/service_worker/embedded_worker_instance.cc:174: DCHECK_NE(-1,
thread_id_);
kInvalidThreadId or something grepable for this?

[dominicc (has gone to gerrit), 2014-08-21 06:41:19]

After reviewing https://codereview.chromium.org/474593002/ ,
https://codereview.chromium.org/433793002/ I am warming up to why this is
necessary.

[michaeln, 2014-08-21 19:26:56]

> Are we suuuuuuuure about this?

Yes. This is a really straightforward change to relax an overly aggressive
dcheck. It just wasn't anticipated that script was executing prior to being in
the RUNNING state, and we didn't have the routing info (threadid) needed to send
messages prior to receipt of the 'started' message either.

>  it sucks that every user of Send has to...

Also this is a lower level EmbeddedWorkerInstance class. The
ServiceWorkerVersion is a higher level class more intended for higher level
consumers (aka modules). ServiceWorkerVersion has a SendMessage() method that
will cause the EmbeddedWorker to be started if need be in order to send the
message.

> Why did we do it this way in the first place?

To ensure that we didn't have or introduce code doing whacky unexpected things.

[jsbell, 2014-08-21 20:43:57]

lgtm, modulo dominicc's nit about -1

[michaeln, 2014-08-25 21:21:55]

michaeln@chromium.org changed reviewers:
+ nasko@chromium.org

[michaeln, 2014-08-25 21:22:23]

On 2014/08/25 21:21:55, michaeln wrote:
> mailto:michaeln@chromium.org changed reviewers:
> + mailto:nasko@chromium.org

@nasko, ptal at the messages.h change

[nasko, 2014-08-26 15:58:24]

The service worker code doesn't follow the code style when it comes to multiple
arguments in function definition. I've flagged a couple of places.

Otherwise IPC LGTM

https://codereview.chromium.org/489253002/diff/100001/content/browser/service...
File content/browser/service_worker/embedded_worker_registry.cc (right):

https://codereview.chromium.org/489253002/diff/100001/content/browser/service...
content/browser/service_worker/embedded_worker_registry.cc:82: int thread_id,
style: From the style guide: "For function declarations and definitions, put
each argument on a separate line unless the whole declaration fits on one line."

https://codereview.chromium.org/489253002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/489253002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:397: int
embedded_worker_id, int thread_id) {
style: if params are on new line, they should all be on new lines.

[michaeln, 2014-08-26 20:27:29]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-08-26 20:28:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaeln@chromium.org/489253002/140001

[nasko, 2014-08-26 21:41:07]

On 2014/08/26 15:58:24, nasko wrote:
> The service worker code doesn't follow the code style when it comes to
multiple
> arguments in function definition. I've flagged a couple of places.
> 
> Otherwise IPC LGTM
> 
>
https://codereview.chromium.org/489253002/diff/100001/content/browser/service...
> File content/browser/service_worker/embedded_worker_registry.cc (right):
> 
>
https://codereview.chromium.org/489253002/diff/100001/content/browser/service...
> content/browser/service_worker/embedded_worker_registry.cc:82: int thread_id,
> style: From the style guide: "For function declarations and definitions, put
> each argument on a separate line unless the whole declaration fits on one
line."
> 
>
https://codereview.chromium.org/489253002/diff/100001/content/browser/service...
> File content/browser/service_worker/service_worker_dispatcher_host.cc (right):
> 
>
https://codereview.chromium.org/489253002/diff/100001/content/browser/service...
> content/browser/service_worker/service_worker_dispatcher_host.cc:397: int
> embedded_worker_id, int thread_id) {
> style: if params are on new line, they should all be on new lines.

Total brain fart on the style, apologies.

[commit-bot: I haz the power, 2014-08-26 22:03:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-26 22:43:21]

Committed patchset #8 (140001) as 087ba168c930b37776d3b4103825650c77d8d389

[michaeln1, 2014-08-27 19:23:40]

> Total brain fart on the style, apologies.

np, i fixed up some style things based on what you said, i put emphasis on
having individual files be internally consistent and also on not making
gratuitous changes (at least i tried to)

[commit-bot: I haz the power, 2014-09-10 02:46:37]

Patchset 8 (id:??) landed as
https://crrev.com/d604aea83210f191044b9c58ad56c4cf081dd543
Cr-Commit-Position: refs/heads/master@{#292008}