Make ServiceWorkerDispatcher reuse existing WebServiceWorkerImpls.

ServiceWorkerDispatcher used to mint new WebServiceWorkerImpl
instances every time it was passed a representation of a Service
Worker. Ultimately, this causes user script to see distinct objects
representing the same Service Worker, which is Confusing and
Wrong. This patch makes ServiceWorkerDispatcher use its table of
existing WebServiceWorkerImpls to consistently represent a
ServiceWorker with the same object.

A chunk of this patch is concerned with making it explicit when a
ServiceWorkerHandleReference, which a WebServiceWorkerImpl wraps, is
being created in the renderer (and should send an add-ref message to
the browser) versus being adopted in the renderer.

BUG=361907
TEST=ServiceWorker*

Review URL: https://codereview.chromium.org/309503014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274477 0039d316-1c4b-4281-b951-d872f2087c98

content/child/service_worker/service_worker_dispatcher.cc

@@ -148,6 +148,29 @@ void ServiceWorkerDispatcher::OnWorkerRunLoopStopped() {
   delete this;
 }
 
+WebServiceWorkerImpl* ServiceWorkerDispatcher::GetServiceWorker(
+    const ServiceWorkerObjectInfo& info,
+    bool adopt_handle) {
+  WorkerObjectMap::iterator existing_worker =
+      service_workers_.find(info.handle_id);
+
+  if (existing_worker != service_workers_.end()) {
+    if (adopt_handle) {
+      // We are instructed to adopt a handle but we already have one, so
+      // adopt and destroy a handle ref.
+      ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_);
+    }
+    return existing_worker->second;
+  }
+
+  scoped_ptr<ServiceWorkerHandleReference> handle_ref =
+      adopt_handle
+          ? ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_)
+          : ServiceWorkerHandleReference::Create(info, thread_safe_sender_);
+  // WebServiceWorkerImpl constructor calls AddServiceWorker.
+  return new WebServiceWorkerImpl(handle_ref.Pass(), thread_safe_sender_);
+}
+
 void ServiceWorkerDispatcher::OnRegistered(
     int thread_id,
     int request_id,
@@ -158,17 +181,7 @@ void ServiceWorkerDispatcher::OnRegistered(
   if (!callbacks)
     return;
 
-  // The browser has to generate the registration_id so the same
-  // worker can be called from different renderer contexts. However,
-  // the impl object doesn't have to be the same instance across calls
-  // unless we require the DOM objects to be identical when there's a
-  // duplicate registration. So for now we mint a new object each
-  // time.
-  //
-  // WebServiceWorkerImpl's ctor internally calls AddServiceWorker.
-  scoped_ptr<WebServiceWorkerImpl> worker(
-      new WebServiceWorkerImpl(info, thread_safe_sender_));
-  callbacks->onSuccess(worker.release());
+  callbacks->onSuccess(GetServiceWorker(info, true));
   pending_callbacks_.Remove(request_id);
 }
 
@@ -228,10 +241,7 @@ void ServiceWorkerDispatcher::OnSetCurrentServiceWorker(
   ScriptClientMap::iterator found = script_clients_.find(provider_id);
   if (found != script_clients_.end()) {
     // Populate the .current field with the new worker object.
-    scoped_ptr<ServiceWorkerHandleReference> handle_ref(
-        ServiceWorkerHandleReference::Create(info, thread_safe_sender_));
-    found->second->setCurrentServiceWorker(
-        new WebServiceWorkerImpl(handle_ref.Pass(), thread_safe_sender_));
+    found->second->setCurrentServiceWorker(GetServiceWorker(info, false));
   }
 }
 

content/child/service_worker/service_worker_dispatcher.h

@@ -70,6 +70,22 @@ class ServiceWorkerDispatcher : public WorkerTaskRunner::Observer {
                        blink::WebServiceWorkerProviderClient* client);
   void RemoveScriptClient(int provider_id);
 
+  // If an existing WebServiceWorkerImpl exists for the Service
+  // Worker, it is returned; otherwise a WebServiceWorkerImpl is
+  // created and its ownership is transferred to the caller. If
+  // |adopt_handle| is true, a ServiceWorkerHandleReference will be
+  // adopted for the specified Service Worker.
+  //
+  // TODO(dominicc): The lifetime of WebServiceWorkerImpl is too tricky; this
+  // method can return an existing WebServiceWorkerImpl, in which case
+  // it is owned by a WebCore::ServiceWorker and the lifetime is not
+  // being transferred to the owner; or it can create a
+  // WebServiceWorkerImpl, in which case ownership is transferred to
+  // the caller who must bounce it to a method that will associate it
+  // with a WebCore::ServiceWorker.
+  WebServiceWorkerImpl* GetServiceWorker(const ServiceWorkerObjectInfo&,
+                                         bool adopt_handle);
+
   // |thread_safe_sender| needs to be passed in because if the call leads to
   // construction it will be needed.
   static ServiceWorkerDispatcher* GetOrCreateThreadSpecificInstance(

content/child/service_worker/service_worker_handle_reference.cc

@@ -17,8 +17,7 @@ ServiceWorkerHandleReference::Create(
   return make_scoped_ptr(new ServiceWorkerHandleReference(info, sender, true));
 }
 
-scoped_ptr<ServiceWorkerHandleReference>
-ServiceWorkerHandleReference::CreateForDeleter(
+scoped_ptr<ServiceWorkerHandleReference> ServiceWorkerHandleReference::Adopt(
     const ServiceWorkerObjectInfo& info,
     ThreadSafeSender* sender) {
   DCHECK(sender);

content/child/service_worker/service_worker_handle_reference.h

@@ -17,12 +17,17 @@ class ThreadSafeSender;
 // (in the browser side) in ctor and dtor.
 class ServiceWorkerHandleReference {
  public:
-  // Creates a new ServiceWorkerHandleReference (and increments ref-count).
+  // Creates a new ServiceWorkerHandleReference and increments ref-count.
   static scoped_ptr<ServiceWorkerHandleReference> Create(
       const ServiceWorkerObjectInfo& info,
       ThreadSafeSender* sender);
-  // This doesn't increment ref-count in ctor.
-  static scoped_ptr<ServiceWorkerHandleReference> CreateForDeleter(
+
+  // Creates a new ServiceWorkerHandleReference by adopting a
+  // ref-count. ServiceWorkerHandleReferences created this way must
+  // have a matching
+  // ServiceWorkerDispatcherHost::RegisterServiceWorkerHandle call on
+  // the browser side.
+  static scoped_ptr<ServiceWorkerHandleReference> Adopt(
       const ServiceWorkerObjectInfo& info,
       ThreadSafeSender* sender);
 

content/child/service_worker/service_worker_provider_context.cc

@@ -36,13 +36,9 @@ ServiceWorkerProviderContext::~ServiceWorkerProviderContext() {
   }
 }
 
-scoped_ptr<ServiceWorkerHandleReference>
-ServiceWorkerProviderContext::GetCurrentServiceWorkerHandle() {
+ServiceWorkerHandleReference* ServiceWorkerProviderContext::current() {
   DCHECK(main_thread_loop_proxy_->RunsTasksOnCurrentThread());
-  if (!current_)
-    return scoped_ptr<ServiceWorkerHandleReference>();
-  return ServiceWorkerHandleReference::Create(
-      current_->info(), thread_safe_sender_);
+  return current_.get();
 }
 
 void ServiceWorkerProviderContext::OnServiceWorkerStateChanged(
@@ -63,8 +59,7 @@ void ServiceWorkerProviderContext::OnSetCurrentServiceWorker(
 
   // This context is is the primary owner of this handle, keeps the
   // initial reference until it goes away.
-  current_ = ServiceWorkerHandleReference::CreateForDeleter(
-      info, thread_safe_sender_);
+  current_ = ServiceWorkerHandleReference::Adopt(info, thread_safe_sender_);
 
   // TODO(kinuko): We can forward the message to other threads here
   // when we support navigator.serviceWorker in dedicated workers.

content/child/service_worker/service_worker_provider_context.h

@@ -37,9 +37,6 @@ class ServiceWorkerProviderContext
  public:
   explicit ServiceWorkerProviderContext(int provider_id);
 
-  // Returns a new handle reference for .current.
-  scoped_ptr<ServiceWorkerHandleReference> GetCurrentServiceWorkerHandle();
-
   // Called from ServiceWorkerDispatcher.
   void OnServiceWorkerStateChanged(int handle_id,
                                    blink::WebServiceWorkerState state);
@@ -47,6 +44,14 @@ class ServiceWorkerProviderContext
                                  const ServiceWorkerObjectInfo& info);
 
   int provider_id() const { return provider_id_; }
+
+  // Gets the context's handle reference for .controller.
+  // TODO(dominicc): Rename this to "controller".
+  ServiceWorkerHandleReference* current();
+
+  // Gets the handle ID of the controller, or
+  // kInvalidServiceWorkerHandleId if the provider is not controlled
+  // by a Service Worker.
   int current_handle_id() const;
 
  private:

content/child/service_worker/web_service_worker_impl.cc

@@ -19,21 +19,6 @@ using blink::WebString;
 
 namespace content {
 
-WebServiceWorkerImpl::WebServiceWorkerImpl(
-    const ServiceWorkerObjectInfo& info,
-    ThreadSafeSender* thread_safe_sender)
-    : handle_ref_(
-          ServiceWorkerHandleReference::CreateForDeleter(info,
-                                                         thread_safe_sender)),
-      state_(handle_ref_->state()),
-      thread_safe_sender_(thread_safe_sender),
-      proxy_(NULL) {
-  ServiceWorkerDispatcher* dispatcher =
-      ServiceWorkerDispatcher::GetThreadSpecificInstance();
-  DCHECK(dispatcher);
-  dispatcher->AddServiceWorker(handle_ref_->handle_id(), this);
-}
-
 WebServiceWorkerImpl::WebServiceWorkerImpl(
     scoped_ptr<ServiceWorkerHandleReference> handle_ref,
     ThreadSafeSender* thread_safe_sender)
@@ -69,6 +54,10 @@ void WebServiceWorkerImpl::setProxy(blink::WebServiceWorkerProxy* proxy) {
   proxy_ = proxy;
 }
 
+blink::WebServiceWorkerProxy* WebServiceWorkerImpl::proxy() {
+  return proxy_;
+}
+
 void WebServiceWorkerImpl::proxyReadyChanged() {
   if (!proxy_->isReady())
     return;

content/child/service_worker/web_service_worker_impl.h

@@ -35,8 +35,6 @@ class ThreadSafeSender;
 class WebServiceWorkerImpl
     : NON_EXPORTED_BASE(public blink::WebServiceWorker) {
  public:
-  WebServiceWorkerImpl(const ServiceWorkerObjectInfo& info,
-                       ThreadSafeSender* thread_safe_sender);
   WebServiceWorkerImpl(scoped_ptr<ServiceWorkerHandleReference> handle_ref,
                        ThreadSafeSender* thread_safe_sender);
   virtual ~WebServiceWorkerImpl();
@@ -47,6 +45,7 @@ class WebServiceWorkerImpl
   void OnStateChanged(blink::WebServiceWorkerState new_state);
 
   virtual void setProxy(blink::WebServiceWorkerProxy* proxy);
+  virtual blink::WebServiceWorkerProxy* proxy();
   virtual void proxyReadyChanged();
   virtual blink::WebURL scope() const;
   virtual blink::WebURL url() const;

content/child/service_worker/web_service_worker_provider_impl.cc

@@ -45,18 +45,14 @@ void WebServiceWorkerProviderImpl::setClient(
   // (e.g. on document and on dedicated workers) can properly share
   // the single provider context across threads. (http://crbug.com/366538
   // for more context)
-  scoped_ptr<ServiceWorkerHandleReference> current =
-      context_->GetCurrentServiceWorkerHandle();
   GetDispatcher()->AddScriptClient(provider_id_, client);
-  if (!current)
+
+  int handle_id = context_->current_handle_id();
+  if (handle_id == kInvalidServiceWorkerHandleId)
     return;
 
-  int handle_id = current->info().handle_id;
-  if (handle_id != kInvalidServiceWorkerHandleId) {
-    scoped_ptr<WebServiceWorkerImpl> worker(
-        new WebServiceWorkerImpl(current.Pass(), thread_safe_sender_));
-    client->setCurrentServiceWorker(worker.release());
-  }
+  client->setCurrentServiceWorker(
+      GetDispatcher()->GetServiceWorker(context_->current()->info(), false));
 }
 
 void WebServiceWorkerProviderImpl::registerServiceWorker(