Removes the references to shared workers from the all documents in being deleted frames

content/browser/shared_worker/shared_worker_service_impl.cc

 // Copyright 2014 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/browser/shared_worker/shared_worker_service_impl.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/stl_util.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "content/browser/devtools/shared_worker_devtools_manager.h"
+#include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/shared_worker/shared_worker_host.h"
 #include "content/browser/shared_worker/shared_worker_instance.h"
 #include "content/browser/shared_worker/shared_worker_message_filter.h"
 #include "content/browser/shared_worker/worker_document_set.h"
+#include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/view_messages.h"
 #include "content/common/worker_messages.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/worker_service_observer.h"
 
 namespace content {
 
 WorkerService* WorkerService::GetInstance() {
   return SharedWorkerServiceImpl::GetInstance();
 }
 
 namespace {
 
@@ skipping 58 matching lines @@
 
 bool TryIncrementWorkerRefCount(int worker_process_id) {
   RenderProcessHostImpl* render_process = static_cast<RenderProcessHostImpl*>(
       RenderProcessHost::FromID(worker_process_id));
   if (!render_process || render_process->FastShutdownStarted())
     return false;
   render_process->IncrementWorkerRefCount();
   return true;
 }
 
+void CallRenderFrameDetachedOnIO(int render_process_id, int render_frame_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  SharedWorkerServiceImpl::GetInstance()->RenderFrameDetached(render_process_id,
+                                                              render_frame_id);
+}
+
+class WorkerDocumentSetWebContentsObserver : public WebContentsObserver {
+ public:
+  WorkerDocumentSetWebContentsObserver(WebContents* web_contents,
+                                       int render_process_id,
+                                       int render_frame_id,
+                                       RenderFrameHost* render_frame_host)
+      : WebContentsObserver(web_contents),
+        render_process_id_(render_process_id),
+        render_frame_id_(render_frame_id),
+        render_frame_host_(render_frame_host) {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  }
+
+ private:
+  // content::WebContentsObserver overrides.

[no sievers, 2016/08/23 20:32:42]

I wonder if this still needs to also listen to WebContentsDestroyed() since the
filter might outlive the WebContents and ~WebContentsObserver() would deref
|web_contents_|.

But then it would also mean that |this| leaks in that case.
It's not totally obvious how this is all balanced (creating this from
OnCreateWorker and deleting it from RFDeleted). Apologies if my suggestion made
this more complicated, did it maybe?

[horo, 2016/08/24 01:15:49]

Done.
 
The fact that SharedWorkerService related classes live on IO thread makes things
complicated.
I think that calling NotifyRenderFrameDetachedOnIO() from ~RenderFrameHostImpl()
(Patch Set 8) is simpler.

If you agree, I'd like to go with Patch Set 8.

+  void RenderFrameDeleted(RenderFrameHost* render_frame_host) override {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+    if (render_frame_host != render_frame_host_)
+      return;
+    Observe(nullptr);
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                            base::Bind(&CallRenderFrameDetachedOnIO,
+                                       render_process_id_, render_frame_id_));
+    base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
+  }
+
+  const int render_process_id_;
+  const int render_frame_id_;
+  const RenderFrameHost* render_frame_host_;
+};
+
+void RegisterWebContentsObserverOnUI(int render_process_id,
+                                     int render_frame_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  RenderFrameHostImpl* render_frame_host =
+      RenderFrameHostImpl::FromID(render_process_id, render_frame_id);
+  if (!render_frame_host)
+    return;
+  WebContentsImpl* web_contents = static_cast<WebContentsImpl*>(
+      WebContents::FromRenderFrameHost(render_frame_host));
+  new WorkerDocumentSetWebContentsObserver(web_contents, render_process_id,
+                                           render_frame_id, render_frame_host);
+}
+
 }  // namespace
 
 class SharedWorkerServiceImpl::SharedWorkerPendingInstance {
  public:
   struct SharedWorkerPendingRequest {
     SharedWorkerPendingRequest(SharedWorkerMessageFilter* filter,
                                int route_id,
                                unsigned long long document_id,
                                int render_process_id,
                                int render_frame_route_id)
@@ skipping 36 matching lines @@
 
   void RemoveRequest(int process_id) {
     auto to_remove = std::remove_if(
         requests_.begin(), requests_.end(),
         [process_id](const std::unique_ptr<SharedWorkerPendingRequest>& r) {
           return r->render_process_id == process_id;
         });
     requests_.erase(to_remove, requests_.end());
   }
 
+  void RemoveRequestFromFrame(int process_id, int frame_id) {
+    auto to_remove = std::remove_if(
+        requests_.begin(), requests_.end(),
+        [process_id,
+         frame_id](const std::unique_ptr<SharedWorkerPendingRequest>& r) {
+          return r->render_process_id == process_id &&
+                 r->render_frame_route_id == frame_id;
+        });
+    requests_.erase(to_remove, requests_.end());
+  }
+
   void RegisterToSharedWorkerHost(SharedWorkerHost* host) {
     for (const auto& request : requests_) {
       host->AddFilter(request->filter, request->route_id);
       host->worker_document_set()->Add(request->filter,
                                        request->document_id,
                                        request->render_process_id,
                                        request->render_frame_route_id);
     }
   }
 
@@ skipping 117 matching lines @@
     const WorkerStoragePartitionId& partition_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   std::unique_ptr<SharedWorkerInstance> instance(new SharedWorkerInstance(
       params.url, params.name, params.content_security_policy,
       params.security_policy_type, params.creation_address_space,
       resource_context, partition_id, params.creation_context_type));
   std::unique_ptr<SharedWorkerPendingInstance::SharedWorkerPendingRequest>
       request(new SharedWorkerPendingInstance::SharedWorkerPendingRequest(
           filter, route_id, params.document_id, filter->render_process_id(),
           params.render_frame_route_id));
+
+  BrowserThread::PostTask(

[no sievers, 2016/08/23 20:32:42]

Is it possible that there is a pending RenderFrameHost destruction here that
races this task posted to the UI thread and we miss it?

[horo, 2016/08/24 01:15:49]

Yes.
So I check "if (!render_frame_host)" in RegisterWebContentsObserverOnUI.

+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&RegisterWebContentsObserverOnUI, filter->render_process_id(),
+                 params.render_frame_route_id));
   if (SharedWorkerPendingInstance* pending = FindPendingInstance(*instance)) {
     if (params.url != pending->instance()->url())
       return blink::WebWorkerCreationErrorURLMismatch;
     pending->AddRequest(std::move(request));
     if (params.creation_context_type !=
         pending->instance()->creation_context_type())
       return blink::WebWorkerCreationErrorSecureContextMismatch;
     return blink::WebWorkerCreationErrorNone;
   }
 
@@ skipping 125 matching lines @@
   std::vector<int> remove_pending_instance_list;
   for (const auto& it : pending_instances_) {
     it.second->RemoveRequest(filter->render_process_id());
     if (it.second->requests()->empty())
       remove_pending_instance_list.push_back(it.first);
   }
   for (int to_remove : remove_pending_instance_list)
     pending_instances_.erase(to_remove);
 }
 
+void SharedWorkerServiceImpl::RenderFrameDetached(int render_process_id,
+                                                  int render_frame_id) {
+  ScopedWorkerDependencyChecker checker(this);
+  for (const auto& it : worker_hosts_) {
+    it.second->RenderFrameDetached(render_process_id, render_frame_id);
+  }
+
+  std::vector<int> remove_pending_instance_list;
+  for (const auto& it : pending_instances_) {
+    it.second->RemoveRequestFromFrame(render_process_id, render_frame_id);
+    if (it.second->requests()->empty())
+      remove_pending_instance_list.push_back(it.first);

[no sievers, 2016/08/23 20:32:42]

why not erase immediately (while post-incrementing the iterator)?

[horo, 2016/08/24 01:15:49]

Done.

+  }
+  for (int to_remove : remove_pending_instance_list)
+    pending_instances_.erase(to_remove);
+}
+
 void SharedWorkerServiceImpl::NotifyWorkerDestroyed(int worker_process_id,
                                                     int worker_route_id) {
   FOR_EACH_OBSERVER(WorkerServiceObserver,
                     observers_,
                     WorkerDestroyed(worker_process_id, worker_route_id));
 }
 
 blink::WebWorkerCreationError
 SharedWorkerServiceImpl::ReserveRenderProcessToCreateWorker(
     std::unique_ptr<SharedWorkerPendingInstance> pending_instance) {
@@ skipping 194 matching lines @@
     UpdateWorkerDependencyFunc new_func) {
   update_worker_dependency_ = new_func;
 }
 
 void SharedWorkerServiceImpl::ChangeTryIncrementWorkerRefCountFuncForTesting(
     bool (*new_func)(int)) {
   s_try_increment_worker_ref_count_ = new_func;
 }
 
 }  // namespace content