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}
(cherry picked from commit bbe84115d3dc969bfcf6ca87bebd1f5608db6ecf)
Review URL: https://codereview.chromium.org/1554233005 .
Cr-Commit-Position: refs/branch-heads/2564@{#478}
Cr-Branched-From: 1283eca15bd9f772387f75241576cde7bdec7f54-refs/heads/master@{#359700}
diff --git a/chrome/browser/extensions/tab_helper.cc b/chrome/browser/extensions/tab_helper.cc
index ef9ab802..a7ca271 100644
--- a/chrome/browser/extensions/tab_helper.cc
+++ b/chrome/browser/extensions/tab_helper.cc
@@ -395,10 +395,7 @@
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();
}
}
diff --git a/chrome/browser/extensions/webstore_inline_installer.cc b/chrome/browser/extensions/webstore_inline_installer.cc
index 045d996..e0eaf43 100644
--- a/chrome/browser/extensions/webstore_inline_installer.cc
+++ b/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 @@
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 @@
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::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 @@
// 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();
}
diff --git a/chrome/browser/extensions/webstore_inline_installer.h b/chrome/browser/extensions/webstore_inline_installer.h
index 583a6c351..573e7d6 100644
--- a/chrome/browser/extensions/webstore_inline_installer.h
+++ b/chrome/browser/extensions/webstore_inline_installer.h
@@ -31,6 +31,7 @@
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 @@
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 @@
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);
diff --git a/chrome/browser/extensions/webstore_inline_installer_browsertest.cc b/chrome/browser/extensions/webstore_inline_installer_browsertest.cc
index 9014479..7d29675 100644
--- a/chrome/browser/extensions/webstore_inline_installer_browsertest.cc
+++ b/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 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 @@
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 @@
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) {
diff --git a/chrome/browser/extensions/webstore_inline_installer_factory.cc b/chrome/browser/extensions/webstore_inline_installer_factory.cc
index 06f4592..e96b725 100644
--- a/chrome/browser/extensions/webstore_inline_installer_factory.cc
+++ b/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
diff --git a/chrome/browser/extensions/webstore_inline_installer_factory.h b/chrome/browser/extensions/webstore_inline_installer_factory.h
index b125741..5b891cc 100644
--- a/chrome/browser/extensions/webstore_inline_installer_factory.h
+++ b/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 @@
// 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);
diff --git a/chrome/browser/extensions/webstore_inline_installer_unittest.cc b/chrome/browser/extensions/webstore_inline_installer_unittest.cc
index d415be25b..ae45287 100644
--- a/chrome/browser/extensions/webstore_inline_installer_unittest.cc
+++ b/chrome/browser/extensions/webstore_inline_installer_unittest.cc
@@ -39,6 +39,7 @@
content::WebContents* contents,
const std::string& requestor_url)
: WebstoreInlineInstaller(contents,
+ contents->GetMainFrame(),
"",
GURL(requestor_url),
base::Bind(&TestInstallerCallback)) {
diff --git a/chrome/test/data/extensions/api_test/webstore_inline_install/empty.html b/chrome/test/data/extensions/api_test/webstore_inline_install/empty.html
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/webstore_inline_install/empty.html
