Service worker registration error support

content/child/service_worker/service_worker_dispatcher.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/child/service_worker/service_worker_dispatcher.h"
 
 #include "base/lazy_instance.h"
 #include "base/threading/thread_local.h"
 #include "content/child/service_worker/web_service_worker_impl.h"
 #include "content/child/thread_safe_sender.h"
 #include "content/common/service_worker_messages.h"
 #include "third_party/WebKit/public/web/WebSecurityOrigin.h"
 
+using WebKit::WebServiceWorkerError;
 using WebKit::WebServiceWorkerProvider;
 using base::ThreadLocalPointer;
 using webkit_glue::WorkerTaskRunner;
 
 namespace content {
 
 namespace {
 
 base::LazyInstance<ThreadLocalPointer<ServiceWorkerDispatcher> >::Leaky
     g_dispatcher_tls = LAZY_INSTANCE_INITIALIZER;
@@ skipping 13 matching lines @@
   g_dispatcher_tls.Pointer()->Set(this);
 }
 
 ServiceWorkerDispatcher::~ServiceWorkerDispatcher() {
   g_dispatcher_tls.Pointer()->Set(kHasBeenDeleted);
 }
 
 void ServiceWorkerDispatcher::OnMessageReceived(const IPC::Message& msg) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ServiceWorkerDispatcher, msg)
-    IPC_MESSAGE_HANDLER(ServiceWorkerMsg_ServiceWorkerRegistered,
-                        OnServiceWorkerRegistered)
+    IPC_MESSAGE_HANDLER(ServiceWorkerMsg_ServiceWorkerRegistered, OnRegistered)
     IPC_MESSAGE_HANDLER(ServiceWorkerMsg_ServiceWorkerUnregistered,
-                        OnServiceWorkerUnregistered)
+                        OnUnregistered)
+    IPC_MESSAGE_HANDLER(ServiceWorkerMsg_ServiceWorkerRegistrationError,
+                        OnRegistrationError)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   DCHECK(handled) << "Unhandled message:" << msg.type();
 }
 
 bool ServiceWorkerDispatcher::Send(IPC::Message* msg) {
   return thread_safe_sender_->Send(msg);
 }
 
 void ServiceWorkerDispatcher::RegisterServiceWorker(
@@ skipping 12 matching lines @@
   DCHECK(callbacks);
   int request_id = pending_callbacks_.Add(callbacks);
   thread_safe_sender_->Send(new ServiceWorkerHostMsg_UnregisterServiceWorker(
       CurrentWorkerId(), request_id, pattern));
 }
 
 ServiceWorkerDispatcher* ServiceWorkerDispatcher::ThreadSpecificInstance(
     ThreadSafeSender* thread_safe_sender) {
   if (g_dispatcher_tls.Pointer()->Get() == kHasBeenDeleted) {
     NOTREACHED() << "Re-instantiating TLS ServiceWorkerDispatcher.";
     g_dispatcher_tls.Pointer()->Set(NULL);

[michaeln, 2013/10/28 20:46:35]

I think i see the potential reason for the !callbacks checks. In this class, the
only way in which request_id's would not be found is if we go down this
NOTREACHED re-initialization code path.

While we still can, it might be better to to return NULL here, instead of
opening the door to odd inputs and states, and to change the consumers to check
for NULL and do an early return if needed.

I'll make a CL for that.

   }
   if (g_dispatcher_tls.Pointer()->Get())
     return g_dispatcher_tls.Pointer()->Get();
 
   ServiceWorkerDispatcher* dispatcher =
       new ServiceWorkerDispatcher(thread_safe_sender);
   if (WorkerTaskRunner::Instance()->CurrentWorkerId())
     webkit_glue::WorkerTaskRunner::Instance()->AddStopObserver(dispatcher);
   return dispatcher;
 }
 
-void ServiceWorkerDispatcher::OnServiceWorkerRegistered(
+void ServiceWorkerDispatcher::OnRegistered(
     int32 thread_id,
     int32 request_id,
     int64 service_worker_id) {
   WebServiceWorkerProvider::WebServiceWorkerCallbacks* callbacks =
       pending_callbacks_.Lookup(request_id);
   DCHECK(callbacks);
   if (!callbacks)
     return;
 
   // the browser has to generate the service_worker_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.
   scoped_ptr<WebServiceWorkerImpl> worker(
       new WebServiceWorkerImpl(service_worker_id));
   callbacks->onSuccess(worker.release());
   pending_callbacks_.Remove(request_id);
 }
 
-void ServiceWorkerDispatcher::OnServiceWorkerUnregistered(
+void ServiceWorkerDispatcher::OnUnregistered(
     int32 thread_id,
     int32 request_id) {
   WebServiceWorkerProvider::WebServiceWorkerCallbacks* callbacks =
       pending_callbacks_.Lookup(request_id);
   DCHECK(callbacks);
   if (!callbacks)
     return;
 
   callbacks->onSuccess(NULL);
   pending_callbacks_.Remove(request_id);
 }
 
+void ServiceWorkerDispatcher::OnRegistrationError(
+    int32 thread_id,
+    int32 request_id,
+    WebServiceWorkerError::ErrorType error_type,
+    string16 message) {
+  WebServiceWorkerProvider::WebServiceWorkerCallbacks* callbacks =
+      pending_callbacks_.Lookup(request_id);
+  DCHECK(callbacks);
+  if (!callbacks)

[michaeln, 2013/10/25 21:41:25]

do we need the test for !callback?

[alecflett, 2013/10/25 23:41:56]

We've needed to do this for security reasons in IDB - error checking on the IPC
boundary.

[michaeln, 2013/10/26 00:41:01]

that makes good sense to me in the browser process, but less so in child
processes, i think we can trust completely ipc params going from browser->child,
and be very wary of param values going from child->browser

[alecflett, 2013/10/28 17:03:14]

Agreed that it's certainly much safer in the browser->child case, but I don't
think we can trust it completely.

But actually trust is just one issue: Going back and looking at IDB history I'm
also remembering that there were shutdown ordering issues that resulted in this
check: Some more details in http://crbug.com/104492

Long explanation: occasionally if a tab was closed while a transaction was
finishing, the renderer-side dispatcher could get cleaned up before the callback
finished. In certain cases, the IPC transaction-cancel message would get sent
from the renderer but wouldn't be serviced by the browser in time to avoid the
attempted callback. End result was a renderer crash.

I think that this addresses the same situation. 

+    return;
+
+  scoped_ptr<WebServiceWorkerError>  error(
+      new WebServiceWorkerError(error_type, message));
+  callbacks->onError(error.release());
+  pending_callbacks_.Remove(request_id);
+}
+
 void ServiceWorkerDispatcher::OnWorkerRunLoopStopped() { delete this; }
 
 }  // namespace content