[RDHI] Refactored blocked_loaders_map_ to key by render frame route id

content/browser/loader/loader_io_thread_notifier_impl.cc

Index: content/browser/loader/loader_io_thread_notifier_impl.cc
diff --git a/content/browser/loader/loader_io_thread_notifier_impl.cc b/content/browser/loader/loader_io_thread_notifier_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..90b8ab92be3eb354908cc9e30a18ac0b72ec13dc
--- /dev/null
+++ b/content/browser/loader/loader_io_thread_notifier_impl.cc
@@ -0,0 +1,108 @@
+// Copyright 2015 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/loader/loader_io_thread_notifier_impl.h"
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/memory/scoped_ptr.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/common/browser_side_navigation_policy.h"
+
+namespace content {
+
+namespace {
+
+bool CollectRenderFrameRoutingIds(std::set<GlobalFrameRoutingId>* route_ids,

[Randy Smith (Not in Mondays), 2016/01/06 21:56:29]

I'd value a quick comment explaining the goal of this call.  When I started
reading it I assumed it was to collect all frame routing ids for an entire tree,
but that doesn't look like it's goal based on the implementation.

Putting it near the function that it's a helper for would also be useful.  I
don't think the style guide prohibits putting static functions anywhere in a
file.

[Charlie Harrison, 2016/01/07 22:47:05]

That is the goal, we just don't want to miss any (including pending /
speculative ones). Will comment this.

+                                  FrameTreeNode* tree_node) {

[Randy Smith (Not in Mondays), 2016/01/06 21:56:29]

Style guide says parameter order is inputs, then outputs:
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

[Charlie Harrison, 2016/01/07 22:47:05]

I'm not sure the best way to reconcile this. FrameTree:ForEach requires a
Callback<bool(FrameTreeNode*)>. The currying rules wrt Bind require you to curry
in one direction (i.e. I have to specify route_ids first cause that's the arg
I'm currying away). LMK if you see a simple solution to this (ideally we'd use
an inline lambda and close over route_ids but I don't think they play nice with
Bind).

+  RenderFrameHostImpl* frame_host = tree_node->current_frame_host();

[Randy Smith (Not in Mondays), 2016/01/06 21:56:30]

DCHECK for thread?  I think it's useful being a-r about that in this file.

[Charlie Harrison, 2016/01/07 22:47:05]

Done.

+  RenderFrameHostImpl* pending_frame_host =
+      IsBrowserSideNavigationEnabled()
+          ? tree_node->render_manager()->speculative_frame_host()
+          : tree_node->render_manager()->pending_frame_host();
+  if (frame_host)
+    route_ids->insert(frame_host->GetGlobalFrameRoutingId());
+  if (pending_frame_host)
+    route_ids->insert(pending_frame_host->GetGlobalFrameRoutingId());
+  return true;
+}
+
+void NotifyForEachFrameOnIO(
+    base::Callback<void(ResourceDispatcherHostImpl*,
+                        const GlobalFrameRoutingId&)> frame_callback,
+    scoped_ptr<std::set<GlobalFrameRoutingId>> routing_ids) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();

[Randy Smith (Not in Mondays), 2016/01/06 21:56:29]

What are the lifetime implications here?  Are you sure that there's no way for
the RDHI to be torn down in a race with this message, and if so, why?  (Probably
worthwhile documenting the rationale somewhere.)

[Charlie Harrison, 2016/01/07 22:47:05]

Well if RDHI is torn down then won't Get() return nullptr? In that case this
function will be a no op. The other way some consumers did it was to call Get()
before the hop, so use-after-frees were possible.

+  if (rdh) {
+    for (const auto& routing_id : *routing_ids)
+      frame_callback.Run(rdh, routing_id);
+  }
+}
+
+void NotifyForFrameOnIO(
+    base::Callback<void(ResourceDispatcherHostImpl*,
+                        const GlobalFrameRoutingId&)> frame_callback,
+    const GlobalFrameRoutingId& routing_id) {

[Randy Smith (Not in Mondays), 2016/01/06 21:56:29]

idea (up to you): I find myself wondering about whether implementing the above
function in terms of this function might be reasonable for code re-use.  It's
right at the edge for me (which is why I'm not pushing it).

[Charlie Harrison, 2016/01/07 22:47:05]

Yeah sgtm.

+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();
+  if (rdh)
+    frame_callback.Run(rdh, routing_id);
+}
+
+}  // namespace
+
+// static
+void LoaderIOThreadNotifierImpl::ResumeBlockedRequestsForFrameInternal(
+    const GlobalFrameRoutingId& routing_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // Only resume blocked requests for valid ids. See
+  // RenderWidgetHelper::CreateNewWindow for when an invalid id might be used.
+  if (routing_id.route_id == MSG_ROUTING_NONE)
+    return;
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &NotifyForFrameOnIO,
+          base::Bind(
+              &ResourceDispatcherHostImpl::ResumeBlockedRequestsForFrame),
+          routing_id));
+}
+
+// static
+void LoaderIOThreadNotifierImpl::NotifyForEachFrame(
+    FrameTree* frame_tree,
+    base::Callback<void(ResourceDispatcherHostImpl*,
+                        const GlobalFrameRoutingId&)> frame_callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  scoped_ptr<std::set<GlobalFrameRoutingId>> routing_ids(
+      new std::set<GlobalFrameRoutingId>());
+  frame_tree->ForEach(
+      base::Bind(&CollectRenderFrameRoutingIds, routing_ids.get()));
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&NotifyForEachFrameOnIO, frame_callback,
+                                     base::Passed(std::move(routing_ids))));
+}
+
+LoaderIOThreadNotifierImpl::LoaderIOThreadNotifierImpl(
+    WebContents* web_contents)
+    : WebContentsObserver(web_contents) {}
+
+LoaderIOThreadNotifierImpl::~LoaderIOThreadNotifierImpl() {}
+
+void LoaderIOThreadNotifierImpl::RenderFrameDeleted(
+    RenderFrameHost* render_frame_host) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(
+          &NotifyForFrameOnIO,
+          base::Bind(&ResourceDispatcherHostImpl::OnRenderFrameHostDeleted),

[nasko, 2016/01/07 19:58:39]

This isn't quite correct. RenderFrame is the renderer side object and this
notification tells you that this one was deleted. RenderFrameHost can persist
around, so technically it isn't deleted. Let's try to be precise.

[Charlie Harrison, 2016/01/07 22:47:05]

Fixed the name to refer to the RenderFrame.

+          static_cast<RenderFrameHostImpl*>(render_frame_host)
+              ->GetGlobalFrameRoutingId()));
+}
+
+}  // namespace content