Flush out initial [un]registerServiceWorker roundtrip

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 c531e915e888504b1396aa27dab35bf4db51b7a3..fb2886b64d7a2f49325fb5d73a7b23a83b4694f3 100644
--- a/content/browser/service_worker/service_worker_dispatcher_host.cc
+++ b/content/browser/service_worker/service_worker_dispatcher_host.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/service_worker/service_worker_dispatcher_host.h"
 
+#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/service_worker/service_worker_context.h"
 #include "content/common/service_worker_messages.h"
 #include "ipc/ipc_message_macros.h"
@@ -12,14 +13,14 @@
 namespace content {
 
 ServiceWorkerDispatcherHost::ServiceWorkerDispatcherHost(
+    int render_process_id,
     ServiceWorkerContext* context)
-    : context_(context) {}
+    : render_process_id_(render_process_id), context_(context) {}
 
 ServiceWorkerDispatcherHost::~ServiceWorkerDispatcherHost() {}
 
 bool ServiceWorkerDispatcherHost::OnMessageReceived(const IPC::Message& message,
                                                     bool* message_was_ok) {
-
   if (IPC_MESSAGE_CLASS(message) != ServiceWorkerMsgStart)
     return false;
 
@@ -36,15 +37,45 @@ bool ServiceWorkerDispatcherHost::OnMessageReceived(const IPC::Message& message,
   return handled;
 }
 
-void ServiceWorkerDispatcherHost::OnRegisterServiceWorker(int32 registry_id,
-                                                       const string16& scope,
-                                                       const GURL& script_url) {
-  // TODO(alecflett): Enforce that script_url must have the same

[michaeln, 2013/10/16 02:03:42]

this todo seems important still

-  // origin as the registering document.
+// TODO(alecflett): Store the service_worker_id keyed by (domain+pattern,
+// script) so we don't always return a new service worker id.
+static int32 NextWorkerId() {

[michaeln, 2013/10/16 02:03:42]

I propose we use int64 bit IDs to identify registered workers and return the
value that identifies the registration in response to registration calls. If the
id we return here is not the id we store persistently, we'll have to maintain
some mapping to go between the two and allocate the transient ids on the fly,
which sounds like add complexity that we can avoid.

[alecflett, 2013/10/23 20:43:17]

Done.

+  static int32 service_worker_id = 0;
+  return service_worker_id++;
+}
+
+void ServiceWorkerDispatcherHost::OnRegisterServiceWorker(

[michaeln, 2013/10/16 02:03:42]

I think goals for this method at this time are primarily to provide completion,
send some response, so the client side sees fullfilled or rejected promises and
can make progress. Beyond that is just a TODO.

[alecflett, 2013/10/23 20:43:17]

Except for origin mismatch, so adding that now.

+    int32 thread_id,
+    int32 request_id,
+    const GURL& scope,
+    const GURL& script_url) {

[michaeln, 2013/10/16 02:03:42]

I think there's a requirement that 'scope' and 'script_url' be of the same
origin. That would be a good check to codify at this time and to go thru an
error return path.

[alecflett, 2013/10/23 20:43:17]

Done.

+  // We're using cookie access as a proxy for ServiceWorker
+  // registration permission.
+  // TODO(alecflett): add a ServiceWorker-specific policy query.
+  ChildProcessSecurityPolicyImpl* policy =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+  if (!policy->CanAccessCookiesForOrigin(render_process_id_, scope))

[michaeln, 2013/10/16 02:03:42]

I'd actually like to define a policy interface here and express in that
interface what it is that we're asking. The impl details of how we those policy
decisions are made can come later and the content settings aspects are
ultimately a function of chome. I'm not suggesting that get done now.

ServiceWorkerPolicy {
   virtual bool CanRegisterServiceWorker(...);
   virtual bool CanUnregisterServiceWorker(...);
};


I'm not sure the CanAccessCookiesForOrigin() checks do what you're expecting
them to do? They look to be of a different nature, not permission related, these
checks are site isolation related and will generally return true unless some
unusual conditions are met...
  // Only might return false if the very experimental
  // --enable-strict-site-isolation or --site-per-process flags are used.

I think we could put a todo here for now about these kinds of checks.
// TODO(xxx): deal with site isolation and ChildProcessSecurityPolicyImpl

[alecflett, 2013/10/23 20:43:17]

Done. Adding a TODO.

+    return;
+  if (!policy->CanAccessCookiesForOrigin(render_process_id_, script_url))
+    return;
+
+  Send(new ServiceWorkerMsg_ServiceWorkerRegistered(
+      thread_id, request_id, NextWorkerId()));
 }
 
-void ServiceWorkerDispatcherHost::OnUnregisterServiceWorker(
-    int32 registry_id,
-    const string16& scope) {}
+void ServiceWorkerDispatcherHost::OnUnregisterServiceWorker(int32 thread_id,
+                                                            int32 request_id,
+                                                            const GURL& scope) {
+  // We're using cookie access as a proxy for ServiceWorker
+  // registration permission.
+  // TODO(alecflett): add a ServiceWorker-specific policy query.
+  ChildProcessSecurityPolicyImpl* policy =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+  if (!policy->CanAccessCookiesForOrigin(render_process_id_, scope))
+    return;
+
+  Send(new ServiceWorkerMsg_ServiceWorkerUnregistered(
+      thread_id, request_id, NextWorkerId()));

[michaeln, 2013/10/16 02:03:42]

I still don't understand the intent of returning the id of the now unregistered
SW?

[alecflett, 2013/10/23 20:43:17]

removed.

+}
 
 }  // namespace content