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

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 88f58a782fd7544ae316343b06dc0b6dd50546c2..e9f8605666ff332b44940f4745667e35867c4c67 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -413,6 +413,7 @@ WebContentsImpl::WebContentsImpl(BrowserContext* browser_context)
   frame_tree_.SetFrameRemoveListener(
       base::Bind(&WebContentsImpl::OnFrameRemoved,
                  base::Unretained(this)));
+  loader_io_thread_notifier_.reset(new LoaderIOThreadNotifierImpl(this));
   media_web_contents_observer_.reset(new MediaWebContentsObserver(this));
   wake_lock_service_context_.reset(new WakeLockServiceContext(this));
 }
@@ -1817,7 +1818,9 @@ void WebContentsImpl::CreateNewWindow(
       // delete the RenderView that had already been created.
       Send(new ViewMsg_Close(route_id));
     }
-    GetRenderViewHost()->GetProcess()->ResumeRequestsForView(route_id);
+    LoaderIOThreadNotifierImpl::ResumeBlockedRequestsForFrameInternal(

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

I presume that it's safe to just target the frame rather than the view here
because this view won't have had a chance to create more frames at this point in
the code?  Worth documenting that if so.

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

Help me understand the context in which this code path is taken?  The existence
of this interface is a wart (IMO) and I'd love to find some way around needing
it.  

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

It's rather confusing :/ but I believe what is happening is that we need to
block requests that'll end up in new windows until the frame is ready and
initialized. This bit of code gets run when the WebContents decides it doesn't
want to be responsible for the new window (and creating a new WebContents).
Thus, it resumes requests and lets its delegate decide what to do (this happens
in ShouldCreateWebContents).

This is my thinking of what can happen:
1. The delegate will create one with the given IDs. This should go through a
similar path wrt creating a new WebContents (same process / route).
2. The delegate will do some other logic (like nothing, or open a new tab,
etc.). In these cases, it seems like the requests should not be blocked, but
cancelled. Because the view is never created, my thinking is that these requests
just sit in the RDHI blocked_loaders_map_. This is arguably better than
servicing the requests, but still seems wrong.

Some preliminary investigation for whether we can remove this now that NPAPI is
gone started out promising, but I tested a CL that does it here:
https://codereview.chromium.org/1567903002
and it looks like WebView depends on it. I'm going to keep it in the meantime
until that gets sorted out.

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

Done.

+        GlobalFrameRoutingId(GetRenderViewHost()->GetProcess()->GetID(),

[nasko, 2016/01/07 00:43:09]

Why are you getting the process id from the RenderViewHost? It is the
RenderFrameHost one, right?

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

Good catch. Actually, I think we should be grabbing the process from
source_site_instance (if we're opening in same process). So to fix these
problems, I think the new code should be

if (site_instance == source_site_instance) {
  ResumeBlockedRequestsForFrameInternal(GlobalFrameRoutingId(render_process_id,
main_frame_id));
}

+                             main_frame_route_id));
     return;
   }