ServiceWorker: Don't start the timeout timer when DevTools is detached

ServiceWorker: Don't start the timeout timer when DevTools is detached

Starting/stopping the timeout timer should be triggered in response to the
lifecycle event of the service worker, that is, only StartWorker() should
call StartTimeoutTimer() and only OnStopped() should call StopTimeoutTimer().

This CL removes the call of StartTimeoutTimer() from SetDevToolsAttached()
and instead reactivates the timer for start timeout when DevTools is detached
and the worker is still under starting phase.

BUG=448003
TEST=content_unittests --gtest_filter=ServiceWorkerVersionTest.*

Committed: https://crrev.com/01dab9680e4a5e82624daf0a6d758f03242361d0
Cr-Commit-Position: refs/heads/master@{#321727}

[nhiroki, 2015-03-13 04:45:41]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-03-13 04:45:46]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-03-13 04:47:53]

Patchset #2 (id:60001) has been deleted

[nhiroki, 2015-03-13 04:50:08]

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

[nhiroki, 2015-03-13 04:51:58]

Ptal, thanks!

[falken, 2015-03-13 07:14:09]

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

[falken, 2015-03-13 07:14:10]

drive-by comment

It's my fault for leaving that StartTimeoutTimer call in SetDevToolsAttached.
The original plan was for the timer to deactivate after it tries to
StopWorkerIfIdle one time (which fails if devtools is attached, so it should be
reactivated when dev tools is detached), but I changed that in my last patch
https://codereview.chromium.org/991743002/

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

https://codereview.chromium.org/1004913003/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:791:
RestartTick(&start_time_);
The start timeout sometimes should be in effect for status STOPPING also. This
is because "StartWorker(); StopWorker();" results in a worker with status
STOPPING that may be in the process of starting up.

https://codereview.chromium.org/1004913003/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:794:
skip_recording_startup_time_ = true;
This line and line 785-786 essentially do the same thing, so should be combined
somehow or commented to explain the difference. The current comment is a bit
weird since this runs when DevTools is detached.

[nhiroki, 2015-03-16 01:41:16]

Patchset #3 (id:100001) has been deleted

[nhiroki, 2015-03-16 01:58:02]

Updated. Can you take another look?

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

https://codereview.chromium.org/1004913003/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:791:
RestartTick(&start_time_);
On 2015/03/13 07:14:10, falken (ooo slow until Mar 23) wrote:
> The start timeout sometimes should be in effect for status STOPPING also. This
> is because "StartWorker(); StopWorker();" results in a worker with status
> STOPPING that may be in the process of starting up.

Thank you, I didn't notice it. Tweaked the condition of the if-statement.

https://codereview.chromium.org/1004913003/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:794:
skip_recording_startup_time_ = true;
On 2015/03/13 07:14:10, falken (ooo slow until Mar 23) wrote:
> This line and line 785-786 essentially do the same thing, so should be
combined
> somehow or commented to explain the difference. The current comment is a bit
> weird since this runs when DevTools is detached.

Exactly. Merged this into line 785.

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (left):

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:883:
RestartTick(&start_time_);
FYI: We may have to call StartTimeoutTimer() here. The patchset3 will get to
call it via StartWorkerInternal().

[kinuko, 2015-03-16 05:16:13]

lgtm, but probably falken@ should take another look

[nhiroki, 2015-03-16 06:51:42]

On 2015/03/16 05:16:13, kinuko wrote:
> lgtm, but probably falken@ should take another look

Thank you!

falken@: Can you take another look when you have a time? Next week is fine :)

[falken, 2015-03-23 00:14:25]

Sorry for the delayed review.

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:785: if
(running_status() == STARTING || running_status() == STOPPING) {
This is kind of a non-obvious condition. Would it make sense to do if
(!start_callbacks_.empty()) and maybe DCHECK for running_status?

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:882:
StartWorkerInternal(false /* pause_after_download */);
Thanks for this helper function.

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1417: if (status !=
SERVICE_WORKER_OK || is_doomed()) {
I feel like non-OK status here is unexpected and worth recording in the
StartWorker histogram, WDYT?

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1434: // Keep the live
registration while staring the worker.
nit: starting

[nhiroki, 2015-03-23 00:43:17]

Thank you! Updated.

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:785: if
(running_status() == STARTING || running_status() == STOPPING) {
On 2015/03/23 00:14:25, falken wrote:
> This is kind of a non-obvious condition. Would it make sense to do if
> (!start_callbacks_.empty()) and maybe DCHECK for running_status?

Done.

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:785: if
(running_status() == STARTING || running_status() == STOPPING) {
On 2015/03/23 00:14:25, falken wrote:
> This is kind of a non-obvious condition. Would it make sense to do if
> (!start_callbacks_.empty()) and maybe DCHECK for running_status?

Done.

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1417: if (status !=
SERVICE_WORKER_OK || is_doomed()) {
On 2015/03/23 00:14:25, falken wrote:
> I feel like non-OK status here is unexpected and worth recording in the
> StartWorker histogram, WDYT?

Sounds reasonable. Added the function call (is_doomed() case is just ignored in
the record function).

https://codereview.chromium.org/1004913003/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1434: // Keep the live
registration while staring the worker.
On 2015/03/23 00:14:25, falken wrote:
> nit: starting

Done.

[falken, 2015-03-23 00:52:31]

https://codereview.chromium.org/1004913003/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1004913003/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1423:
RecordStartWorkerResult(status);
Do we know start_time_ is null here? I think we might not, since it gets reset
in StartTimeoutTimer which doesn't run until later?

[nhiroki, 2015-03-23 01:38:51]

https://codereview.chromium.org/1004913003/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1004913003/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1423:
RecordStartWorkerResult(status);
On 2015/03/23 00:52:31, falken wrote:
> Do we know start_time_ is null here? I think we might not, since it gets reset
> in StartTimeoutTimer which doesn't run until later?

Hmmm... I don't really understand why we need to take care of |start_time_|
here. Are you concerned about recording the time? That would be bypassed at line
1618 due to non-ok |status|.

In addition, non-null |start_time_| means that this version is in starting phase
and |start_callbacks_| own reference to the live registration, that is, |status|
seems always OK in the case.

[falken, 2015-03-23 01:41:10]

Ah right, I had been worried about recording the time. lgtm

[nhiroki, 2015-03-23 01:42:35]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-03-23 01:42:36]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1004913003/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-03-23 01:42:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1004913003/160001

[commit-bot: I haz the power, 2015-03-23 02:43:11]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2015-03-23 02:43:42]

Patchset 5 (id:??) landed as
https://crrev.com/01dab9680e4a5e82624daf0a6d758f03242361d0
Cr-Commit-Position: refs/heads/master@{#321727}