Fix crash during EmbeddedWorkerInstance startup sequence failures

Fix crash during EmbeddedWorkerInstance startup sequence failures

Once EWInstance startup calls the callback, it's possible that the
underlying ServiceWorkerVersion is destroyed, hence destroying
|this|. We must guard against that.

Also some failure points in the startup sequence weren't calling
OnStopped() as expected.

BUG=529520, 531345
Committed: https://crrev.com/37b83877e63f62f8aebb337494f1116539182bdf
Cr-Commit-Position: refs/heads/master@{#349368}

[falken, 2015-09-09 09:56:24]

falken@chromium.org changed reviewers:
+ kinuko@chromium.org, nhiroki@chromium.org

[falken, 2015-09-09 09:56:24]

This is a sneak preview of a patch... I still need to add unittests. But if you
have time until then PTAL.

[falken, 2015-09-10 10:35:25]

Added unittests PTAL

[kinuko (google), 2015-09-10 22:14:19]

kinuko@google.com changed reviewers:
+ kinuko@google.com

[kinuko (google), 2015-09-10 22:14:19]

looking good (will take another look later)

https://codereview.chromium.org/1327723005/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/1327723005/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:25: //
cannot be used.
base::Passed() or base::Owned() works?

[nhiroki, 2015-09-11 04:33:02]

https://codereview.chromium.org/1327723005/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/1327723005/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:319: // already been
called.
The following code might work?

// Here
  OnStartFailed(ABORT, callback);
  return;

// In OnStartFailed
  Status old_status = status_;
  if (status_ != STOPPED)
    ReleaseProcess();  // Sets |status_| to STOPPED. 
  base::WeakPtr<EmbeddedWorkerInstance> weak_this = weak_factory_.GetWeakPtr();
  callback.Run(status);
  if (weak_this && old_status_ != STOPPED)
    FOR_EACH_OBSERVER(Listener, listener_list_, OnStopped(STARTING));

[falken, 2015-09-11 09:48:17]

Thanks, PTAL.

https://codereview.chromium.org/1327723005/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/1327723005/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:319: // already been
called.
On 2015/09/11 04:33:02, nhiroki wrote:
> The following code might work?
> 
> // Here
>   OnStartFailed(ABORT, callback);
>   return;
> 
> // In OnStartFailed
>   Status old_status = status_;
>   if (status_ != STOPPED)
>     ReleaseProcess();  // Sets |status_| to STOPPED. 
>   base::WeakPtr<EmbeddedWorkerInstance> weak_this =
weak_factory_.GetWeakPtr();
>   callback.Run(status);
>   if (weak_this && old_status_ != STOPPED)
>     FOR_EACH_OBSERVER(Listener, listener_list_, OnStopped(STARTING));

I like this, done, with the minor exception of calling ReleaseProcess even if
STOPPED, it should be a no-op.

https://codereview.chromium.org/1327723005/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/1327723005/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:25: //
cannot be used.
On 2015/09/10 22:14:19, kinuko (google) wrote:
> base::Passed() or base::Owned() works?

Huh yes... I must have made a mistake on my first attempt. Done.

[nhiroki, 2015-09-12 23:55:31]

lgtm

[kinuko, 2015-09-13 08:41:12]

lgtm

https://codereview.chromium.org/1327723005/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/1327723005/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:513:
OnStopped(old_status));
nit: would be better to update the comment for Listener::OnStopped about when it
could be called

https://codereview.chromium.org/1327723005/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.h (right):

https://codereview.chromium.org/1327723005/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.h:238: // |callback|
with |status|. May destroy |this|.
nit: ... via |callback|. ?

[nhiroki, 2015-09-17 06:14:13]

> Also some failure points in the startup sequence weren't calling
> OnStopped() as expected.

This could fix issue 531345 (see my inline comment for the details). We'd like
to fix the issue asap, so let me check the CQ box on behalf of falken@ (he is
ooo for a while).

https://codereview.chromium.org/1327723005/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/1327723005/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:353:
OnStartFailed(callback, status);
I guess this could fix issue 531345. Before this patch, EmbeddedWorkerInstance
could be stick on STARTING state and subsequent StartWorker requests are queued
up until the timeout timer is fired (5 minutes). The timeout timer stops the
EWInstance[1] and records TimeoutPhase as SENT_START_WORKER.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...

[nhiroki, 2015-09-17 06:15:00]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-09-17 06:15:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1327723005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1327723005/60001

[commit-bot: I haz the power, 2015-09-17 07:14:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-17 07:15:25]

Patchset 4 (id:??) landed as
https://crrev.com/37b83877e63f62f8aebb337494f1116539182bdf
Cr-Commit-Position: refs/heads/master@{#349368}