ServiceWorker: ignore AddMessageToConsole before process allocation

ServiceWorker: ignore AddMessageToConsole before process allocation

This patch is to skip EmbeddedWorkerInstance::AddMessageToConsole before process
is allocated.

When test is finishing, a renderer process on which the worker's script loading
is running is killed. This cancels the ServiceWorkerWriteToCacheJob
URLRequestJob. So ServiceWorkerWriteToCacheJob::NotifyFinishedCaching() calls
EmbeddedWorkerInstance::AddMessageToConsole.  At the same time with finishing,
ClearAllServiceWorkersForTest calls SWVersion::StopWorker. This causes a
situation that EmbeddedWorkerInstance is in the STARTING state, due to the
restart behavior of Stop, but not yet have a process when AddMessageToConsole is
called. So AddMessageToConsole would crash.


BUG=692439
Review-Url: https://codereview.chromium.org/2703153002
Cr-Commit-Position: refs/heads/master@{#451733}
Committed: https://chromium.googlesource.com/chromium/src/+/acc713786ac1643648471dd35083ce760dcdfd30

[shimazu, 2017-02-20 06:34:05]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 06:34:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2017-02-20 06:35:13]

shimazu@chromium.org changed reviewers:
+ falken@chromium.org

[shimazu, 2017-02-20 06:35:14]

ptal

[falken, 2017-02-20 08:01:03]

Thanks for writing the CL description but I'm a little confused: there is no
SWVersion::Stop(). And I'm not sure how SWVersion::StopWorker() cancels the
URLRequest, do you know how that happens?


Maybe easiest to write out the sequence of events:

ClearAllServiceWorkersForTest calls SWVersion::Stop(). SWVersion::Stop() cancels
the ServiceWorkerWriteToCacheJob URLRequestJob. So
ServiceWorkerWriteToCacheJob::NotifyFinishedCaching() calls
EmbeddedWorkerInstance::AddMessageToConsole. EmbeddedWorkerInstance may be in
the STARTING state, due to the restart behavior of Stop, but not yet have a
process. So AddMessageToConsole would crash.

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:878:
!HasSentStartWorker(starting_phase()))) {
can we turn this into a switch statement?

[falken, 2017-02-20 08:07:13]

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance_unittest.cc:775: // Let
StartWorker fail; binding is discarded in the middle of IPC
I don't understand this comment: where is StartWorker called in lines 776-780?

nit: add a period at the end of the sentence.

[commit-bot: I haz the power, 2017-02-20 08:13:43]

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

[commit-bot: I haz the power, 2017-02-20 08:13:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[falken, 2017-02-20 08:14:45]

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:878:
!HasSentStartWorker(starting_phase()))) {
On 2017/02/20 08:01:03, falken wrote:
> can we turn this into a switch statement?

On second thought, would it be better to just do (!client_.is_bound) { return;
}?

With a comment that this function can be called when the worker is not running?

[shimazu, 2017-02-21 02:18:54]

Description was changed from

==========
ServiceWorker: ignore AddMessageToConsole before process allocation

This patch is to skip EmbeddedWorkerInstance::AddMessageToConsole before process
is allocated. This situation happens when SWVersion::Stop() is forcefully called
and it invokes the restart procedure. Failure of WriteToCacheJob which is
calceled at the Stop() will pass the error message to the embedded worker though
it's allocating a new process. Such forceful Stop() is happening at
SWContextCore::ClearAllServiceWorkersForTest.

BUG=692439
==========

to

==========
ServiceWorker: ignore AddMessageToConsole before process allocation

This patch is to skip EmbeddedWorkerInstance::AddMessageToConsole before process
is allocated.

When test is finishing, a renderer issuing the script loading request is shutted
down. This cancels the ServiceWorkerWriteToCacheJob URLRequestJob. So
ServiceWorkerWriteToCacheJob::NotifyFinishedCaching() calls
EmbeddedWorkerInstance::AddMessageToConsole.  At the same time with finishing,
ClearAllServiceWorkersForTest calls SWVersion::StopWorker. This causes a
situation that EmbeddedWorkerInstance may be in the STARTING state, due to the
restart behavior of Stop, but not yet have a process when AddMessageToConsole is
called. So AddMessageToConsole would crash.


BUG=692439
==========

[shimazu, 2017-02-21 03:13:14]

Description was changed from

==========
ServiceWorker: ignore AddMessageToConsole before process allocation

This patch is to skip EmbeddedWorkerInstance::AddMessageToConsole before process
is allocated.

When test is finishing, a renderer issuing the script loading request is shutted
down. This cancels the ServiceWorkerWriteToCacheJob URLRequestJob. So
ServiceWorkerWriteToCacheJob::NotifyFinishedCaching() calls
EmbeddedWorkerInstance::AddMessageToConsole.  At the same time with finishing,
ClearAllServiceWorkersForTest calls SWVersion::StopWorker. This causes a
situation that EmbeddedWorkerInstance may be in the STARTING state, due to the
restart behavior of Stop, but not yet have a process when AddMessageToConsole is
called. So AddMessageToConsole would crash.


BUG=692439
==========

to

==========
ServiceWorker: ignore AddMessageToConsole before process allocation

This patch is to skip EmbeddedWorkerInstance::AddMessageToConsole before process
is allocated.

When test is finishing, a renderer process on which the worker's script loading
is running is killed. This cancels the ServiceWorkerWriteToCacheJob
URLRequestJob. So ServiceWorkerWriteToCacheJob::NotifyFinishedCaching() calls
EmbeddedWorkerInstance::AddMessageToConsole.  At the same time with finishing,
ClearAllServiceWorkersForTest calls SWVersion::StopWorker. This causes a
situation that EmbeddedWorkerInstance is in the STARTING state, due to the
restart behavior of Stop, but not yet have a process when AddMessageToConsole is
called. So AddMessageToConsole would crash.


BUG=692439
==========

[shimazu, 2017-02-21 03:13:52]

Updated. ptal again:)

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:878:
!HasSentStartWorker(starting_phase()))) {
On 2017/02/20 08:14:45, falken wrote:
> On 2017/02/20 08:01:03, falken wrote:
> > can we turn this into a switch statement?
> 
> On second thought, would it be better to just do (!client_.is_bound) { return;
> }?

|client_| is valid just after calling Start(), so it's not enough in this case.

> With a comment that this function can be called when the worker is not
running?

Yes, this function can be called while starting (e.g. script loading).

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:878:
!HasSentStartWorker(starting_phase()))) {
On 2017/02/20 08:01:03, falken wrote:
> can we turn this into a switch statement?

Done.

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance_unittest.cc:775: // Let
StartWorker fail; binding is discarded in the middle of IPC
On 2017/02/20 08:07:13, falken wrote:
> I don't understand this comment: where is StartWorker called in lines 776-780?
> 
> nit: add a period at the end of the sentence.

Sorry, it's just my copy-and-paste failure. Fixed.

[shimazu, 2017-02-21 03:13:53]

Updated. ptal again:)

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:878:
!HasSentStartWorker(starting_phase()))) {
On 2017/02/20 08:14:45, falken wrote:
> On 2017/02/20 08:01:03, falken wrote:
> > can we turn this into a switch statement?
> 
> On second thought, would it be better to just do (!client_.is_bound) { return;
> }?

|client_| is valid just after calling Start(), so it's not enough in this case.

> With a comment that this function can be called when the worker is not
running?

Yes, this function can be called while starting (e.g. script loading).

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:878:
!HasSentStartWorker(starting_phase()))) {
On 2017/02/20 08:01:03, falken wrote:
> can we turn this into a switch statement?

Done.

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2703153002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance_unittest.cc:775: // Let
StartWorker fail; binding is discarded in the middle of IPC
On 2017/02/20 08:07:13, falken wrote:
> I don't understand this comment: where is StartWorker called in lines 776-780?
> 
> nit: add a period at the end of the sentence.

Sorry, it's just my copy-and-paste failure. Fixed.

[shimazu, 2017-02-21 03:15:50]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 03:15:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[falken, 2017-02-21 04:03:30]

lgtm

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

https://codereview.chromium.org/2703153002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:884: break;
Sorry for keeping suggesting things. Would if (process_id() != kInvalidUniqueID)
work also? It just seems like a tricky piece of code to put in
AddMessageToConsole. Or should we just refactor it to a function HasProcess()?
It's somewhat surprising this is the only place the check is needed...

[shimazu, 2017-02-21 04:39:42]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2017-02-21 04:39:42]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2703153002/#ps60001
(title: "Use process_id() instead of switch statement")

[shimazu, 2017-02-21 04:39:52]

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

https://codereview.chromium.org/2703153002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:884: break;
On 2017/02/21 04:03:30, falken wrote:
> Sorry for keeping suggesting things. Would if (process_id() !=
kInvalidUniqueID)
> work also? It just seems like a tricky piece of code to put in
> AddMessageToConsole. Or should we just refactor it to a function HasProcess()?
> It's somewhat surprising this is the only place the check is needed...

process_id() != kInvalidUniqueID seems much better than current if/switch.
Updated, thanks!

[commit-bot: I haz the power, 2017-02-21 04:40:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 07:33:46]

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

[commit-bot: I haz the power, 2017-02-21 07:33:48]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[shimazu, 2017-02-21 09:35:47]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2017-02-21 09:36:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 11:30:57]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487669747691730,
"parent_rev": "86ff27ab575772808e4aa02430c571f770d556be", "commit_rev":
"acc713786ac1643648471dd35083ce760dcdfd30"}

[commit-bot: I haz the power, 2017-02-21 11:31:30]

Description was changed from

==========
ServiceWorker: ignore AddMessageToConsole before process allocation

This patch is to skip EmbeddedWorkerInstance::AddMessageToConsole before process
is allocated.

When test is finishing, a renderer process on which the worker's script loading
is running is killed. This cancels the ServiceWorkerWriteToCacheJob
URLRequestJob. So ServiceWorkerWriteToCacheJob::NotifyFinishedCaching() calls
EmbeddedWorkerInstance::AddMessageToConsole.  At the same time with finishing,
ClearAllServiceWorkersForTest calls SWVersion::StopWorker. This causes a
situation that EmbeddedWorkerInstance is in the STARTING state, due to the
restart behavior of Stop, but not yet have a process when AddMessageToConsole is
called. So AddMessageToConsole would crash.


BUG=692439
==========

to

==========
ServiceWorker: ignore AddMessageToConsole before process allocation

This patch is to skip EmbeddedWorkerInstance::AddMessageToConsole before process
is allocated.

When test is finishing, a renderer process on which the worker's script loading
is running is killed. This cancels the ServiceWorkerWriteToCacheJob
URLRequestJob. So ServiceWorkerWriteToCacheJob::NotifyFinishedCaching() calls
EmbeddedWorkerInstance::AddMessageToConsole.  At the same time with finishing,
ClearAllServiceWorkersForTest calls SWVersion::StopWorker. This causes a
situation that EmbeddedWorkerInstance is in the STARTING state, due to the
restart behavior of Stop, but not yet have a process when AddMessageToConsole is
called. So AddMessageToConsole would crash.


BUG=692439

Review-Url: https://codereview.chromium.org/2703153002
Cr-Commit-Position: refs/heads/master@{#451733}
Committed:
https://chromium.googlesource.com/chromium/src/+/acc713786ac1643648471dd35083...
==========

[commit-bot: I haz the power, 2017-02-21 11:31:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/acc713786ac1643648471dd35083...