fix external protocol handling for OOPIFs

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 e5a36cc6d8ca2e66af63e593511b455b1402a60d..f24882ee0d12f33ba62f12b5c70d5c9b5b317a23 100644
--- a/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
+++ b/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
@@ -220,7 +220,6 @@ void SendExecuteMimeTypeHandlerEvent(
 
 void LaunchURL(
     const GURL& url,
-    int render_process_id,
     const content::ResourceRequestInfo::WebContentsGetter& web_contents_getter,
     ui::PageTransition page_transition,
     bool has_user_gesture,
@@ -243,15 +242,11 @@ void LaunchURL(
   // 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.
-  // TODO(davidsac): External protocol handling needs to be
-  // fixed for OOPIFs.  See https://crbug.com/668289.
   if (is_whitelisted) {
-    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
-        url, render_process_id,
-        web_contents->GetRenderViewHost()->GetRoutingID());
+    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(url, web_contents);
   } else {
     ExternalProtocolHandler::LaunchUrlWithDelegate(
-        url, render_process_id,
+        url, web_contents->GetRenderViewHost()->GetProcess()->GetID(),

[mmenke, 2016/12/15 20:54:54]

How can we be sure the process associated with the WebContents hasn't changed in
the meantime?

[Charlie Reis, 2016/12/15 21:55:06]

Good point.  Maybe we should be passing both the process ID and the routing ID
in the PostTask?

[davidsac (gone - try alexmos), 2016/12/15 22:40:23]

Yes, we could keep passing the process_id to LaunchURL and cancel the external
protocol launch here if it doesn't match the WebContents' current process there.

But wouldn't it still be ok to launch the external app in this case, as once we
launch the external app, there's really no guarantee that it will come up while
the page that requested it is still the current one in WebContents?  Hence it
might not matter if the WebContents changed processes.

The |web_contents| here is used for two things: getting the BrowserContext when
launching the external app, and for putting up a confirmation dialog to ask the
user whether to launch unknown protocols.  Those happen after the call here, and
the dialog is preceded by a hop to the FILE thread and back, so seems like it
could already end up being shown for the wrong page.  

[mmenke, 2016/12/16 16:24:17]

Why do we need the process ID for either of those things?  Surely we aren't
running the confirmation dialog in the untrusted renderer process?

[alexmos, 2016/12/16 21:11:38]

The process ID and routing ID are just used to get the WebContents which is then
needed for both of these.  

ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck() uses that WebContents
to get the BrowserContext, which is used to launch the external app (if we end
up launching gmail for mailto, profile is needed to navigate in the right one). 
LaunchUrlWithDelegate needs the WebContents to put up the unknown protocol
dialog (depending on platform, this is needed to parent the ui window properly,
etc.), and then calls LaunchUrlWithoutSecurityCheck.

Here's my understanding of the possible races here.  There are two flows:

(a) No dialog, i.e. the |is_whitelisted| flow above: (1) request(IO) -> (2)
LaunchURL(UI) -> (3) LaunchUrlWithoutSecurityCheck(UI) which launches the
external app

Previously, the WebContents was resolved with a RVH process_id/routing_id pair
in (3).  If WebContents went away between (1) and (2), we exited early with a
null check on |web_contents| above in (2).  If the main frame process changed
during the thread hop, we would either fail to find the WebContents (because the
old RFH/RVH went away) and hit the same null check, or we would still find it
and use it in (3) because the old RVH is around in swapped-out state (e.g., when
the RFH is pending deletion), which the old code did not check.

With this CL, the WebContents is now resolved in (2) via |web_contents_getter|,
which internally uses RFH process_id/routing_id (using that instead of RVH IDs
fixes this for OOPIFs).  If the WC went away between (1) and (2), we return
early on the same null check, so no behavior change.  If the main frame process
changed, we still end up either returning early on the same null check, or
proceeding if the old RFH is still around and pending deletion, so again no real
behavior change.  (There is a slight difference in that the swapped-out RVH
might stay around after RFH goes away, which I think makes the new code a bit
saner.)

I'm guessing part of your question is whether we want to detect that the main
frame process changed and stop the external app/dialog if so.  That might be
reasonable, but given that it was already possible in the old code, I don't know
if we should be fixing it here.  Note that this only makes sense for the dialog
-- I don't think we can guarantee anything about when the external handler app
is launched.


b) With dialog: (1) request(IO) -> (2) LaunchURL(UI) -> (3)
CreateShellWorker(FILE) -> (4) OnDefaultProtocolClientWorkerFinished(UI) -> (5)
optional dialog(UI) -> (6) LaunchUrlWithoutSecurityCheck(UI) (if confirmed)

This is actually quite similar.  With the old code, we resolved and null-checked
the WebContents in (5) using RVH IDs, and then again in (6).  Same kinds of
races are possible, except that they can also happen during the hop to the FILE
thread and back.

With the new code, we resolve the WebContents in (2) and pass down its current
RVH IDs through to (5) (this is the line you're asking about).  Then we resolve
it again in (5) and (6), and those all have null checks because WC could go away
between (3) and (5).  Again, I don't think we're changing behavior for any of
the races.

         web_contents->GetRenderViewHost()->GetRoutingID(), page_transition,
         has_user_gesture, g_external_protocol_handler_delegate);
   }
@@ -566,11 +561,12 @@ bool ChromeResourceDispatcherHostDelegate::HandleExternalProtocol(
     // content page.
     return false;
   }
-  int child_id = info->GetChildID();
+
 #if BUILDFLAG(ENABLE_EXTENSIONS)
   // External protocols are disabled for guests. An exception is made for the
   // "mailto" protocol, so that pages that utilize it work properly in a
   // WebView.
+  int child_id = info->GetChildID();
   ChromeNavigationUIData* navigation_data =
       static_cast<ChromeNavigationUIData*>(info->GetNavigationUIData());
   if ((extensions::WebViewRendererState::GetInstance()->IsGuest(child_id) ||
@@ -592,8 +588,7 @@ bool ChromeResourceDispatcherHostDelegate::HandleExternalProtocol(
       url_state == policy::URLBlacklist::URLBlacklistState::URL_IN_WHITELIST;
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
-      base::Bind(&LaunchURL, url, child_id,
-                 info->GetWebContentsGetterForRequest(),
+      base::Bind(&LaunchURL, url, info->GetWebContentsGetterForRequest(),
                  info->GetPageTransition(), info->HasUserGesture(),
                  is_whitelisted));
   return true;