Make CrxInstaller creation take install prompt as a scoped_ptr

This helps to clarify that callers are passing ownership of the
ExtensionInstallPrompt that they pass in. Doing this turned up some cases
in crx_installer_browsertest.cc where we were accessing the raw pointer to
the MockInstallPrompt after handing off ownership of it, so this also 
introduces a MockPromptProxy class to save the information we need in tests
but avoids holding on to a possibly invalid pointer.

BUG=269049

TBR=phajdan.jr,benjhayden,pkasting,joaodasilva,bauerb (for mechanical changes in various files)

Review URL: https://chromiumcodereview.appspot.com/22272005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@216749 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 127 insertions, 59 deletions

diff --git a/chrome/browser/extensions/crx_installer.cc b/chrome/browser/extensions/crx_installer.cc
index a80d1e0..68bc4b2 100644
--- a/chrome/browser/extensions/crx_installer.cc
+++ b/chrome/browser/extensions/crx_installer.cc
@@ -72,23 +72,31 @@ enum OffStoreInstallDecision {
 }  // namespace
 
 // static
+scoped_refptr<CrxInstaller> CrxInstaller::CreateSilent(
+    ExtensionService* frontend) {
+  return new CrxInstaller(frontend->AsWeakPtr(),
+                          scoped_ptr<ExtensionInstallPrompt>(),
+                          NULL);
+}
+
+// static
 scoped_refptr<CrxInstaller> CrxInstaller::Create(
     ExtensionService* frontend,
-    ExtensionInstallPrompt* client) {
-  return new CrxInstaller(frontend->AsWeakPtr(), client, NULL);
+    scoped_ptr<ExtensionInstallPrompt> client) {
+  return new CrxInstaller(frontend->AsWeakPtr(), client.Pass(), NULL);
 }
 
 // static
 scoped_refptr<CrxInstaller> CrxInstaller::Create(
     ExtensionService* service,
-    ExtensionInstallPrompt* client,
+    scoped_ptr<ExtensionInstallPrompt> client,
     const WebstoreInstaller::Approval* approval) {
-  return new CrxInstaller(service->AsWeakPtr(), client, approval);
+  return new CrxInstaller(service->AsWeakPtr(), client.Pass(), approval);
 }
 
 CrxInstaller::CrxInstaller(
     base::WeakPtr<ExtensionService> service_weak,
-    ExtensionInstallPrompt* client,
+    scoped_ptr<ExtensionInstallPrompt> client,
     const WebstoreInstaller::Approval* approval)
     : install_directory_(service_weak->install_directory()),
       install_source_(Manifest::INTERNAL),
@@ -97,7 +105,8 @@ CrxInstaller::CrxInstaller(
       delete_source_(false),
       create_app_shortcut_(false),
       service_weak_(service_weak),
-      client_(client),
+      // See header file comment on |client_| for why we use a raw pointer here.
+      client_(client.release()),
       apps_require_extension_mime_type_(false),
       allow_silent_install_(false),
       install_cause_(extension_misc::INSTALL_CAUSE_UNSET),
diff --git a/chrome/browser/extensions/crx_installer.h b/chrome/browser/extensions/crx_installer.h
index 6f71868..d2bcbbb 100644
--- a/chrome/browser/extensions/crx_installer.h
+++ b/chrome/browser/extensions/crx_installer.h
@@ -76,18 +76,21 @@ class CrxInstaller
     NumOffStoreInstallAllowReasons
   };
 
-  // Extensions will be installed into service->install_directory(),
-  // then registered with |service|. Any install UI will be displayed
-  // using |client|. Pass NULL for |client| for silent install
+  // Extensions will be installed into service->install_directory(), then
+  // registered with |service|. This does a silent install - see below for
+  // other options.
+  static scoped_refptr<CrxInstaller> CreateSilent(ExtensionService* service);
+
+  // Same as above, but use |client| to generate a confirmation prompt.
   static scoped_refptr<CrxInstaller> Create(
       ExtensionService* service,
-      ExtensionInstallPrompt* client);
+      scoped_ptr<ExtensionInstallPrompt> client);
 
   // Same as the previous method, except use the |approval| to bypass the
   // prompt. Note that the caller retains ownership of |approval|.
   static scoped_refptr<CrxInstaller> Create(
       ExtensionService* service,
-      ExtensionInstallPrompt* client,
+      scoped_ptr<ExtensionInstallPrompt> client,
       const WebstoreInstaller::Approval* approval);
 
   // Install the crx in |source_file|.
@@ -199,7 +202,7 @@ class CrxInstaller
   friend class ExtensionCrxInstallerTest;
 
   CrxInstaller(base::WeakPtr<ExtensionService> service_weak,
-               ExtensionInstallPrompt* client,
+               scoped_ptr<ExtensionInstallPrompt> client,
                const WebstoreInstaller::Approval* approval);
   virtual ~CrxInstaller();
 
diff --git a/chrome/browser/extensions/crx_installer_browsertest.cc b/chrome/browser/extensions/crx_installer_browsertest.cc
index 112772f..c288691 100644
--- a/chrome/browser/extensions/crx_installer_browsertest.cc
+++ b/chrome/browser/extensions/crx_installer_browsertest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/memory/ref_counted.h"
 #include "chrome/browser/download/download_crx_util.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
@@ -29,19 +30,54 @@ namespace extensions {
 
 namespace {
 
-// Observer waits for exactly one download to finish.
+class MockInstallPrompt;
 
-class MockInstallPrompt : public ExtensionInstallPrompt {
+// This class holds information about things that happen with a
+// MockInstallPrompt. We create the MockInstallPrompt but need to pass
+// ownership of it to CrxInstaller, so it isn't safe to hang this data on
+// MockInstallPrompt itself becuase we can't guarantee it's lifetime.
+class MockPromptProxy :
+      public base::RefCountedThreadSafe<MockPromptProxy> {
  public:
-  explicit MockInstallPrompt(content::WebContents* web_contents) :
-      ExtensionInstallPrompt(web_contents),
-      confirmation_requested_(false),
-      extension_(NULL) {}
+  explicit MockPromptProxy(content::WebContents* web_contents);
 
-  bool did_succeed() const { return !!extension_; }
-  const Extension* extension() const { return extension_; }
+  bool did_succeed() const { return !extension_id_.empty(); }
+  const std::string& extension_id() { return extension_id_; }
   bool confirmation_requested() const { return confirmation_requested_; }
   const string16& error() const { return error_; }
+
+  // To have any effect, this should be called before CreatePrompt.
+  void set_record_oauth2_grant(bool record_oauth2_grant) {
+    record_oauth2_grant_.reset(new bool(record_oauth2_grant));
+  }
+
+  void set_extension_id(const std::string& id) { extension_id_ = id; }
+  void set_confirmation_requested() { confirmation_requested_ = true; }
+  void set_error(const string16& error) { error_ = error; }
+
+  scoped_ptr<ExtensionInstallPrompt> CreatePrompt();
+
+ private:
+  friend class base::RefCountedThreadSafe<MockPromptProxy>;
+  virtual ~MockPromptProxy();
+
+  // Data used to create a prompt.
+  content::WebContents* web_contents_;
+  scoped_ptr<bool> record_oauth2_grant_;
+
+  // Data reported back to us by the prompt we created.
+  bool confirmation_requested_;
+  std::string extension_id_;
+  string16 error_;
+};
+
+class MockInstallPrompt : public ExtensionInstallPrompt {
+ public:
+  MockInstallPrompt(content::WebContents* web_contents,
+                    MockPromptProxy* proxy) :
+      ExtensionInstallPrompt(web_contents),
+      proxy_(proxy) {}
+
   void set_record_oauth2_grant(bool record) { record_oauth2_grant_ = record; }
 
   // Overriding some of the ExtensionInstallUI API.
@@ -49,27 +85,43 @@ class MockInstallPrompt : public ExtensionInstallPrompt {
       Delegate* delegate,
       const Extension* extension,
       const ShowDialogCallback& show_dialog_callback) OVERRIDE {
-    confirmation_requested_ = true;
+    proxy_->set_confirmation_requested();
     delegate->InstallUIProceed();
   }
   virtual void OnInstallSuccess(const Extension* extension,
                                 SkBitmap* icon) OVERRIDE {
-    extension_ = extension;
+    proxy_->set_extension_id(extension->id());
     base::MessageLoopForUI::current()->Quit();
   }
   virtual void OnInstallFailure(const CrxInstallerError& error) OVERRIDE {
-    error_ = error.message();
+    proxy_->set_error(error.message());
     base::MessageLoopForUI::current()->Quit();
   }
 
  private:
-  bool confirmation_requested_;
-  string16 error_;
-  const Extension* extension_;
+  scoped_refptr<MockPromptProxy> proxy_;
 };
 
-MockInstallPrompt* CreateMockInstallPromptForBrowser(Browser* browser) {
-  return new MockInstallPrompt(
+
+MockPromptProxy::MockPromptProxy(content::WebContents* web_contents) :
+    web_contents_(web_contents),
+    confirmation_requested_(false) {
+}
+
+MockPromptProxy::~MockPromptProxy() {}
+
+scoped_ptr<ExtensionInstallPrompt> MockPromptProxy::CreatePrompt() {
+  scoped_ptr<MockInstallPrompt> prompt(
+      new MockInstallPrompt(web_contents_, this));
+  if (record_oauth2_grant_.get())
+    prompt->set_record_oauth2_grant(*record_oauth2_grant_.get());
+  return prompt.PassAs<ExtensionInstallPrompt>();
+}
+
+
+scoped_refptr<MockPromptProxy> CreateMockPromptProxyForBrowser(
+    Browser* browser) {
+  return new MockPromptProxy(
       browser->tab_strip_model()->GetActiveWebContents());
 }
 
@@ -82,7 +134,7 @@ class ExtensionCrxInstallerTest : public ExtensionBrowserTest {
   scoped_refptr<CrxInstaller> InstallWithPrompt(
       const std::string& ext_relpath,
       const std::string& id,
-      MockInstallPrompt* mock_install_prompt) {
+      scoped_refptr<MockPromptProxy> mock_install_prompt) {
     ExtensionService* service = extensions::ExtensionSystem::Get(
         browser()->profile())->extension_service();
     base::FilePath ext_path = test_data_dir_.AppendASCII(ext_relpath);
@@ -103,7 +155,7 @@ class ExtensionCrxInstallerTest : public ExtensionBrowserTest {
 
     scoped_refptr<CrxInstaller> installer(
         CrxInstaller::Create(service,
-                             mock_install_prompt, /* ownership transferred */
+                             mock_install_prompt->CreatePrompt(),
                              approval.get()       /* keep ownership */));
     installer->set_allow_silent_install(true);
     installer->set_is_gallery_install(true);
@@ -124,15 +176,16 @@ class ExtensionCrxInstallerTest : public ExtensionBrowserTest {
     ExtensionService* service = extensions::ExtensionSystem::Get(
         browser()->profile())->extension_service();
 
-    MockInstallPrompt* mock_prompt =
-        CreateMockInstallPromptForBrowser(browser());
+    scoped_refptr<MockPromptProxy> mock_prompt =
+        CreateMockPromptProxyForBrowser(browser());
+
     mock_prompt->set_record_oauth2_grant(record_oauth2_grant);
     scoped_refptr<CrxInstaller> installer =
         InstallWithPrompt("browsertest/scopes", std::string(), mock_prompt);
 
     scoped_refptr<PermissionSet> permissions =
         service->extension_prefs()->GetGrantedPermissions(
-            mock_prompt->extension()->id());
+            mock_prompt->extension_id());
     ASSERT_TRUE(permissions.get());
   }
 
@@ -171,8 +224,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, MAYBE_Whitelisting) {
       browser()->profile())->extension_service();
 
   // Even whitelisted extensions with NPAPI should not prompt.
-  MockInstallPrompt* mock_prompt =
-      CreateMockInstallPromptForBrowser(browser());
+  scoped_refptr<MockPromptProxy> mock_prompt =
+      CreateMockPromptProxyForBrowser(browser());
   scoped_refptr<CrxInstaller> installer =
       InstallWithPrompt("uitest/plugins", id, mock_prompt);
   EXPECT_FALSE(mock_prompt->confirmation_requested());
@@ -225,9 +278,10 @@ IN_PROC_BROWSER_TEST_F(
   std::string crx_path_string(crx_path.value().begin(), crx_path.value().end());
   GURL url = GURL(std::string("file:///").append(crx_path_string));
 
-  MockInstallPrompt* mock_prompt =
-      CreateMockInstallPromptForBrowser(browser());
-  download_crx_util::SetMockInstallPromptForTesting(mock_prompt);
+  scoped_refptr<MockPromptProxy> mock_prompt =
+      CreateMockPromptProxyForBrowser(browser());
+  download_crx_util::SetMockInstallPromptForTesting(
+      mock_prompt->CreatePrompt());
 
   LOG(ERROR) << "PackAndInstallExtension: Getting download manager";
   content::DownloadManager* download_manager =
@@ -278,10 +332,11 @@ IN_PROC_BROWSER_TEST_F(ExtensionCrxInstallerTest, DISABLED_AllowOffStore) {
   const bool kTestData[] = {false, true};
 
   for (size_t i = 0; i < arraysize(kTestData); ++i) {
-    MockInstallPrompt* mock_prompt =
-        CreateMockInstallPromptForBrowser(browser());
+    scoped_refptr<MockPromptProxy> mock_prompt =
+        CreateMockPromptProxyForBrowser(browser());
+
     scoped_refptr<CrxInstaller> crx_installer(
-        CrxInstaller::Create(service, mock_prompt));
+        CrxInstaller::Create(service, mock_prompt->CreatePrompt()));
     crx_installer->set_install_cause(
         extension_misc::INSTALL_CAUSE_USER_DOWNLOAD);
 
diff --git a/chrome/browser/extensions/extension_browsertest.cc b/chrome/browser/extensions/extension_browsertest.cc
index 9aecbc6..2ebf2dc 100644
--- a/chrome/browser/extensions/extension_browsertest.cc
+++ b/chrome/browser/extensions/extension_browsertest.cc
@@ -406,15 +406,15 @@ const Extension* ExtensionBrowserTest::InstallOrUpdateExtension(
   size_t num_before = service->extensions()->size();
 
   {
-    ExtensionInstallPrompt* install_ui = NULL;
+    scoped_ptr<ExtensionInstallPrompt> install_ui;
     if (ui_type == INSTALL_UI_TYPE_CANCEL) {
-      install_ui = new MockAbortExtensionInstallPrompt();
+      install_ui.reset(new MockAbortExtensionInstallPrompt());
     } else if (ui_type == INSTALL_UI_TYPE_NORMAL) {
-      install_ui = new ExtensionInstallPrompt(
-          browser->tab_strip_model()->GetActiveWebContents());
+      install_ui.reset(new ExtensionInstallPrompt(
+          browser->tab_strip_model()->GetActiveWebContents()));
     } else if (ui_type == INSTALL_UI_TYPE_AUTO_CONFIRM) {
-      install_ui = new MockAutoConfirmExtensionInstallPrompt(
-          browser->tab_strip_model()->GetActiveWebContents());
+      install_ui.reset(new MockAutoConfirmExtensionInstallPrompt(
+          browser->tab_strip_model()->GetActiveWebContents()));
     }
 
     // TODO(tessamac): Update callers to always pass an unpacked extension
@@ -427,7 +427,7 @@ const Extension* ExtensionBrowserTest::InstallOrUpdateExtension(
       return NULL;
 
     scoped_refptr<extensions::CrxInstaller> installer(
-        extensions::CrxInstaller::Create(service, install_ui));
+        extensions::CrxInstaller::Create(service, install_ui.Pass()));
     installer->set_expected_id(id);
     installer->set_is_gallery_install(from_webstore);
     installer->set_install_source(install_source);
diff --git a/chrome/browser/extensions/extension_functional_browsertest.cc b/chrome/browser/extensions/extension_functional_browsertest.cc
index 25ceba5..38a8650 100644
--- a/chrome/browser/extensions/extension_functional_browsertest.cc
+++ b/chrome/browser/extensions/extension_functional_browsertest.cc
@@ -27,7 +27,7 @@ public:
         content::NotificationService::AllSources());
 
     scoped_refptr<extensions::CrxInstaller> installer(
-        extensions::CrxInstaller::Create(service, NULL));
+        extensions::CrxInstaller::CreateSilent(service));
     installer->set_is_gallery_install(false);
     installer->set_allow_silent_install(true);
     installer->set_install_source(Manifest::INTERNAL);
diff --git a/chrome/browser/extensions/extension_service.cc b/chrome/browser/extensions/extension_service.cc
index f4e5ae8..325bea2 100644
--- a/chrome/browser/extensions/extension_service.cc
+++ b/chrome/browser/extensions/extension_service.cc
@@ -626,11 +626,12 @@ bool ExtensionService::UpdateExtension(const std::string& id,
 
   // We want a silent install only for non-pending extensions and
   // pending extensions that have install_silently set.
-  ExtensionInstallPrompt* client = NULL;
+  scoped_ptr<ExtensionInstallPrompt> client;
   if (pending_extension_info && !pending_extension_info->install_silently())
-    client = ExtensionInstallUI::CreateInstallPromptWithProfile(profile_);
+    client.reset(ExtensionInstallUI::CreateInstallPromptWithProfile(profile_));
 
-  scoped_refptr<CrxInstaller> installer(CrxInstaller::Create(this, client));
+  scoped_refptr<CrxInstaller> installer(
+      CrxInstaller::Create(this, client.Pass()));
   installer->set_expected_id(id);
   if (pending_extension_info) {
     installer->set_install_source(pending_extension_info->install_source());
@@ -2728,7 +2729,7 @@ bool ExtensionService::OnExternalExtensionFileFound(
   }
 
   // no client (silent install)
-  scoped_refptr<CrxInstaller> installer(CrxInstaller::Create(this, NULL));
+  scoped_refptr<CrxInstaller> installer(CrxInstaller::CreateSilent(this));
   installer->set_install_source(location);
   installer->set_expected_id(id);
   installer->set_expected_version(*version);
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 0ce3c2a..c0d461e 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -693,7 +693,7 @@ class ExtensionServiceTest
   void StartCRXInstall(const base::FilePath& crx_path, int creation_flags) {
     ASSERT_TRUE(base::PathExists(crx_path))
         << "Path does not exist: "<< crx_path.value().c_str();
-    scoped_refptr<CrxInstaller> installer(CrxInstaller::Create(service_, NULL));
+    scoped_refptr<CrxInstaller> installer(CrxInstaller::CreateSilent(service_));
     installer->set_creation_flags(creation_flags);
     if (!(creation_flags & Extension::WAS_INSTALLED_BY_DEFAULT)) {
       installer->set_allow_silent_install(true);
@@ -779,7 +779,7 @@ class ExtensionServiceTest
     EXPECT_TRUE(base::PathExists(crx_path))
         << "Path does not exist: "<< crx_path.value().c_str();
     // no client (silent install)
-    scoped_refptr<CrxInstaller> installer(CrxInstaller::Create(service_, NULL));
+    scoped_refptr<CrxInstaller> installer(CrxInstaller::CreateSilent(service_));
     installer->set_install_source(install_location);
     installer->InstallCrx(crx_path);
 
@@ -1868,7 +1868,7 @@ TEST_F(ExtensionServiceTest, InstallUserScript) {
              .AppendASCII("user_script_basic.user.js");
 
   ASSERT_TRUE(base::PathExists(path));
-  scoped_refptr<CrxInstaller> installer(CrxInstaller::Create(service_, NULL));
+  scoped_refptr<CrxInstaller> installer(CrxInstaller::CreateSilent(service_));
   installer->set_allow_silent_install(true);
   installer->InstallUserScript(
       path,
@@ -1898,7 +1898,7 @@ TEST_F(ExtensionServiceTest, InstallExtensionDuringShutdown) {
   service_->set_browser_terminating_for_test(true);
 
   base::FilePath path = data_dir_.AppendASCII("good.crx");
-  scoped_refptr<CrxInstaller> installer(CrxInstaller::Create(service_, NULL));
+  scoped_refptr<CrxInstaller> installer(CrxInstaller::CreateSilent(service_));
   installer->set_allow_silent_install(true);
   installer->InstallCrx(path);
   base::RunLoop().RunUntilIdle();
diff --git a/chrome/browser/extensions/updater/extension_updater_unittest.cc b/chrome/browser/extensions/updater/extension_updater_unittest.cc
index 3c78899..8b8cb2d 100644
--- a/chrome/browser/extensions/updater/extension_updater_unittest.cc
+++ b/chrome/browser/extensions/updater/extension_updater_unittest.cc
@@ -1218,9 +1218,9 @@ class ExtensionUpdaterTest : public testing::Test {
     extension_service->set_show_extensions_prompts(false);
 
     scoped_refptr<CrxInstaller> fake_crx1(
-        CrxInstaller::Create(extension_service, NULL));
+        CrxInstaller::CreateSilent(extension_service));
     scoped_refptr<CrxInstaller> fake_crx2(
-        CrxInstaller::Create(extension_service, NULL));
+        CrxInstaller::CreateSilent(extension_service));
 
     if (updates_start_running) {
       // Add fake CrxInstaller to be returned by service.UpdateExtension().