Stop using GetDefaultProfile() in Chrome OS implementation of platform_util::OpenExternal()

chrome/browser/external_protocol/external_protocol_handler.cc

Index: chrome/browser/external_protocol/external_protocol_handler.cc
diff --git a/chrome/browser/external_protocol/external_protocol_handler.cc b/chrome/browser/external_protocol/external_protocol_handler.cc
index 2f9b4d9f985fc9776f11fa0ac4e1066c93ca1315..291652d5e929122ab34c6b95924798160ba6051b 100644
--- a/chrome/browser/external_protocol/external_protocol_handler.cc
+++ b/chrome/browser/external_protocol/external_protocol_handler.cc
@@ -17,8 +17,11 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/platform_util.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
 #include "net/base/escape.h"
 #include "url/gurl.h"
 
@@ -70,11 +73,15 @@ void RunExternalProtocolDialogWithDelegate(
 
 void LaunchUrlWithoutSecurityCheckWithDelegate(
     const GURL& url,
+    int render_process_host_id,
+    int tab_contents_id,
     ExternalProtocolHandler::Delegate* delegate) {
-  if (!delegate)
-    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(url);
-  else
+  if (!delegate) {
+    ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
+        url, render_process_host_id, tab_contents_id);
+  } else {
     delegate->LaunchUrlWithoutSecurityCheck(url);
+  }
 }
 
 // When we are about to launch a URL with the default OS level application,
@@ -124,7 +131,8 @@ class ExternalDefaultProtocolObserver
       return;
     }
 
-    LaunchUrlWithoutSecurityCheckWithDelegate(escaped_url_, delegate_);
+    LaunchUrlWithoutSecurityCheckWithDelegate(
+        escaped_url_, render_process_host_id_, tab_contents_id_, delegate_);
   }
 
   virtual bool IsOwnedByWorker() OVERRIDE { return true; }
@@ -282,18 +290,17 @@ void ExternalProtocolHandler::LaunchUrlWithDelegate(const GURL& url,
 }
 
 // static
-void ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(const GURL& url) {
-#if defined(OS_MACOSX)
-  // This must run on the UI thread on OS X.
-  platform_util::OpenExternal(url);
-#else
-  // Otherwise put this work on the file thread. On Windows ShellExecute may
-  // block for a significant amount of time, and it shouldn't hurt on Linux.
-  BrowserThread::PostTask(
-      BrowserThread::FILE,
-      FROM_HERE,
-      base::Bind(&platform_util::OpenExternal, url));
-#endif
+void ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck(
+    const GURL& url,
+    int render_process_host_id,
+    int tab_contents_id) {
+  content::WebContents* web_contents = tab_util::GetWebContentsByID(

[sky, 2013/12/10 17:32:11]

Can't this code be passed the Profile?

[hashimoto, 2013/12/11 08:57:51]

I preferred receiving a pair of int to Profile* because:

1. All users of this function are receiving a pair of int.
 This function is called in a callback chain starting from
ChromeResourceDispatcherHostDelegate::HandleExternalProtocol() who posts a task
to run ExternalProtocolHandler::LaunchUrl().
 ExternalProtocolHandler::LaunchUrl() receives a pair of int, and other
external_protocol_dialog related codes are also passing a pair of int around.
 Passing a pair of int as arguments here looks consistent with neighbors. (and
user codes do not need to call tab_util::GetWebContentsByID by themselves)

2. This function is called asynchronously.
 This function is called asynchronously by CreateShellWorker (instantiated in
LaunchUrlWithDelegate() above) or external_protocol_dialog UI code.
 I think calling GetWebContentsByID() and perform NULL check against its result
here is safer than doing it earlier and using the result raw pointer of Profile
later.

I might be too nervous about #2 because it seems ProfileManager does not delete
Profile until it gets destroyed in
ChromeBrowserMainParts::PostMainMessageLoopRun().
If it's OK to stop worrying about use-after-free of Profile*, I will change this
file to call GetWebContentsByID() in
ExternalProtocolHandler::LaunchUrlWithDelegate() and pass a raw pointer of
Profile to this function.

[sky, 2013/12/11 15:19:25]

If this goes through different threads, keep what you have here.

+      render_process_host_id, tab_contents_id);
+  if (!web_contents)
+    return;
+
+  platform_util::OpenExternal(
+      Profile::FromBrowserContext(web_contents->GetBrowserContext()), url);
 }
 
 // static