Remove WebContents::GetRoutingID().

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.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 "chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h"
 
 #include <stdint.h>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 226 matching lines @@
     prerender_contents->Destroy(prerender::FINAL_STATUS_UNSUPPORTED_SCHEME);
     prerender::ReportPrerenderExternalURL();
     return;
   }
 
   // If the URL is in whitelist, we launch it without asking the user and
   // without any additional security checks. Since the URL is whitelisted,
   // we assume it can be executed.
   if (is_whitelisted) {
     ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
-        url, render_process_id, web_contents->GetRoutingID());
+        url, render_process_id,
+        web_contents->GetRenderViewHost()->GetRoutingID());

[Charlie Reis, 2016/11/23 07:53:35]

This line looks potentially wrong to me, before and after the change.  Nick, can
you help me confirm whether there's a bug here?

Looks like LaunchURL is run via a task posted from the IO thread to the UI
thread, with |render_process_id| set to a ResourceRequestInfo's GetChildID. 
That means there's potentially a race where |web_contents| has done a
cross-process navigation and is now showing a different RenderViewHost that
doesn't correspond to render_process_id.

This LaunchUrlWithoutSecurityCheck uses the process ID / routing ID pair to look
up the WebContents, so if we pass in a non-matching process ID and routing ID,
we could get the wrong WebContents.

Maybe we should be returning early at the top of LaunchURL if
web_contents->GetRenderViewHost()->GetProcess()->GetID() isn't equal to
|render_process_id|, similar to how we return if the tab is now gone?

[ncarter (slow), 2016/11/23 18:04:22]

Agree, with charlie that there are bugs here that are worth fixing. In
particular:

 - In general, render_process_ids and routing_ids need to be conveyed as a
   (logical) tuple, since each process has its own namespace of route_ids.
   You never want to take a route_id from one place and pair it with a
   process_id taken from another place.
 - If this request originated from an oopifs, then render_process_id is going
   to differ from web_contents->GetRenderViewHost()->GetProcess()->GetID().
   Thus this routing pair might resolve to nullptr, or event, to a
   totally unrelated tab.
 - As Charlie mentioned, if there is a race with a process-transferring
   navigation in the main frame, it's possible that render_process_id
   pertained to the previous value of RenderViewHost, causing the same
   bug as above.
 - This is aesthetic, not functional, but in the called function,
   the route_id value is named |tab_contents_id|. Route ids don't
   (and can't) uniquely identify a tab; these plumbed arguments
   should be renamed to route_id.

As background, this codepath is what's exercised when you click a mailto: link
on
every platform except Android -- really, we get here for any protocol that isn't
directly handled by chrome, and which might have an OS-registered handler.
The external handler could be an application outside of Chrome, like Outlook.

[alexmos, 2016/11/23 22:46:20]

Nick/Charlie: thanks for pointing out these problems!  Indeed, links for
external protocols are broken in OOPIFs. I've filed
https://bugs.chromium.org/p/chromium/issues/detail?id=668289 with some more
details and discussion. 

Since that would likely require several more changes as well as a test, is it ok
for us to fix that in a followup CL, and land this one with a TODO here?

[Charlie Reis, 2016/11/23 22:48:27]

Yes, a TODO citing that bug is fine.  No need to fix it in this CL.  Thanks!

   } else {
     ExternalProtocolHandler::LaunchUrlWithDelegate(
-        url, render_process_id, web_contents->GetRoutingID(), page_transition,
+        url, render_process_id,
+        web_contents->GetRenderViewHost()->GetRoutingID(), page_transition,
         has_user_gesture, g_external_protocol_handler_delegate);
   }
 }
 
 #if !defined(DISABLE_NACL)
 void AppendComponentUpdaterThrottles(
     net::URLRequest* request,
     const ResourceRequestInfo& info,
     content::ResourceContext* resource_context,
     ResourceType resource_type,
@@ skipping 618 matching lines @@
           base::Bind(&ChromeResourceDispatcherHostDelegate::OnAbortedFrameLoad,
                     base::Unretained(this), url, request_loading_time));
       return;
   }
 
   std::string metric_name = (request_loading_time.InMilliseconds() < 100 ?
       "Net.ErrAborted.Fast" : "Net.ErrAborted.Slow");
   rappor::SampleDomainAndRegistryFromGURL(
       g_browser_process->rappor_service(), metric_name, url);
 }