Implement ServiceWorker::postMessage() [Chromium]

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 a02c5690faf9bd6d5de743a6c257decf1dd056b5..389ed4e8796f821255aab311e772b0b6088cf742 100644
--- a/content/browser/service_worker/service_worker_dispatcher_host.cc
+++ b/content/browser/service_worker/service_worker_dispatcher_host.cc
@@ -5,10 +5,12 @@
 #include "content/browser/service_worker/service_worker_dispatcher_host.h"
 
 #include "base/strings/utf_string_conversions.h"
+#include "content/browser/message_port_message_filter.h"
+#include "content/browser/message_port_service.h"
 #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_provider_host.h"
+#include "content/browser/service_worker/service_worker_registration.h"
 #include "content/common/service_worker/embedded_worker_messages.h"
 #include "content/common/service_worker/service_worker_messages.h"
 #include "ipc/ipc_message_macros.h"
@@ -34,10 +36,12 @@ const uint32 kFilteredMessageClasses[] = {
 namespace content {
 
 ServiceWorkerDispatcherHost::ServiceWorkerDispatcherHost(
-    int render_process_id)
-    : BrowserMessageFilter(
-          kFilteredMessageClasses, arraysize(kFilteredMessageClasses)),
-      render_process_id_(render_process_id) {
+    int render_process_id,
+    MessagePortMessageFilter* message_port_message_filter)
+    : BrowserMessageFilter(kFilteredMessageClasses,
+                           arraysize(kFilteredMessageClasses)),
+      render_process_id_(render_process_id),
+      message_port_message_filter_(message_port_message_filter) {
 }
 
 ServiceWorkerDispatcherHost::~ServiceWorkerDispatcherHost() {
@@ -84,6 +88,8 @@ bool ServiceWorkerDispatcherHost::OnMessageReceived(
                         OnAddScriptClient)
     IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_RemoveScriptClient,
                         OnRemoveScriptClient)
+    IPC_MESSAGE_HANDLER(ServiceWorkerHostMsg_PostMessage,
+                        OnPostMessage)
     IPC_MESSAGE_HANDLER(EmbeddedWorkerHostMsg_WorkerStarted,
                         OnWorkerStarted)
     IPC_MESSAGE_HANDLER(EmbeddedWorkerHostMsg_WorkerStopped,
@@ -157,6 +163,47 @@ void ServiceWorkerDispatcherHost::OnUnregisterServiceWorker(
                  request_id));
 }
 
+void ServiceWorkerDispatcherHost::OnPostMessage(
+    int64 registration_id,
+    const base::string16& message,
+    const std::vector<int>& sent_message_port_ids) {
+  if (!context_ || !context_->IsEnabled())
+    return;
+
+  context_->storage()->FindRegistrationForId(
+      registration_id,
+      base::Bind(&ServiceWorkerDispatcherHost::PostMessageFoundRegistration,
+                 this,
+                 message,
+                 sent_message_port_ids));
+}
+
+static void NoOpStatusCallback(ServiceWorkerStatusCode status) {}

[jsbell, 2014/03/13 17:53:27]

BTW, this is necessary since ServiceWorkerVersion::SendMessage() takes a ref not
pointer, although its comment says "|callback| can be null if the sender does
not need to know if the message is successfully sent or not"

I could rework SendMessage() to take a pointer, or fix the comment. I could also
move NoOpStatusCallback into ServiceWorkerVersion.h so it could be re-used.
Thoughts?

[michaeln, 2014/03/13 21:16:42]

No strong opionion. Putting it in the _utils.h file might be nice. Starts to
matter if we notice a fire & forget pattern being used more.

+
+void ServiceWorkerDispatcherHost::PostMessageFoundRegistration(
+    const base::string16& message,
+    const std::vector<int>& sent_message_port_ids,
+    ServiceWorkerStatusCode status,
+    const scoped_refptr<ServiceWorkerRegistration>& result) {
+  // TODO(jsbell): Can we assert anything here?
+  if (status == SERVICE_WORKER_ERROR_NOT_FOUND)

[michaeln, 2014/03/12 23:38:47]

Maybe test for SUCCESS instead, there could be more than one way in which this
can fail. 

I guess we can DCHECK(result) if status == OK.

[jsbell, 2014/03/13 17:53:27]

Done.

+    return;
+
+  std::vector<int> new_routing_ids(sent_message_port_ids.size());
+  for (size_t i = 0; i < sent_message_port_ids.size(); ++i) {
+    new_routing_ids[i] = message_port_message_filter_->GetNextRoutingID();

[michaeln, 2014/03/12 23:38:47]

I'm not so sure about the safety of this message_port_message_filter_ raw ptr.

We're not inside an IPC handler at this point. The bind closure around this
method call can potentially have kept 'this' object alive past our channel
having gone away (along with all the other filters).

[jsbell, 2014/03/13 17:53:27]

Very nice catch. This can be addressed by moving the routing ID allocation into
the OnPostMessage IPC handler, and pass new_routing_ids into the closure.

Which suddenly makes me wonder what happens to all this message port voodoo if
the message ends up not being delivered. I'll try and answer that or ask a guru.

+    MessagePortService::GetInstance()->UpdateMessagePort(
+        sent_message_port_ids[i],
+        message_port_message_filter_,
+        new_routing_ids[i]);
+  }
+
+  // TODO(jsbell): Route message to appropriate version. crbug.com/351797
+  result->active_version()->SendMessage(

[michaeln, 2014/03/12 23:38:47]

Any chance there may be no active version? If not DCHECK or is so, bail out
early in that case.

[jsbell, 2014/03/13 17:53:27]

If window1 made a registration, window2 deletes the registration, then window2
registers again but it's pending, and window1 posts a message, there could be a
registration but not an active one? That'd be a different registration_id
though, wouldn't it?

Making it resilient to no active_version for now, but can change it to a DCHECK.

[michaeln, 2014/03/13 21:16:42]

Resilient sgtm if we're unsure but we need to nail down the semantics of our
growing pile of stuff.

What is the contract that FindRegistrationForXXX holds up, can it find things
that have no active registration? During initial installation, when we're first
firing up a script, seems like thats a time when callers might want to get a
hold of things w/o an active version? But in that case, we could probably use a
FindRegistrationForVersionId(xxxx) which can lookup by id of the
pending_version.

[jsbell, 2014/03/14 18:11:44]

+N for large N.
 
Yes, and per email discussion that's going to be valid for postMessage. This
code will definitely need to change.

There's a related case; if a registration is deleted then a postMessage comes
through, I believe FindRegistrationForXXXX would result in NOT_FOUND. This is
handled above, but that struck me as weird since the sender is holding onto what
it thinks is a registered SW. Lots of fun sequences to test.

+      ServiceWorkerMsg_Message(message, sent_message_port_ids, new_routing_ids),
+      base::Bind(&NoOpStatusCallback));
+}
+
 void ServiceWorkerDispatcherHost::OnProviderCreated(int provider_id) {
   if (!context_)
     return;