Don't allow inline install if frame is deleted before user accepts

If the frame that called the chrome.webstore.install method to begin an
inline install gets deleted before the user accepts from the dialog, we
don't want the install to continue because a navigation could make it
look like the install request was coming from some unrelated site.

One downside of this approach is that the dialog stays around even after
the frame is deleted, and hitting either accept or cancel buttons both
just cancel the install. It would be better if the dialog is
automatically cancelled, but doing that would involve a lot more
refactoring. The approach in this CL was easier and is probably worth
getting out, and we can improve on it in the future.

BUG=550047

Review URL: https://codereview.chromium.org/1403293008

Cr-Commit-Position: refs/heads/master@{#361218}

chrome/browser/extensions/tab_helper.cc

@@ -395,10 +395,7 @@ void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host,
                    return_route_id);
     scoped_refptr<WebstoreInlineInstaller> installer(
         webstore_inline_installer_factory_->CreateInstaller(
-            web_contents(),
-            webstore_item_id,
-            requestor_url,
-            callback));
+            web_contents(), host, webstore_item_id, requestor_url, callback));
     installer->BeginInstall();
   }
 }

chrome/browser/extensions/webstore_inline_installer.cc

@@ -7,6 +7,7 @@
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_finder.h"
+#include "content/public/browser/navigation_details.h"
 #include "content/public/browser/web_contents.h"
 
 using content::WebContents;
@@ -29,6 +30,7 @@ const char kInitiatedFromPopupError[] =
 
 WebstoreInlineInstaller::WebstoreInlineInstaller(
     content::WebContents* web_contents,
+    content::RenderFrameHost* host,
     const std::string& webstore_item_id,
     const GURL& requestor_url,
     const Callback& callback)
@@ -37,8 +39,8 @@ WebstoreInlineInstaller::WebstoreInlineInstaller(
           Profile::FromBrowserContext(web_contents->GetBrowserContext()),
           callback),
       content::WebContentsObserver(web_contents),
-      requestor_url_(requestor_url) {
-}
+      host_(host),
+      requestor_url_(requestor_url) {}
 
 WebstoreInlineInstaller::~WebstoreInlineInstaller() {}
 
@@ -91,8 +93,8 @@ bool WebstoreInlineInstaller::IsRequestorPermitted(
 }
 
 bool WebstoreInlineInstaller::CheckRequestorAlive() const {
-  // The tab may have gone away - cancel installation in that case.
-  return web_contents() != NULL;
+  // The frame or tab may have gone away - cancel installation in that case.
+  return host_ != nullptr && web_contents() != nullptr;
 }
 
 const GURL& WebstoreInlineInstaller::GetRequestorURL() const {
@@ -176,6 +178,15 @@ bool WebstoreInlineInstaller::CheckRequestorPermitted(
 // Private implementation.
 //
 
+void WebstoreInlineInstaller::DidNavigateAnyFrame(
+    content::RenderFrameHost* render_frame_host,
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  if (!details.is_in_page &&
+      (render_frame_host == host_ || details.is_main_frame))
+    host_ = nullptr;
+}
+
 void WebstoreInlineInstaller::WebContentsDestroyed() {
   AbortInstall();
 }

chrome/browser/extensions/webstore_inline_installer.h

@@ -31,6 +31,7 @@ class WebstoreInlineInstaller : public WebstoreStandaloneInstaller,
   typedef WebstoreStandaloneInstaller::Callback Callback;
 
   WebstoreInlineInstaller(content::WebContents* web_contents,
+                          content::RenderFrameHost* host,
                           const std::string& webstore_item_id,
                           const GURL& requestor_url,
                           const Callback& callback);
@@ -61,6 +62,9 @@ class WebstoreInlineInstaller : public WebstoreStandaloneInstaller,
 
  private:
   // content::WebContentsObserver interface implementation.
+  void DidNavigateAnyFrame(content::RenderFrameHost* render_frame_host,
+                           const content::LoadCommittedDetails& details,
+                           const content::FrameNavigateParams& params) override;
   void WebContentsDestroyed() override;
 
   // Checks whether the install is initiated by a page in a verified site
@@ -68,6 +72,8 @@ class WebstoreInlineInstaller : public WebstoreStandaloneInstaller,
   static bool IsRequestorURLInVerifiedSite(const GURL& requestor_url,
                                            const std::string& verified_site);
 
+  // This corresponds to the frame that initiated the install request.
+  content::RenderFrameHost* host_;
   GURL requestor_url_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(WebstoreInlineInstaller);

chrome/browser/extensions/webstore_inline_installer_browsertest.cc

@@ -18,6 +18,7 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "url/gurl.h"
@@ -35,6 +36,14 @@ const char kTestExtensionId[] = "ecglahbcnmdpdciemllbhojghbkagdje";
 const char kTestDataPath[] = "extensions/api_test/webstore_inline_install";
 const char kCrxFilename[] = "extension.crx";
 
+// A struct for letting us store the actual parameters that were passed to
+// the install callback.
+struct InstallResult {
+  bool success = false;
+  std::string error;
+  webstore_install::Result result = webstore_install::RESULT_LAST;
+};
+
 }  // namespace
 
 class WebstoreInlineInstallerTest : public WebstoreInstallerTest {
@@ -87,46 +96,82 @@ ExtensionInstallPrompt::Delegate* ProgrammableInstallPrompt::delegate_;
 class WebstoreInlineInstallerForTest : public WebstoreInlineInstaller {
  public:
   WebstoreInlineInstallerForTest(WebContents* contents,
+                                 content::RenderFrameHost* host,
                                  const std::string& extension_id,
                                  const GURL& requestor_url,
                                  const Callback& callback)
       : WebstoreInlineInstaller(
             contents,
+            host,
             kTestExtensionId,
             requestor_url,
-            base::Bind(DummyCallback)),
-        programmable_prompt_(NULL) {
-  }
+            base::Bind(&WebstoreInlineInstallerForTest::InstallCallback,
+                       base::Unretained(this))),
+        install_result_target_(nullptr),
+        programmable_prompt_(nullptr) {}
 
   scoped_ptr<ExtensionInstallPrompt> CreateInstallUI() override {
     programmable_prompt_ = new ProgrammableInstallPrompt(web_contents());
     return make_scoped_ptr(programmable_prompt_);
   }
 
+  // Added here to make it public so that test cases can call it below.
+  bool CheckRequestorAlive() const override {
+    return WebstoreInlineInstaller::CheckRequestorAlive();
+  }
+
+  // Tests that care about the actual arguments to the install callback can use
+  // this to receive a copy in |install_result_target|.
+  void set_install_result_target(
+      scoped_ptr<InstallResult>* install_result_target) {
+    install_result_target_ = install_result_target;
+  }
+
  private:
   ~WebstoreInlineInstallerForTest() override {}
 
   friend class base::RefCountedThreadSafe<WebstoreStandaloneInstaller>;
 
-  static void DummyCallback(bool success,
-                            const std::string& error,
-                            webstore_install::Result result) {
+  void InstallCallback(bool success,
+                       const std::string& error,
+                       webstore_install::Result result) {
+    if (install_result_target_) {
+      install_result_target_->reset(new InstallResult);
+      (*install_result_target_)->success = success;
+      (*install_result_target_)->error = error;
+      (*install_result_target_)->result = result;
+    }
   }
 
+  // This can be set by tests that want to get the actual install callback
+  // arguments.
+  scoped_ptr<InstallResult>* install_result_target_;
+
   ProgrammableInstallPrompt* programmable_prompt_;
 };
 
 class WebstoreInlineInstallerForTestFactory :
     public WebstoreInlineInstallerFactory {
+ public:
+  WebstoreInlineInstallerForTestFactory() : last_installer_(nullptr) {}
   ~WebstoreInlineInstallerForTestFactory() override {}
+
+  WebstoreInlineInstallerForTest* last_installer() { return last_installer_; }
+
   WebstoreInlineInstaller* CreateInstaller(
       WebContents* contents,
+      content::RenderFrameHost* host,
       const std::string& webstore_item_id,
       const GURL& requestor_url,
       const WebstoreStandaloneInstaller::Callback& callback) override {
-    return new WebstoreInlineInstallerForTest(
-        contents, webstore_item_id, requestor_url, callback);
+    last_installer_ = new WebstoreInlineInstallerForTest(
+        contents, host, webstore_item_id, requestor_url, callback);
+    return last_installer_;
   }
+
+ private:
+  // The last installer that was created.
+  WebstoreInlineInstallerForTest* last_installer_;
 };
 
 IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest,
@@ -145,6 +190,42 @@ IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest,
   ProgrammableInstallPrompt::Accept();
 }
 
+IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest,
+                       NavigateBeforeInstallConfirmation) {
+  GURL install_url = GenerateTestServerUrl(kAppDomain, "install.html");
+  ui_test_utils::NavigateToURL(browser(), install_url);
+  WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  TabHelper* tab_helper = TabHelper::FromWebContents(web_contents);
+  WebstoreInlineInstallerForTestFactory* factory =
+      new WebstoreInlineInstallerForTestFactory();
+  tab_helper->SetWebstoreInlineInstallerFactoryForTests(factory);
+  RunTestAsync("runTest");
+  while (!ProgrammableInstallPrompt::Ready())
+    base::RunLoop().RunUntilIdle();
+  GURL new_url = GenerateTestServerUrl(kNonAppDomain, "empty.html");
+  web_contents->GetController().LoadURL(
+      new_url, content::Referrer(), ui::PAGE_TRANSITION_LINK, std::string());
+  EXPECT_TRUE(content::WaitForLoadStop(web_contents));
+  ASSERT_NE(factory->last_installer(), nullptr);
+  EXPECT_NE(factory->last_installer()->web_contents(), nullptr);
+  EXPECT_FALSE(factory->last_installer()->CheckRequestorAlive());
+
+  // Right now the way we handle navigations away from the frame that began the
+  // inline install is to just declare the requestor to be dead, but not to
+  // kill the prompt (that would be a better UX, but more complicated to
+  // implement). If we ever do change things to kill the prompt in this case,
+  // the following code can be removed (it verifies that clicking ok on the
+  // dialog does not result in an install).
+  scoped_ptr<InstallResult> install_result;
+  factory->last_installer()->set_install_result_target(&install_result);
+  ASSERT_TRUE(ProgrammableInstallPrompt::Ready());
+  ProgrammableInstallPrompt::Accept();
+  ASSERT_NE(install_result.get(), nullptr);
+  EXPECT_EQ(install_result->success, false);
+  EXPECT_EQ(install_result->result, webstore_install::ABORTED);
+}
+
 // Flaky: https://crbug.com/537526.
 IN_PROC_BROWSER_TEST_F(WebstoreInlineInstallerTest,
                        DISABLED_ShouldBlockInlineInstallFromPopupWindow) {

chrome/browser/extensions/webstore_inline_installer_factory.cc

@@ -10,12 +10,13 @@
 namespace extensions {
 
 WebstoreInlineInstaller* WebstoreInlineInstallerFactory::CreateInstaller(
-      content::WebContents* contents,
-      const std::string& webstore_item_id,
-      const GURL& requestor_url,
-      const WebstoreStandaloneInstaller::Callback& callback) {
-  return new WebstoreInlineInstaller(
-      contents, webstore_item_id, requestor_url, callback);
+    content::WebContents* contents,
+    content::RenderFrameHost* host,
+    const std::string& webstore_item_id,
+    const GURL& requestor_url,
+    const WebstoreStandaloneInstaller::Callback& callback) {
+  return new WebstoreInlineInstaller(contents, host, webstore_item_id,
+                                     requestor_url, callback);
 }
 
 }  // namespace extensions

chrome/browser/extensions/webstore_inline_installer_factory.h

@@ -12,7 +12,7 @@
 #include "chrome/browser/extensions/webstore_standalone_installer.h"
 
 namespace content {
-  class WebContents;
+class WebContents;
 }
 
 class GURL;
@@ -28,6 +28,7 @@ class WebstoreInlineInstallerFactory {
   // Create a new WebstoreInlineInstallerInstance to be owned by the caller.
   virtual WebstoreInlineInstaller* CreateInstaller(
       content::WebContents* contents,
+      content::RenderFrameHost* host,
       const std::string& webstore_item_id,
       const GURL& requestor_url,
       const WebstoreStandaloneInstaller::Callback& callback);

chrome/browser/extensions/webstore_inline_installer_unittest.cc

@@ -39,6 +39,7 @@ TestWebstoreInlineInstaller::TestWebstoreInlineInstaller(
     content::WebContents* contents,
     const std::string& requestor_url)
     : WebstoreInlineInstaller(contents,
+                              contents->GetMainFrame(),
                               "",
                               GURL(requestor_url),
                               base::Bind(&TestInstallerCallback)) {