Fixes a race condition when a shared worker exits while one parent is loading

it.
Changed the shared worker startup code to assign a route ID at the time that we
initially lookup the worker, and pass that same route ID in when we later try
to create the worker, to gracefully handle this race condition.

BUG=29243

TEST=existing tests suffice (can't reproduce race condition in tests)

Review URL: http://codereview.chromium.org/600103

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

13 files changed, 88 insertions, 58 deletions

diff --git a/chrome/browser/renderer_host/resource_message_filter.cc b/chrome/browser/renderer_host/resource_message_filter.cc
index d5f7a55..92efd4b 100644
--- a/chrome/browser/renderer_host/resource_message_filter.cc
+++ b/chrome/browser/renderer_host/resource_message_filter.cc
@@ -761,23 +761,20 @@ void ResourceMessageFilter::OnLaunchNaCl(
 
 void ResourceMessageFilter::OnCreateWorker(
     const ViewHostMsg_CreateWorker_Params& params, int* route_id) {
-  *route_id = render_widget_helper_->GetNextRoutingID();
+  *route_id = params.route_id != MSG_ROUTING_NONE ?
+      params.route_id : render_widget_helper_->GetNextRoutingID();
   WorkerService::GetInstance()->CreateWorker(
       params.url, params.is_shared, off_the_record(), params.name,
       params.document_id, id(), params.render_view_route_id, this, *route_id);
 }
 
-void ResourceMessageFilter::OnLookupSharedWorker(const GURL& url,
-                                                 const string16& name,
-                                                 unsigned long long document_id,
-                                                 int render_view_route_id,
-                                                 int* route_id,
-                                                 bool* url_mismatch) {
-  int new_route_id = render_widget_helper_->GetNextRoutingID();
-  bool worker_found = WorkerService::GetInstance()->LookupSharedWorker(
-      url, name, off_the_record(), document_id, id(), render_view_route_id,
-      this, new_route_id, url_mismatch);
-  *route_id = worker_found ? new_route_id : MSG_ROUTING_NONE;
+void ResourceMessageFilter::OnLookupSharedWorker(
+    const ViewHostMsg_CreateWorker_Params& params, bool* exists, int* route_id,
+    bool* url_mismatch) {
+  *route_id = render_widget_helper_->GetNextRoutingID();
+  *exists = WorkerService::GetInstance()->LookupSharedWorker(
+      params.url, params.name, off_the_record(), params.document_id, id(),
+      params.render_view_route_id, this, *route_id, url_mismatch);
 }
 
 void ResourceMessageFilter::OnDocumentDetached(unsigned long long document_id) {
diff --git a/chrome/browser/renderer_host/resource_message_filter.h b/chrome/browser/renderer_host/resource_message_filter.h
index 062ac7d..82070fb 100644
--- a/chrome/browser/renderer_host/resource_message_filter.h
+++ b/chrome/browser/renderer_host/resource_message_filter.h
@@ -174,10 +174,8 @@ class ResourceMessageFilter : public IPC::ChannelProxy::MessageFilter,
                     IPC::Message* reply_msg);
   void OnCreateWorker(const ViewHostMsg_CreateWorker_Params& params,
                       int* route_id);
-  void OnLookupSharedWorker(const GURL& url,
-                            const string16& name,
-                            unsigned long long document_id,
-                            int render_view_route_id,
+  void OnLookupSharedWorker(const ViewHostMsg_CreateWorker_Params& params,
+                            bool* exists,
                             int* route_id,
                             bool* url_error);
   void OnDocumentDetached(unsigned long long document_id);
diff --git a/chrome/browser/worker_host/worker_process_host.cc b/chrome/browser/worker_host/worker_process_host.cc
index 3c3113d..f366b98 100644
--- a/chrome/browser/worker_host/worker_process_host.cc
+++ b/chrome/browser/worker_host/worker_process_host.cc
@@ -386,24 +386,22 @@ void WorkerProcessHost::UpdateTitle() {
   set_name(ASCIIToWide(display_title));
 }
 
-void WorkerProcessHost::OnLookupSharedWorker(const GURL& url,
-                                             const string16& name,
-                                             unsigned long long document_id,
-                                             int render_view_route_id,
-                                             int* route_id,
-                                             bool* url_mismatch) {
-  int new_route_id = WorkerService::GetInstance()->next_worker_route_id();
+void WorkerProcessHost::OnLookupSharedWorker(
+    const ViewHostMsg_CreateWorker_Params& params,
+    bool* exists,
+    int* route_id,
+    bool* url_mismatch) {
+  *route_id = WorkerService::GetInstance()->next_worker_route_id();
   // TODO(atwilson): Add code to pass in the current worker's document set for
   // these nested workers. Code below will not work for SharedWorkers as it
   // only looks at a single parent.
   DCHECK(instances_.front().worker_document_set()->documents().size() == 1);
   WorkerDocumentSet::DocumentInfoSet::const_iterator first_parent =
       instances_.front().worker_document_set()->documents().begin();
-  bool worker_found = WorkerService::GetInstance()->LookupSharedWorker(
-      url, name, instances_.front().off_the_record(), document_id,
-      first_parent->renderer_id(), first_parent->render_view_route_id(), this,
-      new_route_id, url_mismatch);
-  *route_id = worker_found ? new_route_id : MSG_ROUTING_NONE;
+  *exists = WorkerService::GetInstance()->LookupSharedWorker(
+      params.url, params.name, instances_.front().off_the_record(),
+      params.document_id, first_parent->renderer_id(),
+      first_parent->render_view_route_id(), this, *route_id, url_mismatch);
 }
 
 void WorkerProcessHost::OnCreateWorker(
@@ -415,7 +413,8 @@ void WorkerProcessHost::OnCreateWorker(
   DCHECK(instances_.front().worker_document_set()->documents().size() == 1);
   WorkerDocumentSet::DocumentInfoSet::const_iterator first_parent =
       instances_.front().worker_document_set()->documents().begin();
-  *route_id = WorkerService::GetInstance()->next_worker_route_id();
+  *route_id = params.route_id == MSG_ROUTING_NONE ?
+      WorkerService::GetInstance()->next_worker_route_id() : params.route_id;
   WorkerService::GetInstance()->CreateWorker(
       params.url, params.is_shared, instances_.front().off_the_record(),
       params.name, params.document_id, first_parent->renderer_id(),
diff --git a/chrome/browser/worker_host/worker_process_host.h b/chrome/browser/worker_host/worker_process_host.h
index bed53b5..6d1df6d 100644
--- a/chrome/browser/worker_host/worker_process_host.h
+++ b/chrome/browser/worker_host/worker_process_host.h
@@ -122,10 +122,8 @@ class WorkerProcessHost : public ChildProcessHost {
   void OnWorkerContextClosed(int worker_route_id);
 
   // Called if a worker tries to connect to a shared worker.
-  void OnLookupSharedWorker(const GURL& url,
-                            const string16& name,
-                            unsigned long long document_id,
-                            int render_view_route_id,
+  void OnLookupSharedWorker(const ViewHostMsg_CreateWorker_Params& params,
+                            bool* exists,
                             int* route_id,
                             bool* url_error);
 
diff --git a/chrome/browser/worker_host/worker_service.cc b/chrome/browser/worker_host/worker_service.cc
index fda28d4..f021e24 100644
--- a/chrome/browser/worker_host/worker_service.cc
+++ b/chrome/browser/worker_host/worker_service.cc
@@ -94,13 +94,16 @@ bool WorkerService::CreateWorkerFromInstance(
     WorkerProcessHost::WorkerInstance* existing_instance =
         FindSharedWorkerInstance(
             instance.url(), instance.name(), instance.off_the_record());
+    WorkerProcessHost::WorkerInstance::SenderInfo sender_info =
+        instance.GetSender();
     // If this worker is already running, no need to create a new copy. Just
     // inform the caller that the worker has been created.
     if (existing_instance) {
-      // TODO(atwilson): Change this to scan the sender list (crbug.com/29243).
-      WorkerProcessHost::WorkerInstance::SenderInfo sender_info =
-          instance.GetSender();
-      existing_instance->AddSender(sender_info.first, sender_info.second);
+      // Walk the worker's sender list to see if this client is listed. If not,
+      // then it means that the worker started by the client already exited so
+      // we should not attach to this new one (http://crbug.com/29243).
+      if (!existing_instance->HasSender(sender_info.first, sender_info.second))
+        return false;
       sender_info.first->Send(new ViewMsg_WorkerCreated(sender_info.second));
       return true;
     }
@@ -108,11 +111,13 @@ bool WorkerService::CreateWorkerFromInstance(
     // Look to see if there's a pending instance.
     WorkerProcessHost::WorkerInstance* pending = FindPendingInstance(
         instance.url(), instance.name(), instance.off_the_record());
-    // If there's no instance *and* no pending instance, then it means the
+    // If there's no instance *and* no pending instance (or there is a pending
+    // instance but it does not contain our sender info), then it means the
     // worker started up and exited already. Log a warning because this should
     // be a very rare occurrence and is probably a bug, but it *can* happen so
     // handle it gracefully.
-    if (!pending) {
+    if (!pending ||
+        !pending->HasSender(sender_info.first, sender_info.second)) {
       DLOG(WARNING) << "Pending worker already exited";
       return false;
     }
@@ -121,6 +126,11 @@ bool WorkerService::CreateWorkerFromInstance(
     // worker to the new instance.
     DCHECK(!pending->worker_document_set()->IsEmpty());
     instance.ShareDocumentSet(*pending);
+    for (WorkerProcessHost::WorkerInstance::SenderList::const_iterator i =
+             pending->senders().begin();
+         i != pending->senders().end(); ++i) {
+      instance.AddSender(i->first, i->second);
+    }
     RemovePendingInstances(
         instance.url(), instance.name(), instance.off_the_record());
 
@@ -186,8 +196,7 @@ bool WorkerService::LookupSharedWorker(const GURL &url,
   }
 
   // Add our route ID to the existing instance so we can send messages to it.
-  if (found_instance)
-    instance->AddSender(sender, sender_route_id);
+  instance->AddSender(sender, sender_route_id);
 
   // Add the passed sender/document_id to the worker instance.
   instance->worker_document_set()->Add(
@@ -500,9 +509,6 @@ void WorkerService::RemovePendingInstances(const GURL& url,
   for (WorkerProcessHost::Instances::iterator iter =
            pending_shared_workers_.begin();
        iter != pending_shared_workers_.end(); ) {
-    // Pending workers should have no senders - only actively running worker
-    // instances have senders.
-    DCHECK(iter->NumSenders() == 0);
     if (iter->Matches(url, name, off_the_record)) {
       iter = pending_shared_workers_.erase(iter);
     } else {
diff --git a/chrome/common/render_messages.h b/chrome/common/render_messages.h
index 257d87e..533c92d 100644
--- a/chrome/common/render_messages.h
+++ b/chrome/common/render_messages.h
@@ -551,6 +551,10 @@ struct ViewHostMsg_CreateWorker_Params {
 
   // RenderView routing id used to send messages back to the parent.
   int render_view_route_id;
+
+  // The route ID to associate with the worker. If MSG_ROUTING_NONE is passed,
+  // a new unique ID is created and assigned to the worker.
+  int route_id;
 };
 
 // Creates a new view via a control message since the view doesn't yet exist.
@@ -2401,6 +2405,7 @@ struct ParamTraits<ViewHostMsg_CreateWorker_Params> {
     WriteParam(m, p.name);
     WriteParam(m, p.document_id);
     WriteParam(m, p.render_view_route_id);
+    WriteParam(m, p.route_id);
   }
   static bool Read(const Message* m, void** iter, param_type* p) {
     return
@@ -2408,7 +2413,8 @@ struct ParamTraits<ViewHostMsg_CreateWorker_Params> {
         ReadParam(m, iter, &p->is_shared) &&
         ReadParam(m, iter, &p->name) &&
         ReadParam(m, iter, &p->document_id) &&
-        ReadParam(m, iter, &p->render_view_route_id);
+        ReadParam(m, iter, &p->render_view_route_id) &&
+        ReadParam(m, iter, &p->route_id);
   }
   static void Log(const param_type& p, std::wstring* l) {
     l->append(L"(");
@@ -2422,6 +2428,8 @@ struct ParamTraits<ViewHostMsg_CreateWorker_Params> {
     l->append(L", ");
     LogParam(p.render_view_route_id, l);
     l->append(L")");
+    LogParam(p.route_id, l);
+    l->append(L")");
   }
 };
 
diff --git a/chrome/common/render_messages_internal.h b/chrome/common/render_messages_internal.h
index 00cf0ed..68a209a 100644
--- a/chrome/common/render_messages_internal.h
+++ b/chrome/common/render_messages_internal.h
@@ -1843,17 +1843,17 @@ IPC_BEGIN_MESSAGES(ViewHost)
                               int /* route_id */)
 
   // This message is sent to the browser to see if an instance of this shared
-  // worker already exists (returns route_id != MSG_ROUTING_NONE). This route
-  // id can be used to forward messages to the worker via ForwardToWorker. If a
+  // worker already exists. If so, it returns exists == true. If a
   // non-empty name is passed, also validates that the url matches the url of
   // the existing worker. If a matching worker is found, the passed-in
   // document_id is associated with that worker, to ensure that the worker
   // stays alive until the document is detached.
-  IPC_SYNC_MESSAGE_CONTROL4_2(ViewHostMsg_LookupSharedWorker,
-                              GURL /* url */,
-                              string16 /* name */,
-                              unsigned long long /* document_id */,
-                              int /* render_view_route_id */,
+  // The route_id returned can be used to forward messages to the worker via
+  // ForwardToWorker if it exists, otherwise it should be passed in to any
+  // future call to CreateWorker to avoid creating duplicate workers.
+  IPC_SYNC_MESSAGE_CONTROL1_3(ViewHostMsg_LookupSharedWorker,
+                              ViewHostMsg_CreateWorker_Params,
+                              bool /* exists */,
                               int /* route_id */,
                               bool /* url_mismatch */)
 
diff --git a/chrome/renderer/render_view.cc b/chrome/renderer/render_view.cc
index c387004..ac6af64 100644
--- a/chrome/renderer/render_view.cc
+++ b/chrome/renderer/render_view.cc
@@ -1987,14 +1987,23 @@ WebSharedWorker* RenderView::createSharedWorker(
     unsigned long long document_id) {
 
   int route_id = MSG_ROUTING_NONE;
+  bool exists = false;
   bool url_mismatch = false;
+  ViewHostMsg_CreateWorker_Params params;
+  params.url = url;
+  params.is_shared = true;
+  params.name = name;
+  params.document_id = document_id;
+  params.render_view_route_id = routing_id_;
+  params.route_id = MSG_ROUTING_NONE;
   Send(new ViewHostMsg_LookupSharedWorker(
-      url, name, document_id, routing_id_, &route_id, &url_mismatch));
+      params, &exists, &route_id, &url_mismatch));
   if (url_mismatch) {
     return NULL;
   } else {
     return new WebSharedWorkerProxy(RenderThread::current(),
                                     document_id,
+                                    exists,
                                     route_id,
                                     routing_id_);
   }
diff --git a/chrome/renderer/websharedworker_proxy.cc b/chrome/renderer/websharedworker_proxy.cc
index c6741c1..afb668f 100644
--- a/chrome/renderer/websharedworker_proxy.cc
+++ b/chrome/renderer/websharedworker_proxy.cc
@@ -11,9 +11,14 @@
 
 WebSharedWorkerProxy::WebSharedWorkerProxy(ChildThread* child_thread,
                                            unsigned long long document_id,
+                                           bool exists,
                                            int route_id,
                                            int render_view_route_id)
-    : WebWorkerBase(child_thread, document_id, route_id, render_view_route_id),
+    : WebWorkerBase(child_thread,
+                    document_id,
+                    exists ? route_id : MSG_ROUTING_NONE,
+                    render_view_route_id),
+      pending_route_id_(route_id),
       connect_listener_(NULL) {
 }
 
@@ -27,7 +32,8 @@ void WebSharedWorkerProxy::startWorkerContext(
     const WebKit::WebString& user_agent,
     const WebKit::WebString& source_code) {
   DCHECK(!isStarted());
-  CreateWorkerContext(script_url, true, name, user_agent, source_code);
+  CreateWorkerContext(script_url, true, name, user_agent, source_code,
+                      pending_route_id_);
 }
 
 void WebSharedWorkerProxy::terminateWorkerContext() {
diff --git a/chrome/renderer/websharedworker_proxy.h b/chrome/renderer/websharedworker_proxy.h
index 4a93ff6..be88871 100644
--- a/chrome/renderer/websharedworker_proxy.h
+++ b/chrome/renderer/websharedworker_proxy.h
@@ -23,6 +23,7 @@ class WebSharedWorkerProxy : public WebKit::WebSharedWorker,
   // If the worker not loaded yet, route_id == MSG_ROUTING_NONE
   WebSharedWorkerProxy(ChildThread* child_thread,
                        unsigned long long document_id,
+                       bool exists,
                        int route_id,
                        int render_view_route_id);
 
@@ -43,6 +44,10 @@ class WebSharedWorkerProxy : public WebKit::WebSharedWorker,
  private:
   void OnWorkerCreated();
 
+  // The id for the placeholder worker instance we've stored on the
+  // browser process (we need to pass this same route id back in when creating
+  // the worker).
+  int pending_route_id_;
   ConnectListener* connect_listener_;
 
   DISALLOW_COPY_AND_ASSIGN(WebSharedWorkerProxy);
diff --git a/chrome/renderer/webworker_base.cc b/chrome/renderer/webworker_base.cc
index e78ef64..2bc10b5 100644
--- a/chrome/renderer/webworker_base.cc
+++ b/chrome/renderer/webworker_base.cc
@@ -54,7 +54,8 @@ void WebWorkerBase::CreateWorkerContext(const GURL& script_url,
                                         bool is_shared,
                                         const string16& name,
                                         const string16& user_agent,
-                                        const string16& source_code) {
+                                        const string16& source_code,
+                                        int pending_route_id) {
   DCHECK(route_id_ == MSG_ROUTING_NONE);
   ViewHostMsg_CreateWorker_Params params;
   params.url = script_url;
@@ -62,6 +63,7 @@ void WebWorkerBase::CreateWorkerContext(const GURL& script_url,
   params.name = name;
   params.document_id = document_id_;
   params.render_view_route_id = render_view_route_id_;
+  params.route_id = pending_route_id;
   IPC::Message* create_message = new ViewHostMsg_CreateWorker(
       params, &route_id_);
   child_thread_->Send(create_message);
diff --git a/chrome/renderer/webworker_base.h b/chrome/renderer/webworker_base.h
index 6c031c8..b6b3da8 100644
--- a/chrome/renderer/webworker_base.h
+++ b/chrome/renderer/webworker_base.h
@@ -31,7 +31,8 @@ class WebWorkerBase : public IPC::Channel::Listener {
                            bool is_shared,
                            const string16& name,
                            const string16& user_agent,
-                           const string16& source_code);
+                           const string16& source_code,
+                           int pending_route_id);
 
   // Returns true if the worker is running (can send messages to it).
   bool IsStarted();
diff --git a/chrome/renderer/webworker_proxy.cc b/chrome/renderer/webworker_proxy.cc
index 553a3ac..e64abf6 100644
--- a/chrome/renderer/webworker_proxy.cc
+++ b/chrome/renderer/webworker_proxy.cc
@@ -46,7 +46,8 @@ void WebWorkerProxy::startWorkerContext(
     const WebURL& script_url,
     const WebString& user_agent,
     const WebString& source_code) {
-  CreateWorkerContext(script_url, false, string16(), user_agent, source_code);
+  CreateWorkerContext(script_url, false, string16(), user_agent, source_code,
+                      MSG_ROUTING_NONE);
 }
 
 void WebWorkerProxy::terminateWorkerContext() {