service worker: Wait for inflight requests before activating

content/browser/service_worker/service_worker_version.cc

Index: content/browser/service_worker/service_worker_version.cc
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index 46237101d755621167608438b99d5d04f8c14bf3..af4192d9c4d8e352404a571bcf8ec725195923d2 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -545,6 +545,9 @@ bool ServiceWorkerVersion::FinishRequest(int request_id, bool was_handled) {
   TRACE_EVENT_ASYNC_END1("ServiceWorker", "ServiceWorkerVersion::Request",
                          request, "Handled", was_handled);
   pending_requests_.Remove(request_id);
+  if (!HasInflightRequests())
+    FOR_EACH_OBSERVER(Listener, listeners_, OnNoInflightRequests(this));
+
   return true;
 }
 
@@ -601,9 +604,8 @@ void ServiceWorkerVersion::RemoveControllee(
   controllee_map_.erase(uuid);
   FOR_EACH_OBSERVER(Listener, listeners_,
                     OnControlleeRemoved(this, provider_host));
-  if (HasControllee())
-    return;
-  FOR_EACH_OBSERVER(Listener, listeners_, OnNoControllees(this));
+  if (!HasControllee())
+    FOR_EACH_OBSERVER(Listener, listeners_, OnNoControllees(this));
 }
 
 void ServiceWorkerVersion::AddStreamingURLRequestJob(
@@ -616,6 +618,8 @@ void ServiceWorkerVersion::AddStreamingURLRequestJob(
 void ServiceWorkerVersion::RemoveStreamingURLRequestJob(
     const ServiceWorkerURLRequestJob* request_job) {
   streaming_url_request_jobs_.erase(request_job);
+  if (!HasInflightRequests())
+    FOR_EACH_OBSERVER(Listener, listeners_, OnNoInflightRequests(this));
 }
 
 void ServiceWorkerVersion::AddListener(Listener* listener) {
@@ -1716,8 +1720,9 @@ void ServiceWorkerVersion::OnStoppedInternal(EmbeddedWorkerStatus old_status) {
   streaming_url_request_jobs_.clear();
 
   FOR_EACH_OBSERVER(Listener, listeners_, OnRunningStateChanged(this));
-
-  if (should_restart)
+  if (!HasInflightRequests() && !should_restart)

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

HasInflightRequests() always returns false here because we already cleared
|pending_requests_| and |streaming_url_request_jobs_|.

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

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

+    FOR_EACH_OBSERVER(Listener, listeners_, OnNoInflightRequests(this));

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

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?

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

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.

+  else if (should_restart)
     StartWorkerInternal();

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

if (should_restart)
  StartWorkerIntenal();
else if (!HasInflightRequests())
  FOR_EACH_OBSERVER(...);

...would be simpler.

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

Done.

 }