service worker: Wait for inflight requests before activating

service worker: Wait for inflight requests before activating

This patch makes changes as per discussion at
https://github.com/slightlyoff/ServiceWorker/issues/916

Namely,
* The waiting worker gets promoted to active once there are no
controllees and the incumbent worker has no in-flight events
(fetch/sync/push/message etc).
* skipWaiting bypasses the no controllees requirement, but we
still wait until there are no in-flight events.
* As usual, incoming fetch/sync/push events always go to the
active worker.

BUG=616331
Committed: https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e
Cr-Commit-Position: refs/heads/master@{#405628}

[falken, 2016-07-07 05:55:48]

Description was changed from

==========
WIP: Wait for inflight requests.

BUG=
==========

to

==========
service worker: Wait for inflight requests before activating

This patch makes changes as per discussion at
https://github.com/slightlyoff/ServiceWorker/issues/916

Namely,
* The waiting worker gets promoted to active once there are no
controllees and the incumbent worker has no in-flight events
(fetch/sync/push/message etc).
* skipWaiting bypasses the no controllees requirement, but we
still wait until there are no in-flight events.
* As usual, incoming fetch/sync/push events always go to the
active worker.

BUG=616331
==========

[falken, 2016-07-07 05:56:53]

falken@chromium.org changed reviewers:
+ bkelly@mozilla.com, nhiroki@chromium.org

[falken, 2016-07-07 05:56:54]

nhiroki: PTAL
bkelly: FYI, you might want to look at the WPT tests

[bkelly, 2016-07-07 13:48:08]

Overall the tests look really good.  Thanks!  I just had one comment about
avoiding setTimeout().

Do you expect to uplift this or should I pull into the wpt tree when we fix the
problem in gecko?

https://codereview.chromium.org/2119143002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html
(right):

https://codereview.chromium.org/2119143002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:59:
return new Promise(resolve => setTimeout(resolve, 0));
I don't think a single event loop turn is really adequate here.  Can we make a
helper function that does something like this?

1) Register a second service worker on a different scope
2) Wait for the second service worker to become active
3) Unregister the second service worker

This would ensure that the activation had adequate time to complete since we
observed it complete for a different service worker.

I think we should use something like this instead of setTimeout() throughout
this test if possible.

[falken, 2016-07-08 03:36:19]

Thanks!

https://codereview.chromium.org/2119143002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html
(right):

https://codereview.chromium.org/2119143002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:59:
return new Promise(resolve => setTimeout(resolve, 0));
On 2016/07/07 13:48:08, bkelly wrote:
> I don't think a single event loop turn is really adequate here.  Can we make a
> helper function that does something like this?
> 
> 1) Register a second service worker on a different scope
> 2) Wait for the second service worker to become active
> 3) Unregister the second service worker
> 
> This would ensure that the activation had adequate time to complete since we
> observed it complete for a different service worker.
> 
> I think we should use something like this instead of setTimeout() throughout
> this test if possible.

That's a great idea. Done.

[falken, 2016-07-08 03:38:00]

> Do you expect to uplift this or should I pull into the wpt tree when we fix
the
problem in gecko?

I'll give a try, it'd be good for me to understand the process.

[nhiroki, 2016-07-08 05:04:42]

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:266: bool
ServiceWorkerRegistration::IsReadyToActivate() const {
DCHECK(waiting_version()) ?

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:273:
(active->HasControllee() && !waiting_version()->skip_waiting())));
Negating the whole complex expression would make it difficult to read. How about
deforming it with De Morgan's laws like this?

   !(active && (HasInflight || (HasControllee && !skip_waiting)))
-> !active || !(HasInflight || (HasControllee && !skip_waiting))
-> !active || (!HasInflight && !(HasControllee && !skip_waiting))
-> !active || (!HasInflight && (!HasControllee || skip_waiting))

if (!active)
  return true;
if (!active->HasInflightRequests() &&
    (!HasControllee || waiting_version()->skip_waiting()))
  return true;
return false;

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:280:
DCHECK(should_activate_when_ready_);
Can you replace this check with "DCHECK(IsReadyToActivate())" ?

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_registration_unittest.cc:259:
TEST_F(ServiceWorkerActivationTest, NoControllee) {
It would be better to swap line 258-259 with line 276-277, because actually this
tests activation when an inflight request is done.

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_registration_unittest.cc:277:
TEST_F(ServiceWorkerActivationTest, NoInflightRequest) {
ditto.

[falken, 2016-07-08 07:06:46]

PTAL

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:266: bool
ServiceWorkerRegistration::IsReadyToActivate() const {
On 2016/07/08 05:04:42, nhiroki wrote:
> DCHECK(waiting_version()) ?

Hmm good catch. The code assumes should_activate_when_ready_ is only true if
there is a waiting_version, but I think thanks to DeleteVersion that's not true.
Updated.

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:273:
(active->HasControllee() && !waiting_version()->skip_waiting())));
On 2016/07/08 05:04:42, nhiroki wrote:
> Negating the whole complex expression would make it difficult to read. How
about
> deforming it with De Morgan's laws like this?
> 
>    !(active && (HasInflight || (HasControllee && !skip_waiting)))
> -> !active || !(HasInflight || (HasControllee && !skip_waiting))
> -> !active || (!HasInflight && !(HasControllee && !skip_waiting))
> -> !active || (!HasInflight && (!HasControllee || skip_waiting))
> 
> if (!active)
>   return true;
> if (!active->HasInflightRequests() &&
>     (!HasControllee || waiting_version()->skip_waiting()))
>   return true;
> return false;

Done.

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_registration.cc:280:
DCHECK(should_activate_when_ready_);
On 2016/07/08 05:04:42, nhiroki wrote:
> Can you replace this check with "DCHECK(IsReadyToActivate())" ?

Good. Done.

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/2119143002/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_registration_unittest.cc:259:
TEST_F(ServiceWorkerActivationTest, NoControllee) {
On 2016/07/08 05:04:42, nhiroki wrote:
> It would be better to swap line 258-259 with line 276-277, because actually
this
> tests activation when an inflight request is done.

Good eye, done.

[falken, 2016-07-12 05:38:27]

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

[falken, 2016-07-12 05:38:28]

kinuko: Do you have time to review this since most of the team is away? (I hope
to get this fix in soon...)

[nhiroki, 2016-07-13 01:12:22]

On 2016/07/12 05:38:28, falken wrote:
> kinuko: Do you have time to review this since most of the team is away? (I
hope
> to get this fix in soon...)

fyi: I just resumed reviewing this.

[kinuko, 2016-07-13 02:28:35]

looks like hiroki's reviewing this too so I hope he can catch details I'd miss
:)

lgtm % some nits

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_registration.cc:189:
ActivateWaitingVersion(false);
nit: add '/* delay */' comment

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_registration_unittest.cc:250: int
inflight_request_id() { return request_id_; }
nit: const method

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_registration_unittest.cc:255: int
request_id_;
nit: request_id_ --> inflight_request_id_

nit: initialize this to something in ctor or here (it's uninitialized until we
call SetUp)

[nhiroki, 2016-07-13 04:29:30]

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_registration.cc:110: if
(active_version_ == version)
(This comment is not related to this CL. You don't have to work on this in this
CL)

I suspect this condition is never satisfied. In production, SetActiveVersion()
can be called from ServiceWorkerStorage::GetOrCreateRegistration() or
ServiceWorkerRegistration::ActivateWaitingVersion(). On the former callsite, a
registration is newly created and its |active_version_| should always be
nullptr. On the latter callsite, it would be strange if |activating_version| is
equal to |exiting_version|.

If these are true, we could replace this if-statement with
DCHECK_NE(active_version_, version) etc.

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_registration.cc:130: if
(waiting_version_ == version)
ditto.

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

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1723: if
(!HasInflightRequests() && !should_restart)
HasInflightRequests() always returns false here because we already cleared
|pending_requests_| and |streaming_url_request_jobs_|.

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1724:
FOR_EACH_OBSERVER(Listener, listeners_, OnNoInflightRequests(this));
Is there any reason not to notify of OnNoInflightRequests when |should_restart|
is true? IMO, it'd be more stable that observers are notified whenever an event
happens.

If you're concerned about a case where this version becomes redundant during
OnNoInflightRequests before restarting, we could check is_redundant() and give
up restarting the worker if it's true like this:

// An observer may call StopWorker(), but it immediately returns
// because running_status() is now STOPPED.
FOR_EACH_OBSERVER(Listener, listeners_, OnNoInflightRequeses(this));
if (is_redundant()) {
  should_restart = false;
  RunCallbacks(this, &start_callbacks_, ...);
}
if (should_restart)
  StartWorkerInternal();

WDYT?

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1726:
StartWorkerInternal();
if (should_restart)
  StartWorkerIntenal();
else if (!HasInflightRequests())
  FOR_EACH_OBSERVER(...);

...would be simpler.

[nhiroki, 2016-07-13 04:48:43]

Layout tests look good.

https://codereview.chromium.org/2119143002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html
(right):

https://codereview.chromium.org/2119143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:18:
navigator.serviceWorker.register('resources/empty-worker.js', { scope:
dummy_scope })
Can you wrap at 80 columns?

https://codereview.chromium.org/2119143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:48: //
Register.
This comment seems useless.

https://codereview.chromium.org/2119143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:60:
assert_not_equals(iframe.contentWindow.navigator.serviceWorker.controller,
null);
ditto(80 columns)

[falken, 2016-07-14 13:49:02]

Patchset #7 (id:120001) has been deleted

[falken, 2016-07-14 13:49:14]

Patchset #8 (id:160001) has been deleted

[falken, 2016-07-14 13:52:08]

Updated PTAL.

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_registration.cc (right):

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_registration.cc:110: if
(active_version_ == version)
On 2016/07/13 04:29:30, nhiroki (slow) wrote:
> (This comment is not related to this CL. You don't have to work on this in
this
> CL)
> 
> I suspect this condition is never satisfied. In production, SetActiveVersion()
> can be called from ServiceWorkerStorage::GetOrCreateRegistration() or
> ServiceWorkerRegistration::ActivateWaitingVersion(). On the former callsite, a
> registration is newly created and its |active_version_| should always be
> nullptr. On the latter callsite, it would be strange if |activating_version|
is
> equal to |exiting_version|.
> 
> If these are true, we could replace this if-statement with
> DCHECK_NE(active_version_, version) etc.

Acknowledged. (OTOH, it may be reasonable to write complex setters like this to
be idempotent, even if current code doesn't use them that way.)

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_registration.cc:189:
ActivateWaitingVersion(false);
On 2016/07/13 02:28:34, kinuko wrote:
> nit: add '/* delay */' comment

Done.

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_registration_unittest.cc
(right):

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_registration_unittest.cc:250: int
inflight_request_id() { return request_id_; }
On 2016/07/13 02:28:35, kinuko wrote:
> nit: const method

Done.

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_registration_unittest.cc:255: int
request_id_;
On 2016/07/13 02:28:35, kinuko wrote:
> nit: request_id_ --> inflight_request_id_
> 
> nit: initialize this to something in ctor or here (it's uninitialized until we
> call SetUp)

Done.

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

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1723: if
(!HasInflightRequests() && !should_restart)
On 2016/07/13 04:29:30, nhiroki (slow) wrote:
> HasInflightRequests() always returns false here because we already cleared
> |pending_requests_| and |streaming_url_request_jobs_|.

Good point. I guess an observer can't add a pending request or request job since
the worker is stopped.

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1724:
FOR_EACH_OBSERVER(Listener, listeners_, OnNoInflightRequests(this));
On 2016/07/13 04:29:30, nhiroki (slow) wrote:
> Is there any reason not to notify of OnNoInflightRequests when
|should_restart|
> is true? IMO, it'd be more stable that observers are notified whenever an
event
> happens.

That's true and a good point. The reason is that |should_restart| indicates
there are start_callbacks, which means there are more requests coming. If we
notify OnNoInflightRequests and the registration kicks out this version, those
requests are dropped just like in the bug this patch is meant to fix.

I think what we need to do is consider start_callbacks the same as an inflight
request. See the new patch.

https://codereview.chromium.org/2119143002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:1726:
StartWorkerInternal();
On 2016/07/13 04:29:30, nhiroki (slow) wrote:
> if (should_restart)
>   StartWorkerIntenal();
> else if (!HasInflightRequests())
>   FOR_EACH_OBSERVER(...);
> 
> ...would be simpler.

Done.

https://codereview.chromium.org/2119143002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html
(right):

https://codereview.chromium.org/2119143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:18:
navigator.serviceWorker.register('resources/empty-worker.js', { scope:
dummy_scope })
On 2016/07/13 04:48:43, nhiroki (slow) wrote:
> Can you wrap at 80 columns?

Done.

https://codereview.chromium.org/2119143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:18:
navigator.serviceWorker.register('resources/empty-worker.js', { scope:
dummy_scope })
OMG more importantly this didn't return anything so this function didn't work.

https://codereview.chromium.org/2119143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:48: //
Register.
On 2016/07/13 04:48:43, nhiroki (slow) wrote:
> This comment seems useless.

Moved "Create an active worker" up here.

https://codereview.chromium.org/2119143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:60:
assert_not_equals(iframe.contentWindow.navigator.serviceWorker.controller,
null);
On 2016/07/13 04:48:43, nhiroki (slow) wrote:
> ditto(80 columns)

Done.

[falken, 2016-07-14 13:52:14]

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

[commit-bot: I haz the power, 2016-07-14 13:52:28]

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

[nhiroki, 2016-07-14 14:24:51]

lgtm

https://codereview.chromium.org/2119143002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html
(right):

https://codereview.chromium.org/2119143002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:17: var
dummy_scope = 'resources/there/is/no/there/there';
Maybe "var registration;" is necessary?

[falken, 2016-07-14 14:54:19]

https://codereview.chromium.org/2119143002/diff/180001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html
(right):

https://codereview.chromium.org/2119143002/diff/180001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/serviceworker/activation.html:17: var
dummy_scope = 'resources/there/is/no/there/there';
On 2016/07/14 14:24:50, nhiroki (slow) wrote:
> Maybe "var registration;" is necessary?

Done.

[falken, 2016-07-14 14:54:27]

The CQ bit was checked by falken@chromium.org

[falken, 2016-07-14 14:54:27]

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

[commit-bot: I haz the power, 2016-07-14 14:54:38]

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, 2016-07-14 16:26:39]

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

[commit-bot: I haz the power, 2016-07-14 16:26:40]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[falken, 2016-07-14 22:26:16]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2016-07-14 22:26:40]

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, 2016-07-14 23:40:07]

Description was changed from

==========
service worker: Wait for inflight requests before activating

This patch makes changes as per discussion at
https://github.com/slightlyoff/ServiceWorker/issues/916

Namely,
* The waiting worker gets promoted to active once there are no
controllees and the incumbent worker has no in-flight events
(fetch/sync/push/message etc).
* skipWaiting bypasses the no controllees requirement, but we
still wait until there are no in-flight events.
* As usual, incoming fetch/sync/push events always go to the
active worker.

BUG=616331
==========

to

==========
service worker: Wait for inflight requests before activating

This patch makes changes as per discussion at
https://github.com/slightlyoff/ServiceWorker/issues/916

Namely,
* The waiting worker gets promoted to active once there are no
controllees and the incumbent worker has no in-flight events
(fetch/sync/push/message etc).
* skipWaiting bypasses the no controllees requirement, but we
still wait until there are no in-flight events.
* As usual, incoming fetch/sync/push events always go to the
active worker.

BUG=616331
==========

[commit-bot: I haz the power, 2016-07-14 23:40:08]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2016-07-14 23:40:14]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 23:42:04]

Description was changed from

==========
service worker: Wait for inflight requests before activating

This patch makes changes as per discussion at
https://github.com/slightlyoff/ServiceWorker/issues/916

Namely,
* The waiting worker gets promoted to active once there are no
controllees and the incumbent worker has no in-flight events
(fetch/sync/push/message etc).
* skipWaiting bypasses the no controllees requirement, but we
still wait until there are no in-flight events.
* As usual, incoming fetch/sync/push events always go to the
active worker.

BUG=616331
==========

to

==========
service worker: Wait for inflight requests before activating

This patch makes changes as per discussion at
https://github.com/slightlyoff/ServiceWorker/issues/916

Namely,
* The waiting worker gets promoted to active once there are no
controllees and the incumbent worker has no in-flight events
(fetch/sync/push/message etc).
* skipWaiting bypasses the no controllees requirement, but we
still wait until there are no in-flight events.
* As usual, incoming fetch/sync/push events always go to the
active worker.

BUG=616331

Committed: https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e
Cr-Commit-Position: refs/heads/master@{#405628}
==========

[commit-bot: I haz the power, 2016-07-14 23:42:05]

Patchset 9 (id:??) landed as
https://crrev.com/6c95b0ff8f694cda918e38c96387e5445713fc8e
Cr-Commit-Position: refs/heads/master@{#405628}