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

content/browser/shared_worker/shared_worker_service_impl.cc

Index: content/browser/shared_worker/shared_worker_service_impl.cc
diff --git a/content/browser/shared_worker/shared_worker_service_impl.cc b/content/browser/shared_worker/shared_worker_service_impl.cc
index 03b279369e10145bb8b9bf8ecce92152fd5b7b18..4404a28dce0dd3aa6a1e1efbd655ce9a15fd2d78 100644
--- a/content/browser/shared_worker/shared_worker_service_impl.cc
+++ b/content/browser/shared_worker/shared_worker_service_impl.cc
@@ -14,15 +14,20 @@
 #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 {
@@ -101,6 +106,56 @@ bool TryIncrementWorkerRefCount(int worker_process_id) {
   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 {
@@ -157,6 +212,17 @@ class SharedWorkerServiceImpl::SharedWorkerPendingInstance {
     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);
@@ -294,6 +360,11 @@ blink::WebWorkerCreationError SharedWorkerServiceImpl::CreateWorker(
       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;
@@ -439,6 +510,23 @@ void SharedWorkerServiceImpl::OnSharedWorkerMessageFilterClosing(
     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,