Remove WebContents::GetRoutingID().

chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc

Index: chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
diff --git a/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc b/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
index cde562989399e1a5cbb13dff2ccda27a1e7e7b20..988e99c92858bf4baaf35dfcb4c8ccbd83da58a6 100644
--- a/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
+++ b/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
@@ -244,10 +244,12 @@ void LaunchURL(
   // 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);
   }
 }