service worker: Wait for inflight requests before activating

content/browser/service_worker/service_worker_version.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_version.h"
 
 #include <stddef.h>
 
 #include <limits>
 #include <map>
@@ skipping 527 matching lines @@
   // TODO(kinuko): Record other event statuses too.
   metrics_->RecordEventHandledStatus(request->event_type, was_handled);
   ServiceWorkerMetrics::RecordEventDuration(
       request->event_type, base::TimeTicks::Now() - request->start_time,
       was_handled);
 
   RestartTick(&idle_time_);
   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;
 }
 
 void ServiceWorkerVersion::RunAfterStartWorker(
     ServiceWorkerMetrics::EventType purpose,
     const base::Closure& task,
     const StatusCallback& error_callback) {
   if (running_status() == EmbeddedWorkerStatus::RUNNING) {
     DCHECK(start_callbacks_.empty());
     task.Run();
@@ skipping 36 matching lines @@
                     OnControlleeAdded(this, provider_host));
 }
 
 void ServiceWorkerVersion::RemoveControllee(
     ServiceWorkerProviderHost* provider_host) {
   const std::string& uuid = provider_host->client_uuid();
   DCHECK(ContainsKey(controllee_map_, uuid));
   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(
     const ServiceWorkerURLRequestJob* request_job) {
   DCHECK(streaming_url_request_jobs_.find(request_job) ==
          streaming_url_request_jobs_.end());
   streaming_url_request_jobs_.insert(request_job);
 }
 
 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) {
   listeners_.AddObserver(listener);
 }
 
 void ServiceWorkerVersion::RemoveListener(Listener* listener) {
   listeners_.RemoveObserver(listener);
 }
 
@@ skipping 1080 matching lines @@
   pending_requests_.Clear();
 
   // Close all mojo services. This will also fire and clear all callbacks
   // for messages that are still outstanding for those services.
   mojo_services_.clear();
 
   // TODO(falken): Call SWURLRequestJob::ClearStream here?
   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.

 }
 
 void ServiceWorkerVersion::OnMojoConnectionError(const char* service_name) {
   // Simply deleting the service will cause error callbacks to be called from
   // the destructor of the MojoServiceWrapper instance.
   mojo_services_.erase(service_name);
 }
 
 void ServiceWorkerVersion::OnBeginEvent() {
   if (should_exclude_from_uma_ ||
       running_status() != EmbeddedWorkerStatus::RUNNING ||
       idle_time_.is_null()) {
     return;
   }
   ServiceWorkerMetrics::RecordTimeBetweenEvents(base::TimeTicks::Now() -
                                                 idle_time_);
 }
 
 }  // namespace content