Add ref count to service workers for extension API.

content/browser/service_worker/service_worker_context_wrapper.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_context_wrapper.h"
 
 #include <map>
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
+#include "content/browser/service_worker/embedded_worker_registry.h"

[falken, 2016/09/30 01:48:55]

Is this used? Can it just be embedded_worker_status.h?

[lazyboy, 2016/09/30 08:02:15]

Thanks.
Done.

 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_observer.h"
 #include "content/browser/service_worker/service_worker_process_manager.h"
 #include "content/browser/service_worker/service_worker_quota_client.h"
 #include "content/browser/service_worker/service_worker_version.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/common/service_worker/service_worker_utils.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
@@ skipping 335 matching lines @@
 void ServiceWorkerContextWrapper::DidFinishNavigationHintTaskOnUI(
     int render_process_id,
     const ResultCallback& callback,
     bool result) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!--navigation_hint_task_count_per_process_[render_process_id])
     navigation_hint_task_count_per_process_.erase(render_process_id);
   callback.Run(result);
 }
 
+void ServiceWorkerContextWrapper::ExternalRequestErrorCallback(
+    int64_t service_worker_version_id,
+    ServiceWorkerStatusCode status) {
+  if (status != SERVICE_WORKER_OK)
+    pending_external_requests_[service_worker_version_id].pop();

[falken, 2016/09/30 01:48:55]

Hmm... it might be slightly safer to bind the request_id to this function and
then delete it specifically. But I guess it doesn't matter since this callback
is called for each pending external request. WDYT?

Also, now there's a new issue: the renderer doesn't know that the request got
cancelled and removed, so it might still call DecrementPendingActivity. This is
getting tricky.

I haven't thought things through but maybe a nonce could be generated on the
renderer-side for each pending activity, that get mapped to a request id on
browser-side. Or the current patch is basically okay and just silently dropping
DecrementPendingActivity when there are no pending requests will eventually lead
to a stable state again.

[lazyboy, 2016/09/30 08:02:15]

It won't be possible to do this without changing StartRequest code, because
StartRequest
returns the request id (and this is a param to StartRequest).

But I guess it doesn't matter since this callback
This seems like safer alternative, I've used GUID for this, let's see how we
feel about this.
We should be able to reason about a DecrementPendingActivity better now given
the
request_uuid.
 that get mapped to a request id on

+}
+
 bool ServiceWorkerContextWrapper::IsRunningNavigationHintTask(
     int render_process_id) const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   return base::ContainsKey(navigation_hint_task_count_per_process_,
                            render_process_id);
 }
 
 void ServiceWorkerContextWrapper::StartServiceWorker(
     const GURL& pattern,
     const StatusCallback& callback) {
@@ skipping 428 matching lines @@
       user_data_directory, std::move(database_task_manager), disk_cache_thread,
       quota_manager_proxy, special_storage_policy, observer_list_.get(), this));
 }
 
 void ServiceWorkerContextWrapper::ShutdownOnIO() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   resource_context_ = nullptr;
   context_core_.reset();
 }
 
+bool ServiceWorkerContextWrapper::IncrementPendingActivity(
+    int64_t service_worker_version_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  ServiceWorkerVersion* version =
+      context()->GetLiveVersion(service_worker_version_id);
+  if (!version) {
+    DVLOG(1) << "ServiceWorkerVersion not found: " << service_worker_version_id;
+    return false;

[falken, 2016/09/30 01:48:55]

Sorry I didn't suggest this sooner. Consider having these unexpected cases in
Increment/DecrementPendingActivity lead to a BadMessageReceived (at
ExtensionServiceWorkerMessageFilter). That way we get a crash report and you
don't need to do any special error handling at the callsite. And you might not
need the DVLOGs or DCHECKs or NOTREACHEDs here.

[lazyboy, 2016/09/30 08:02:15]

Done.

+  }
+  if (version->running_status() != EmbeddedWorkerStatus::RUNNING) {
+    DVLOG(1) << "ServiceWorkerVersion is not running: "
+             << service_worker_version_id;
+    return false;
+  }
+  int request_id = version->StartRequest(
+      ServiceWorkerMetrics::EventType::EXTERNAL_REQUEST,
+      base::Bind(&ServiceWorkerContextWrapper::ExternalRequestErrorCallback,
+                 this, service_worker_version_id));
+  pending_external_requests_[service_worker_version_id].push(request_id);
+  return true;
+}
+
+bool ServiceWorkerContextWrapper::DecrementPendingActivity(
+    int64_t service_worker_version_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  ServiceWorkerVersion* version =
+      context()->GetLiveVersion(service_worker_version_id);
+
+  std::queue<int>& pending_requests =
+      pending_external_requests_[service_worker_version_id];
+  DCHECK(pending_requests.size() > 0);

[falken, 2016/09/30 01:48:55]

Now that requests can be removed by the error callback, it might be best to
silently ignore invalid DecrementPendingActivity calls. We've ran into issues
before where we used to BadMessageReceived when receiving a response for a timed
out request: https://codereview.chromium.org/2150863002 

[lazyboy, 2016/09/30 08:02:15]

Slightly modified code now, but done.

+  int request_id = pending_requests.front();
+  pending_requests.pop();
+
+  // The service worker should be present and running.
+  if (!version || version->running_status() != EmbeddedWorkerStatus::RUNNING) {
+    NOTREACHED()
+        << "Failed to decrement pending activity of ServiceWorkerVersion: "
+        << service_worker_version_id;
+    return false;
+  }
+  return version->FinishRequest(request_id, true, base::Time::Now());
+}
+
 void ServiceWorkerContextWrapper::DidDeleteAndStartOver(
     ServiceWorkerStatusCode status) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   if (status != SERVICE_WORKER_OK) {
     context_core_.reset();
     return;
   }
   context_core_.reset(new ServiceWorkerContextCore(context_core_.get(), this));
   DVLOG(1) << "Restarted ServiceWorkerContextCore successfully.";
 
   observer_list_->Notify(FROM_HERE,
                          &ServiceWorkerContextObserver::OnStorageWiped);
 }
 
 ServiceWorkerContextCore* ServiceWorkerContextWrapper::context() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   return context_core_.get();
 }
 
 }  // namespace content