Introduce ServiceWorkerHandle for tracking WebServiceWorkerImpl reference

Introduce ServiceWorkerHandle for tracking WebServiceWorkerImpl reference

* Introduce a new browser-side class, ServiceWorkerHandle for keeping {Registration,Version} references for a WebSWImpl in the renderer process
* Keep live handles in ServiceWorkerDispatcherHost (as handles_)
* Remove cyclic dependency between Registration and Version
* Remove shutdown semantics in Registration and Version

BUG=349919, 361052
R=michaeln@chromium.org, tsepez@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262636

[kinuko, 2014-04-07 14:42:00]

An attempt to fix/extend the ownership/object model. WDYT?

[michaeln, 2014-04-08 01:21:00]

i really like this change a lot!

[michaeln, 2014-04-08 01:21:40]

ooops sent ^^^ w/o the comments

https://codereview.chromium.org/224733014/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/224733014/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:213: //
corresponding WebServiceWorkerImpl alive.
and by switching to handle_id as input, takes care of this todo

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:134: new
WebServiceWorkerImpl(handle_id, version_id,
in cases were the message doesn't get far and no WebServiceWorkerImpl is cons'd
up, we'll want to send the ServiceWorkerObjectDestroyed msg

so if the taskrunner returned by msgfilter::OverrideTaskRunner() failes to post
the task to dispatch the OnRegistered msg because the thread has been shutdown
already... when that happens.... objectDestroyed msg.

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.h:75: int handle_id,
do we need version id on the client side anymore? given handle_id, we can easily
look up version_id on the browser side.

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
File content/child/service_worker/web_service_worker_impl.cc (right):

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
content/child/service_worker/web_service_worker_impl.cc:35: version_id_,
if we change postMessage to take handle_id as input, looks like we don't need
version_id here (we can do that in a later cl if it makes sense)

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
File content/child/service_worker/web_service_worker_impl.h (right):

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
content/child/service_worker/web_service_worker_impl.h:32:
scoped_refptr<ThreadSafeSender> thread_safe_sender_;
thnx for fixing

[michaeln, 2014-04-08 01:24:32]

generally lgtm, if you want me to take another look after making further changes
lmk

[kinuko, 2014-04-08 08:43:30]

Thanks, addressed comments. I also added thread_id param to the
ServiceWorkerHandle ctor (as it's needed to dispatch events). I'm going to ask
security folks to messages review

https://codereview.chromium.org/224733014/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/224733014/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:213: //
corresponding WebServiceWorkerImpl alive.
On 2014/04/08 01:21:41, michaeln wrote:
> and by switching to handle_id as input, takes care of this todo

Done.

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.cc (right):

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.cc:134: new
WebServiceWorkerImpl(handle_id, version_id,
On 2014/04/08 01:21:41, michaeln wrote:
> in cases were the message doesn't get far and no WebServiceWorkerImpl is
cons'd
> up, we'll want to send the ServiceWorkerObjectDestroyed msg
> 
> so if the taskrunner returned by msgfilter::OverrideTaskRunner() failes to
post
> the task to dispatch the OnRegistered msg because the thread has been shutdown
> already... when that happens.... objectDestroyed msg.

Done.

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
File content/child/service_worker/service_worker_dispatcher.h (right):

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
content/child/service_worker/service_worker_dispatcher.h:75: int handle_id,
On 2014/04/08 01:21:41, michaeln wrote:
> do we need version id on the client side anymore? given handle_id, we can
easily
> look up version_id on the browser side.

Done.

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
File content/child/service_worker/web_service_worker_impl.cc (right):

https://codereview.chromium.org/224733014/diff/20001/content/child/service_wo...
content/child/service_worker/web_service_worker_impl.cc:35: version_id_,
On 2014/04/08 01:21:41, michaeln wrote:
> if we change postMessage to take handle_id as input, looks like we don't need
> version_id here (we can do that in a later cl if it makes sense)

Done.

[kinuko, 2014-04-08 08:56:47]

tsepez@: could you review the messages file? Thx
content/common/service_worker/service_worker_messages.h

[Tom Sepez, 2014-04-08 17:30:57]

Messages LGTM.

[kinuko, 2014-04-09 00:28:53]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-04-09 00:39:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/90001

[commit-bot: I haz the power, 2014-04-09 01:00:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/90001

[commit-bot: I haz the power, 2014-04-09 01:12:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/90001

[commit-bot: I haz the power, 2014-04-09 01:31:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/90001

[commit-bot: I haz the power, 2014-04-09 01:42:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/90001

[commit-bot: I haz the power, 2014-04-09 01:52:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/90001

[commit-bot: I haz the power, 2014-04-09 02:03:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/90001

[commit-bot: I haz the power, 2014-04-09 02:20:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/90001

[commit-bot: I haz the power, 2014-04-09 04:39:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/90001

[commit-bot: I haz the power, 2014-04-09 04:40:08]

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

[commit-bot: I haz the power, 2014-04-09 04:40:09]

Failed to apply patch for
content/browser/service_worker/service_worker_dispatcher_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/service_worker/service_worker_dispatcher_host.cc
  Hunk #1 succeeded at 11 (offset 1 line).
  Hunk #2 FAILED at 101.
  Hunk #3 succeeded at 172 (offset 3 lines).
  Hunk #4 succeeded at 187 (offset 3 lines).
  Hunk #5 succeeded at 207 (offset 3 lines).
  Hunk #6 succeeded at 261 (offset 3 lines).
  Hunk #7 succeeded at 315 with fuzz 1 (offset 15 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
content/browser/service_worker/service_worker_dispatcher_host.cc.rej

Patch:       content/browser/service_worker/service_worker_dispatcher_host.cc
Index: content/browser/service_worker/service_worker_dispatcher_host.cc
diff --git a/content/browser/service_worker/service_worker_dispatcher_host.cc
b/content/browser/service_worker/service_worker_dispatcher_host.cc
index
dd29cfc9246a89e8c3086331eb93792b3b3683f8..302bb60ea962d6bd8798b1e7c51a67846084043e
100644
--- a/content/browser/service_worker/service_worker_dispatcher_host.cc
+++ b/content/browser/service_worker/service_worker_dispatcher_host.cc
@@ -10,6 +10,7 @@
 #include "content/browser/service_worker/embedded_worker_registry.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
+#include "content/browser/service_worker/service_worker_handle.h"
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_utils.h"
 #include "content/common/service_worker/embedded_worker_messages.h"
@@ -100,12 +101,19 @@ bool ServiceWorkerDispatcherHost::OnMessageReceived(
                         OnWorkerStopped)
     IPC_MESSAGE_HANDLER(EmbeddedWorkerHostMsg_SendMessageToBrowser,
                         OnSendMessageToBrowser)
+    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_ServiceWorkerObjectDestroyed,
+                        OnServiceWorkerObjectDestroyed)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 
   return handled;
 }
 
+int ServiceWorkerDispatcherHost::RegisterServiceWorkerHandle(
+    scoped_ptr<ServiceWorkerHandle> handle) {
+  return handles_.Add(handle.release());
+}
+
 void ServiceWorkerDispatcherHost::OnRegisterServiceWorker(
     int32 thread_id,
     int32 request_id,
@@ -168,7 +176,7 @@ void ServiceWorkerDispatcherHost::OnUnregisterServiceWorker(
 }
 
 void ServiceWorkerDispatcherHost::OnPostMessage(
-    int64 version_id,
+    int handle_id,
     const base::string16& message,
     const std::vector<int>& sent_message_port_ids) {
   if (!context_ || !ServiceWorkerUtils::IsFeatureEnabled())
@@ -183,13 +191,13 @@ void ServiceWorkerDispatcherHost::OnPostMessage(
         new_routing_ids[i]);
   }
 
-  // TODO(kinuko,michaeln): Make sure we keep the version that has
-  // corresponding WebServiceWorkerImpl alive.
-  ServiceWorkerVersion* version = context_->GetLiveVersion(version_id);
-  if (!version)
+  ServiceWorkerHandle* handle = handles_.Lookup(handle_id);
+  if (!handle) {
+    BadMessageReceived();
     return;
+  }
 
-  version->SendMessage(
+  handle->version()->SendMessage(
       ServiceWorkerMsg_Message(message, sent_message_port_ids,
new_routing_ids),
       base::Bind(&NoOpStatusCallback));
 }
@@ -203,7 +211,7 @@ void ServiceWorkerDispatcherHost::OnProviderCreated(int
provider_id) {
   }
   scoped_ptr<ServiceWorkerProviderHost> provider_host(
       new ServiceWorkerProviderHost(
-          render_process_id_, provider_id, context_));
+          render_process_id_, provider_id, context_, this));
   context_->AddProviderHost(provider_host.Pass());
 }
 
@@ -257,13 +265,21 @@ void ServiceWorkerDispatcherHost::RegistrationComplete(
     ServiceWorkerStatusCode status,
     int64 registration_id,
     int64 version_id) {
+  if (!context_)
+    return;
+
   if (status != SERVICE_WORKER_OK) {
     SendRegistrationError(thread_id, request_id, status);
     return;
   }
 
+  ServiceWorkerVersion* version = context_->GetLiveVersion(version_id);
+  DCHECK(version);
+  DCHECK_EQ(registration_id, version->registration_id());
+  int handle_id = RegisterServiceWorkerHandle(
+      ServiceWorkerHandle::Create(context_, this, thread_id, version));
   Send(new ServiceWorkerMsg_ServiceWorkerRegistered(
-      thread_id, request_id, version_id));
+      thread_id, request_id, handle_id));
 }
 
 void ServiceWorkerDispatcherHost::OnWorkerStarted(
@@ -291,6 +307,11 @@ void ServiceWorkerDispatcherHost::OnSendMessageToBrowser(
       embedded_worker_id, request_id, message);
 }
 
+void ServiceWorkerDispatcherHost::OnServiceWorkerObjectDestroyed(
+    int handle_id) {
+  handles_.Remove(handle_id);
+}
+
 void ServiceWorkerDispatcherHost::UnregistrationComplete(
     int32 thread_id,
     int32 request_id,

[kinuko, 2014-04-09 06:18:37]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-04-09 06:18:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 06:45:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 06:53:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 07:00:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 07:06:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 07:13:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 07:20:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 07:26:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 07:32:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 07:41:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[kinuko, 2014-04-09 07:43:24]

Committed patchset #5 manually as r262636 (presubmit successful).

[commit-bot: I haz the power, 2014-04-09 07:48:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/224733014/110001

[commit-bot: I haz the power, 2014-04-09 07:48:46]

Failed to apply patch for
content/browser/service_worker/service_worker_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/service_worker/service_worker_browsertest.cc
  Hunk #1 FAILED at 223.
  1 out of 1 hunk FAILED -- saving rejects to file
content/browser/service_worker/service_worker_browsertest.cc.rej

Patch:       content/browser/service_worker/service_worker_browsertest.cc
Index: content/browser/service_worker/service_worker_browsertest.cc
diff --git a/content/browser/service_worker/service_worker_browsertest.cc
b/content/browser/service_worker/service_worker_browsertest.cc
index
544405ab8b90fa5f72f044b173ccc7d309594495..7536592a55bc63823fe3a660fae8e7030ae6ff9e
100644
--- a/content/browser/service_worker/service_worker_browsertest.cc
+++ b/content/browser/service_worker/service_worker_browsertest.cc
@@ -223,14 +223,8 @@ class ServiceWorkerVersionBrowserTest : public
ServiceWorkerBrowserTest {
   virtual ~ServiceWorkerVersionBrowserTest() {}
 
   virtual void TearDownOnIOThread() OVERRIDE {
-    if (registration_) {
-      registration_->Shutdown();
-      registration_ = NULL;
-    }
-    if (version_) {
-      version_->Shutdown();
-      version_ = NULL;
-    }
+    registration_ = NULL;
+    version_ = NULL;
   }
 
   void InstallTestHelper(const std::string& worker_url,