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

chrome/browser/external_protocol/external_protocol_handler.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/external_protocol/external_protocol_handler.h"
 
 #include <set>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
 #include "base/prefs/scoped_user_pref_update.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread.h"
 #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"
 
 using content::BrowserThread;
 
 // Whether we accept requests for launching external protocols. This is set to
 // false every time an external protocol is requested, and set back to true on
 // each user gesture. This variable should only be accessed from the UI thread.
 static bool g_accept_requests = true;
 
@@ skipping 31 matching lines @@
                                                        render_process_host_id,
                                                        routing_id);
   } else {
     delegate->RunExternalProtocolDialog(url, render_process_host_id,
                                         routing_id);
   }
 }
 
 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,
 // we check if that external application will be us. If it is we just ignore
 // the request.
 class ExternalDefaultProtocolObserver
     : public ShellIntegration::DefaultWebClientObserver {
  public:
   ExternalDefaultProtocolObserver(const GURL& escaped_url,
                                   int render_process_host_id,
@@ skipping 29 matching lines @@
     // what the default is, so we proceed.
     if (prompt_user_) {
       // Ask the user if they want to allow the protocol. This will call
       // LaunchUrlWithoutSecurityCheck if the user decides to accept the
       // protocol.
       RunExternalProtocolDialogWithDelegate(escaped_url_,
           render_process_host_id_, tab_contents_id_, delegate_);
       return;
     }
 
-    LaunchUrlWithoutSecurityCheckWithDelegate(escaped_url_, delegate_);
+    LaunchUrlWithoutSecurityCheckWithDelegate(
+        escaped_url_, render_process_host_id_, tab_contents_id_, delegate_);
   }
 
   virtual bool IsOwnedByWorker() OVERRIDE { return true; }
 
  private:
   ExternalProtocolHandler::Delegate* delegate_;
   GURL escaped_url_;
   int render_process_host_id_;
   int tab_contents_id_;
   bool prompt_user_;
@@ skipping 137 matching lines @@
   scoped_refptr<ShellIntegration::DefaultProtocolClientWorker> worker =
       CreateShellWorker(observer, escaped_url.scheme(), delegate);
 
   // Start the check process running. This will send tasks to the FILE thread
   // and when the answer is known will send the result back to the observer on
   // the UI thread.
   worker->StartCheckIsDefault();
 }
 
 // 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
 void ExternalProtocolHandler::RegisterPrefs(PrefRegistrySimple* registry) {
   registry->RegisterDictionaryPref(prefs::kExcludedSchemes);
 }
 
 // static
 void ExternalProtocolHandler::PermitLaunchUrl() {
   DCHECK_EQ(base::MessageLoop::TYPE_UI, base::MessageLoop::current()->type());
   g_accept_requests = true;
 }