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

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 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/web_contents/web_contents_impl.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 395 matching lines @@
       geolocation_service_context_(new GeolocationServiceContext()),
       accessibility_mode_(
           BrowserAccessibilityStateImpl::GetInstance()->accessibility_mode()),
       audio_stream_monitor_(this),
       virtual_keyboard_requested_(false),
       page_scale_factor_is_one_(true),
       loading_weak_factory_(this) {
   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));
 }
 
 WebContentsImpl::~WebContentsImpl() {
   is_being_destroyed_ = true;
 
   rwh_input_event_router_.reset();
 
   // Delete all RFH pending shutdown, which will lead the corresponding RVH to
@@ skipping 1384 matching lines @@
       !delegate_->ShouldCreateWebContents(
           this, route_id, main_frame_route_id, main_frame_widget_route_id,
           params.window_container_type, params.frame_name, params.target_url,
           partition_id, session_storage_namespace)) {
     if (route_id != MSG_ROUTING_NONE &&
         !RenderViewHost::FromID(render_process_id, route_id)) {
       // If the embedder didn't create a WebContents for this route, we need to
       // 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;
   }
 
   // Create the new web contents. This will automatically create the new
   // WebContentsView. In the future, we may want to create the view separately.
   CreateParams create_params(GetBrowserContext(), site_instance.get());
   create_params.routing_id = route_id;
   create_params.main_frame_routing_id = main_frame_route_id;
   create_params.main_frame_widget_routing_id = main_frame_widget_route_id;
   create_params.main_frame_name = params.frame_name;
@@ skipping 2890 matching lines @@
     const WebContentsObserver::MediaPlayerId& id) {
   FOR_EACH_OBSERVER(WebContentsObserver, observers_, MediaStartedPlaying(id));
 }
 
 void WebContentsImpl::MediaStoppedPlaying(
     const WebContentsObserver::MediaPlayerId& id) {
   FOR_EACH_OBSERVER(WebContentsObserver, observers_, MediaStoppedPlaying(id));
 }
 
 }  // namespace content