[Extensions Click-to-Script] (Hopefully) de-flake action runner tests
ExtensionActionRunnerBrowserTest.ActiveScriptsAreDisplayedAndDelayExecution
has been flaky on Windows. Looking at the test, this could be caused by
not waiting a sufficient amount of time after loading the extension for
the extension background page to register a listener for the tab update.
This, in turn, would result in the extension not appearing to want to
run when it should.
Solve this potential issue by dispatching a "ready" message from the
extension once it has registered the listener, and waiting for that
message in the C++ before creating the new tab. Also explicitly wait
for the script to be injected in the case of automatic injection, and
wait for the blocked action to be added for the case of requiring
consent.
Additionally, break apart the test from being a single test that
exercised four different expectations to four tests, each managing a
single expectation. This eliminates the chance of any of the test steps
interfering with one another.
Bug: 867882
Change-Id: I2859bd35573415080e3c70a3e08bf2108430ee28
Reviewed-on: https://chromium-review.googlesource.com/1195844
Commit-Queue: Devlin <[email protected]>
Reviewed-by: Karan Bhatia <[email protected]>
Cr-Commit-Position: refs/heads/master@{#587822}diff --git a/chrome/browser/extensions/extension_action_runner.cc b/chrome/browser/extensions/extension_action_runner.cc
index d650642..ab63e53 100644
--- a/chrome/browser/extensions/extension_action_runner.cc
+++ b/chrome/browser/extensions/extension_action_runner.cc
@@ -67,6 +67,7 @@
browser_context_(web_contents->GetBrowserContext()),
was_used_on_page_(false),
ignore_active_tab_granted_(false),
+ test_observer_(nullptr),
extension_registry_observer_(this),
weak_factory_(this) {
CHECK(web_contents);
@@ -152,6 +153,8 @@
void ExtensionActionRunner::OnWebRequestBlocked(const Extension* extension) {
web_request_blocked_.insert(extension->id());
+ if (test_observer_)
+ test_observer_->OnBlockedActionAdded();
}
int ExtensionActionRunner::GetBlockedActions(const Extension* extension) {
@@ -231,6 +234,9 @@
NotifyChange(extension);
was_used_on_page_ = true;
+
+ if (test_observer_)
+ test_observer_->OnBlockedActionAdded();
}
void ExtensionActionRunner::RunPendingScriptsForExtension(
diff --git a/chrome/browser/extensions/extension_action_runner.h b/chrome/browser/extensions/extension_action_runner.h
index 9ede1c3..dac959d 100644
--- a/chrome/browser/extensions/extension_action_runner.h
+++ b/chrome/browser/extensions/extension_action_runner.h
@@ -43,6 +43,11 @@
class ExtensionActionRunner : public content::WebContentsObserver,
public ExtensionRegistryObserver {
public:
+ class TestObserver {
+ public:
+ virtual void OnBlockedActionAdded() = 0;
+ };
+
explicit ExtensionActionRunner(content::WebContents* web_contents);
~ExtensionActionRunner() override;
@@ -88,6 +93,9 @@
std::unique_ptr<ToolbarActionsBarBubbleDelegate::CloseAction> action) {
default_bubble_close_action_for_testing_ = std::move(action);
}
+ void set_observer_for_testing(TestObserver* observer) {
+ test_observer_ = observer;
+ }
#if defined(UNIT_TEST)
// Only used in tests.
@@ -208,6 +216,8 @@
std::unique_ptr<ToolbarActionsBarBubbleDelegate::CloseAction>
default_bubble_close_action_for_testing_;
+ TestObserver* test_observer_;
+
ScopedObserver<ExtensionRegistry, ExtensionRegistryObserver>
extension_registry_observer_;
diff --git a/chrome/browser/extensions/extension_action_runner_browsertest.cc b/chrome/browser/extensions/extension_action_runner_browsertest.cc
index 76631c8..598e6236 100644
--- a/chrome/browser/extensions/extension_action_runner_browsertest.cc
+++ b/chrome/browser/extensions/extension_action_runner_browsertest.cc
@@ -46,7 +46,8 @@
" code: \"chrome.test.sendMessage('inject succeeded');\"\n"
" });"
"};\n"
- "chrome.tabs.onUpdated.addListener(listener);";
+ "chrome.tabs.onUpdated.addListener(listener);\n"
+ "chrome.test.sendMessage('ready');";
const char kContentScriptSource[] =
"chrome.test.sendMessage('inject succeeded');";
@@ -102,6 +103,12 @@
InjectionType injection_type,
WithholdPermissions withhold_permissions);
+ void RunActiveScriptsTest(const std::string& name,
+ HostType host_type,
+ InjectionType injection_type,
+ WithholdPermissions withhold_permissions,
+ RequiresConsent requires_consent);
+
private:
std::vector<std::unique_ptr<TestExtensionDir>> test_extension_dirs_;
std::vector<const Extension*> extensions_;
@@ -165,7 +172,15 @@
injection_type == CONTENT_SCRIPT ? kContentScriptSource
: kBackgroundScriptSource);
- const Extension* extension = LoadExtension(dir->UnpackedPath());
+ const Extension* extension = nullptr;
+ if (injection_type == CONTENT_SCRIPT) {
+ extension = LoadExtension(dir->UnpackedPath());
+ } else {
+ ExtensionTestMessageListener listener("ready", false);
+ extension = LoadExtension(dir->UnpackedPath());
+ EXPECT_TRUE(listener.WaitUntilSatisfied());
+ }
+
if (extension) {
test_extension_dirs_.push_back(std::move(dir));
extensions_.push_back(extension);
@@ -181,176 +196,112 @@
return extension;
}
-class ActiveScriptTester {
- public:
- ActiveScriptTester(const std::string& name,
- const Extension* extension,
- Browser* browser,
- RequiresConsent requires_consent,
- InjectionType type);
- ~ActiveScriptTester();
+void ExtensionActionRunnerBrowserTest::RunActiveScriptsTest(
+ const std::string& name,
+ HostType host_type,
+ InjectionType injection_type,
+ WithholdPermissions withhold_permissions,
+ RequiresConsent requires_consent) {
+ ASSERT_TRUE(embedded_test_server()->Start());
- testing::AssertionResult Verify();
-
- std::string name() const;
-
- private:
- // Returns the ExtensionActionRunner, or null if one does not exist.
- ExtensionActionRunner* GetExtensionActionRunner();
-
- // Returns true if the extension has a pending request with the
- // ExtensionActionRunner.
- bool WantsToRun();
-
- // The name of the extension, and also the message it sends.
- std::string name_;
-
- // The extension associated with this tester.
- const Extension* extension_;
-
- // The browser the tester is running in.
- Browser* browser_;
-
- // Whether or not the extension has permission to run the script without
- // asking the user.
- RequiresConsent requires_consent_;
-
- // All of these extensions should inject a script (either through content
- // scripts or through chrome.tabs.executeScript()) that sends a message with
- // the |kInjectSucceeded| message.
- std::unique_ptr<ExtensionTestMessageListener> inject_success_listener_;
-};
-
-ActiveScriptTester::ActiveScriptTester(const std::string& name,
- const Extension* extension,
- Browser* browser,
- RequiresConsent requires_consent,
- InjectionType type)
- : name_(name),
- extension_(extension),
- browser_(browser),
- requires_consent_(requires_consent),
- inject_success_listener_(
- new ExtensionTestMessageListener(kInjectSucceeded,
- false /* won't reply */)) {
- inject_success_listener_->set_extension_id(extension->id());
-}
-
-ActiveScriptTester::~ActiveScriptTester() {}
-
-std::string ActiveScriptTester::name() const {
- return name_;
-}
-
-testing::AssertionResult ActiveScriptTester::Verify() {
- if (!extension_)
- return testing::AssertionFailure() << "Could not load extension: " << name_;
+ const Extension* extension =
+ CreateExtension(host_type, injection_type, withhold_permissions);
+ ASSERT_TRUE(extension);
content::WebContents* web_contents =
- browser_ ? browser_->tab_strip_model()->GetActiveWebContents() : NULL;
- if (!web_contents)
- return testing::AssertionFailure() << "No web contents.";
+ browser()->tab_strip_model()->GetActiveWebContents();
+ ASSERT_TRUE(web_contents);
- // Give the extension plenty of time to inject.
- if (!RunAllPendingInRenderer(web_contents))
- return testing::AssertionFailure() << "Could not run pending in renderer.";
+ ExtensionActionRunner* runner =
+ ExtensionActionRunner::GetForWebContents(web_contents);
+ ASSERT_TRUE(runner);
- // Make sure all running tasks are complete.
- content::RunAllPendingInMessageLoop();
+ const bool will_reply = false;
+ ExtensionTestMessageListener inject_success_listener(kInjectSucceeded,
+ will_reply);
+ auto navigate = [this]() {
+ // Navigate to an URL (which matches the explicit host specified in the
+ // extension content_scripts_explicit_hosts). All extensions should
+ // inject the script.
+ ui_test_utils::NavigateToURL(browser(), embedded_test_server()->GetURL(
+ "/extensions/test_file.html"));
+ };
- ExtensionActionRunner* runner = GetExtensionActionRunner();
- if (!runner)
- return testing::AssertionFailure() << "Could not find runner.";
-
- bool wants_to_run = WantsToRun();
-
- // An extension should have an action displayed iff it requires user consent.
- if ((requires_consent_ == REQUIRES_CONSENT && !wants_to_run) ||
- (requires_consent_ == DOES_NOT_REQUIRE_CONSENT && wants_to_run)) {
- return testing::AssertionFailure()
- << "Improper wants to run for " << name_ << ": expected "
- << (requires_consent_ == REQUIRES_CONSENT) << ", found "
- << wants_to_run;
+ if (requires_consent == DOES_NOT_REQUIRE_CONSENT) {
+ // If the extension doesn't require explicit consent, it should inject
+ // automatically right away.
+ navigate();
+ EXPECT_FALSE(runner->WantsToRun(extension));
+ EXPECT_TRUE(inject_success_listener.WaitUntilSatisfied());
+ EXPECT_FALSE(runner->WantsToRun(extension));
+ return;
}
- // If the extension has permission, we should be able to simply wait for it
- // to execute.
- if (requires_consent_ == DOES_NOT_REQUIRE_CONSENT) {
- EXPECT_TRUE(inject_success_listener_->WaitUntilSatisfied());
- return testing::AssertionSuccess();
- }
+ ASSERT_EQ(REQUIRES_CONSENT, requires_consent);
- // Otherwise, we don't have permission, and have to grant it. Ensure the
- // script has *not* already executed.
- if (inject_success_listener_->was_satisfied()) {
- return testing::AssertionFailure() << name_
- << "'s script ran without permission.";
- }
+ class BlockedActionWaiter : public ExtensionActionRunner::TestObserver {
+ public:
+ explicit BlockedActionWaiter(ExtensionActionRunner* runner)
+ : runner_(runner) {
+ runner_->set_observer_for_testing(this);
+ }
+ ~BlockedActionWaiter() { runner_->set_observer_for_testing(nullptr); }
- // If we reach this point, we should always want to run.
- DCHECK(wants_to_run);
+ void Wait() { run_loop_.Run(); }
+
+ private:
+ // ExtensionActionRunner::TestObserver:
+ void OnBlockedActionAdded() override { run_loop_.Quit(); }
+
+ ExtensionActionRunner* runner_;
+ base::RunLoop run_loop_;
+
+ DISALLOW_COPY_AND_ASSIGN(BlockedActionWaiter);
+ };
+
+ BlockedActionWaiter waiter(runner);
+ navigate();
+ waiter.Wait();
+ EXPECT_TRUE(runner->WantsToRun(extension));
+ EXPECT_FALSE(inject_success_listener.was_satisfied());
// Grant permission by clicking on the extension action.
- runner->RunAction(extension_, true);
+ runner->RunAction(extension, true);
// Now, the extension should be able to inject the script.
- EXPECT_TRUE(inject_success_listener_->WaitUntilSatisfied());
+ EXPECT_TRUE(inject_success_listener.WaitUntilSatisfied());
// The extension should no longer want to run.
- wants_to_run = WantsToRun();
- if (wants_to_run) {
- return testing::AssertionFailure()
- << "Extension " << name_ << " still wants to run after injecting.";
- }
-
- return testing::AssertionSuccess();
+ EXPECT_FALSE(runner->WantsToRun(extension));
}
-ExtensionActionRunner* ActiveScriptTester::GetExtensionActionRunner() {
- return ExtensionActionRunner::GetForWebContents(
- browser_ ? browser_->tab_strip_model()->GetActiveWebContents() : nullptr);
+// Load up different combinations of extensions, and verify that script
+// injection is properly withheld and indicated to the user.
+// NOTE: Though these could be parameterized test cases, there's enough
+// bits here that just having a helper method is quite a bit more readable.
+IN_PROC_BROWSER_TEST_F(
+ ExtensionActionRunnerBrowserTest,
+ ActiveScriptsAreDisplayedAndDelayExecution_ExecuteScripts_AllHosts) {
+ RunActiveScriptsTest("execute_scripts_all_hosts", ALL_HOSTS, EXECUTE_SCRIPT,
+ WITHHOLD_PERMISSIONS, REQUIRES_CONSENT);
}
-
-bool ActiveScriptTester::WantsToRun() {
- ExtensionActionRunner* runner = GetExtensionActionRunner();
- return runner ? runner->WantsToRun(extension_) : false;
+IN_PROC_BROWSER_TEST_F(
+ ExtensionActionRunnerBrowserTest,
+ ActiveScriptsAreDisplayedAndDelayExecution_ExecuteScripts_ExplicitHosts) {
+ RunActiveScriptsTest("execute_scripts_explicit_hosts", EXPLICIT_HOSTS,
+ EXECUTE_SCRIPT, WITHHOLD_PERMISSIONS, REQUIRES_CONSENT);
}
-
-IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
- ActiveScriptsAreDisplayedAndDelayExecution) {
- // First, we load up four extensions:
- // - An extension that injects scripts into all hosts,
- // - An extension that injects scripts into explicit hosts,
- // - An extension with a content script that runs on all hosts,
- // - An extension with a content script that runs on explicit hosts.
- // All extensions should require user consent.
- std::vector<std::unique_ptr<ActiveScriptTester>> testers;
- testers.push_back(std::make_unique<ActiveScriptTester>(
- "inject_scripts_all_hosts",
- CreateExtension(ALL_HOSTS, EXECUTE_SCRIPT, WITHHOLD_PERMISSIONS),
- browser(), REQUIRES_CONSENT, EXECUTE_SCRIPT));
- testers.push_back(std::make_unique<ActiveScriptTester>(
- "inject_scripts_explicit_hosts",
- CreateExtension(EXPLICIT_HOSTS, EXECUTE_SCRIPT, WITHHOLD_PERMISSIONS),
- browser(), REQUIRES_CONSENT, EXECUTE_SCRIPT));
- testers.push_back(std::make_unique<ActiveScriptTester>(
- "content_scripts_all_hosts",
- CreateExtension(ALL_HOSTS, CONTENT_SCRIPT, WITHHOLD_PERMISSIONS),
- browser(), REQUIRES_CONSENT, CONTENT_SCRIPT));
- testers.push_back(std::make_unique<ActiveScriptTester>(
- "content_scripts_explicit_hosts",
- CreateExtension(EXPLICIT_HOSTS, CONTENT_SCRIPT, WITHHOLD_PERMISSIONS),
- browser(), REQUIRES_CONSENT, CONTENT_SCRIPT));
-
- // Navigate to an URL (which matches the explicit host specified in the
- // extension content_scripts_explicit_hosts). All four extensions should
- // inject the script.
- ASSERT_TRUE(embedded_test_server()->Start());
- ui_test_utils::NavigateToURL(
- browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
-
- for (const auto& tester : testers)
- EXPECT_TRUE(tester->Verify()) << tester->name();
+IN_PROC_BROWSER_TEST_F(
+ ExtensionActionRunnerBrowserTest,
+ ActiveScriptsAreDisplayedAndDelayExecution_ContentScripts_AllHosts) {
+ RunActiveScriptsTest("content_scripts_all_hosts", ALL_HOSTS, CONTENT_SCRIPT,
+ WITHHOLD_PERMISSIONS, REQUIRES_CONSENT);
+}
+IN_PROC_BROWSER_TEST_F(
+ ExtensionActionRunnerBrowserTest,
+ ActiveScriptsAreDisplayedAndDelayExecution_ContentScripts_ExplicitHosts) {
+ RunActiveScriptsTest("content_scripts_explicit_hosts", EXPLICIT_HOSTS,
+ CONTENT_SCRIPT, WITHHOLD_PERMISSIONS, REQUIRES_CONSENT);
}
// Test that removing an extension with pending injections a) removes the
@@ -546,25 +497,16 @@
web_contents->GetController().GetLastCommittedEntry()->GetUniqueID());
}
+// If we don't withhold permissions, extensions should execute normally.
IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
- ScriptsExecuteWhenNoPermissionsWithheld) {
- // If we don't withhold permissions, extensions should execute normally.
- std::vector<std::unique_ptr<ActiveScriptTester>> testers;
- testers.push_back(std::make_unique<ActiveScriptTester>(
- "content_scripts_all_hosts",
- CreateExtension(ALL_HOSTS, CONTENT_SCRIPT, DONT_WITHHOLD_PERMISSIONS),
- browser(), DOES_NOT_REQUIRE_CONSENT, CONTENT_SCRIPT));
- testers.push_back(std::make_unique<ActiveScriptTester>(
- "inject_scripts_all_hosts",
- CreateExtension(ALL_HOSTS, EXECUTE_SCRIPT, DONT_WITHHOLD_PERMISSIONS),
- browser(), DOES_NOT_REQUIRE_CONSENT, EXECUTE_SCRIPT));
-
- ASSERT_TRUE(embedded_test_server()->Start());
- ui_test_utils::NavigateToURL(
- browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
-
- for (const auto& tester : testers)
- EXPECT_TRUE(tester->Verify()) << tester->name();
+ ScriptsExecuteWhenNoPermissionsWithheld_ContentScripts) {
+ RunActiveScriptsTest("content_scripts_all_hosts", ALL_HOSTS, CONTENT_SCRIPT,
+ DONT_WITHHOLD_PERMISSIONS, DOES_NOT_REQUIRE_CONSENT);
+}
+IN_PROC_BROWSER_TEST_F(ExtensionActionRunnerBrowserTest,
+ ScriptsExecuteWhenNoPermissionsWithheld_ExecuteScripts) {
+ RunActiveScriptsTest("execute_scripts_all_hosts", ALL_HOSTS, EXECUTE_SCRIPT,
+ DONT_WITHHOLD_PERMISSIONS, DOES_NOT_REQUIRE_CONSENT);
}
// A version of the test with the flag off, in order to test that everything
@@ -579,23 +521,14 @@
};
IN_PROC_BROWSER_TEST_F(FlagOffExtensionActionRunnerBrowserTest,
- ScriptsExecuteWhenFlagAbsent) {
- std::vector<std::unique_ptr<ActiveScriptTester>> testers;
- testers.push_back(std::make_unique<ActiveScriptTester>(
- "content_scripts_all_hosts",
- CreateExtension(ALL_HOSTS, CONTENT_SCRIPT, DONT_WITHHOLD_PERMISSIONS),
- browser(), DOES_NOT_REQUIRE_CONSENT, CONTENT_SCRIPT));
- testers.push_back(std::make_unique<ActiveScriptTester>(
- "inject_scripts_all_hosts",
- CreateExtension(ALL_HOSTS, EXECUTE_SCRIPT, DONT_WITHHOLD_PERMISSIONS),
- browser(), DOES_NOT_REQUIRE_CONSENT, EXECUTE_SCRIPT));
-
- ASSERT_TRUE(embedded_test_server()->Start());
- ui_test_utils::NavigateToURL(
- browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
-
- for (const auto& tester : testers)
- EXPECT_TRUE(tester->Verify()) << tester->name();
+ ScriptsExecuteWhenFlagAbsent_ContentScripts) {
+ RunActiveScriptsTest("content_scripts_all_hosts", ALL_HOSTS, CONTENT_SCRIPT,
+ DONT_WITHHOLD_PERMISSIONS, DOES_NOT_REQUIRE_CONSENT);
+}
+IN_PROC_BROWSER_TEST_F(FlagOffExtensionActionRunnerBrowserTest,
+ ScriptsExecuteWhenFlagAbsent_ExecuteScripts) {
+ RunActiveScriptsTest("execute_scripts_all_hosts", ALL_HOSTS, EXECUTE_SCRIPT,
+ DONT_WITHHOLD_PERMISSIONS, DOES_NOT_REQUIRE_CONSENT);
}
} // namespace extensions
