ServiceWorker: Confirm the liveness of the associated context before handling message

ServiceWorker: Confirm the liveness of the associated context before handling message

SWProviderHost is associated with SWContextCore which can be purged on
corruption recovery process while the host is still referring to it, so the
host should check the liveness of the context before handling the message from
renderer.


BUG=371675
TEST=content_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277738

[nhiroki, 2014-06-11 10:23:31]

Hi, can you review this? Thanks!

DesignDoc:
https://docs.google.com/document/d/1eXdgnAOZC4dDDybmRpXT0t0lOYGhFf7xajOSanEb9...

[michaeln, 2014-06-11 22:02:17]

The CL looks pretty reasonable.

I think i see an alternative to this that i'd like to see what you think about.
At DeleteAndStartOver time, transfer ownership of the map of providers to a new
ContextCore instance. The .swap the map out of the old core and into the new
core.

What i like about that are
- smaller code changes from current sources
- avoids adding the new 'registry' object to the model

https://codereview.chromium.org/325173002/diff/20017/content/browser/service_...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/325173002/diff/20017/content/browser/service_...
content/browser/service_worker/service_worker_request_handler.cc:75: if
(!provider_host)
also return early if !provider_host->IsAlive() ?

[nhiroki, 2014-06-12 00:19:46]

Thank you. Reply only.

On 2014/06/11 22:02:17, michaeln wrote:
> The CL looks pretty reasonable.
> 
> I think i see an alternative to this that i'd like to see what you think
about.
> At DeleteAndStartOver time, transfer ownership of the map of providers to a
new
> ContextCore instance. The .swap the map out of the old core and into the new
> core.
> 
> What i like about that are
> - smaller code changes from current sources
> - avoids adding the new 'registry' object to the model

Looks very smart. I'd love to avoid new complication and just swapping the map
can seem to simplify things. I'll try.

[nhiroki, 2014-06-16 09:50:33]

Updated. Can you take another look?

On 2014/06/12 00:19:46, nhiroki wrote:
> Thank you. Reply only.
> 
> On 2014/06/11 22:02:17, michaeln wrote:
> > The CL looks pretty reasonable.
> > 
> > I think i see an alternative to this that i'd like to see what you think
> about.
> > At DeleteAndStartOver time, transfer ownership of the map of providers to a
> new
> > ContextCore instance. The .swap the map out of the old core and into the new
> > core.
> > 
> > What i like about that are
> > - smaller code changes from current sources
> > - avoids adding the new 'registry' object to the model
> 
> Looks very smart. I'd love to avoid new complication and just swapping the map
> can seem to simplify things. I'll try.

Discarded ServiceWorkerProviderHostRegistry. This CL only checks the liveness of
the associated context and does not take care of ownership of SWProviderHost.
The ".swap" process you mentioned will be added in a separate patch (maybe,
http://crrev.com/334483006, this is still WIP).

[michaeln, 2014-06-16 18:41:35]

lgtm

https://codereview.chromium.org/325173002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/325173002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:99: bool
IsAlive();
nit: IsContextAlive() might be more clear

[nhiroki, 2014-06-16 21:43:47]

https://codereview.chromium.org/325173002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/325173002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_provider_host.h:99: bool
IsAlive();
On 2014/06/16 18:41:35, michaeln wrote:
> nit: IsContextAlive() might be more clear

Done.

[nhiroki, 2014-06-16 21:43:55]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-06-16 21:45:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/325173002/40002

[nhiroki, 2014-06-17 08:56:15]

https://codereview.chromium.org/325173002/diff/20017/content/browser/service_...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/325173002/diff/20017/content/browser/service_...
content/browser/service_worker/service_worker_request_handler.cc:75: if
(!provider_host)
On 2014/06/11 22:02:17, michaeln wrote:
> also return early if !provider_host->IsAlive() ?

Done.

[nhiroki, 2014-06-17 08:56:19]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-06-17 08:58:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/325173002/80001

[commit-bot: I haz the power, 2014-06-17 12:15:38]

Change committed as 277738