Resubmit: Block content scripts from executing until user grants permission

chrome/browser/extensions/active_script_controller_browsertest.cc

Index: chrome/browser/extensions/active_script_controller_browsertest.cc
diff --git a/chrome/browser/extensions/active_script_controller_browsertest.cc b/chrome/browser/extensions/active_script_controller_browsertest.cc
index 6234be632b03aae07a700e022288c18b34e73c83..2088752f8c4b3acec079f284660e12ac96620ff3 100644
--- a/chrome/browser/extensions/active_script_controller_browsertest.cc
+++ b/chrome/browser/extensions/active_script_controller_browsertest.cc
@@ -15,7 +15,9 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/ui_test_utils.h"
+#include "content/public/test/browser_test_utils.h"
 #include "extensions/common/feature_switch.h"
+#include "extensions/common/switches.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -61,10 +63,9 @@ enum RequiresConsent {
 
 class ActiveScriptControllerBrowserTest : public ExtensionBrowserTest {
  public:
-  ActiveScriptControllerBrowserTest()
-      : feature_override_(FeatureSwitch::scripts_require_action(),
-                          FeatureSwitch::OVERRIDE_ENABLED) {}
+  ActiveScriptControllerBrowserTest() {}
 
+  virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE;
   virtual void CleanUpOnMainThread() OVERRIDE;
 
   // Returns an extension with the given |host_type| and |injection_type|. If
@@ -75,11 +76,19 @@ class ActiveScriptControllerBrowserTest : public ExtensionBrowserTest {
                                         InjectionType injection_type);
 
  private:
-  FeatureSwitch::ScopedOverride feature_override_;
   ScopedVector<TestExtensionDir> test_extension_dirs_;
   std::vector<const Extension*> extensions_;
 };
 
+void ActiveScriptControllerBrowserTest::SetUpCommandLine(
+    base::CommandLine* command_line) {
+  ExtensionBrowserTest::SetUpCommandLine(command_line);
+  // We append the actual switch to the commandline because it needs to be
+  // passed over to the renderer, which a FeatureSwitch::ScopedOverride will
+  // not do.
+  command_line->AppendSwitch(switches::kEnableScriptsRequireAction);
+}
+
 void ActiveScriptControllerBrowserTest::CleanUpOnMainThread() {
   test_extension_dirs_.clear();
 }
@@ -266,10 +275,7 @@ testing::AssertionResult ActiveScriptTester::Verify() {
 
   // Otherwise, we don't have permission, and have to grant it. Ensure the
   // script has *not* already executed.
-  // Currently, it's okay for content scripts to execute, because we don't
-  // block them.
-  // TODO(rdevlin.cronin): Fix this.
-  if (inject_success_listener_->was_satisfied() && type_ != CONTENT_SCRIPT) {
+  if (inject_success_listener_->was_satisfied()) {
     return testing::AssertionFailure() <<
         name_ << "'s script ran without permission.";
   }
@@ -316,7 +322,7 @@ ExtensionAction* ActiveScriptTester::GetAction() {
 }
 
 IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
-                       ActiveScriptsAreDisplayed) {
+                       ActiveScriptsAreDisplayedAndDelayExecution) {
   base::FilePath active_script_path =
       test_data_dir_.AppendASCII("active_script");
 
@@ -362,7 +368,7 @@ IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
   };
 
   // Navigate to an URL (which matches the explicit host specified in the
-  // extension content_scripts_explicit_hosts). All three extensions should
+  // extension content_scripts_explicit_hosts). All four extensions should
   // inject the script.
   ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
   ui_test_utils::NavigateToURL(
@@ -372,4 +378,91 @@ IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
     EXPECT_TRUE(testers[i].Verify()) << kExtensionNames[i];
 }
 
+// Test that removing an extension with pending injections a) removes the
+// pending injections for that extension, and b) does not affect pending
+// injections for other extensions.
+IN_PROC_BROWSER_TEST_F(ActiveScriptControllerBrowserTest,
+                       RemoveExtensionWithPendingInjections) {
+  const Extension* extension1 = GetOrCreateExtension(ALL_HOSTS, CONTENT_SCRIPT);
+  ASSERT_TRUE(extension1);
+  const Extension* extension2 = GetOrCreateExtension(ALL_HOSTS, EXECUTE_SCRIPT);
+  ASSERT_TRUE(extension2);
+
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(web_contents);
+  ActiveScriptController* active_script_controller =
+      ActiveScriptController::GetForWebContents(web_contents);
+  ASSERT_TRUE(active_script_controller);
+
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
+
+  // Both extensions should have pending requests.
+  EXPECT_TRUE(active_script_controller->GetActionForExtension(extension1));
+  EXPECT_TRUE(active_script_controller->GetActionForExtension(extension2));
+
+  // Unload one of the extensions.
+  UnloadExtension(extension2->id());
+
+  // This is slight hack to achieve a RunPendingInRenderer() method. Since IPCs
+  // are sent synchronously, the renderer will be notified of the extension
+  // being unloaded before the script is executed, and, since ExecuteScript() is
+  // synchronous, the renderer is guaranteed to be done updating scripts.
+  EXPECT_TRUE(content::ExecuteScript(web_contents, "1 == 1;"));
+
+  // We should have pending requests for extension1, but not the removed
+  // extension2.
+  EXPECT_TRUE(active_script_controller->GetActionForExtension(extension1));
+  EXPECT_FALSE(active_script_controller->GetActionForExtension(extension2));
+
+  // We should still be able to run the request for extension1.
+  ExtensionTestMessageListener inject_success_listener(
+      new ExtensionTestMessageListener(kInjectSucceeded,
+                                       false /* won't reply */));
+  active_script_controller->OnClicked(extension1);
+  inject_success_listener.WaitUntilSatisfied();

[not at google - send to devlin, 2014/06/02 21:28:17]

it doesn't seem like you're necessarily testing renderer logic here. the
GetActionForExtension(..) is browser-side logic, and all you're doing here is
clicking on the icon? how about, like with the other tests (Verify() etc),
assert that the script did get injected, the other one didn't, and that no icon
is showing anymore?

[Devlin, 2014/06/02 22:19:16]

We are testing that the script gets injected by waiting until the listener is
satisfied.  If it's not injected, this will fail.  We can also check that the
icon disappears, but that seems like it's simply testing what was tested above,
a pattern which we frequently discourage because it leads to obscenely long
tests which give no added benefit.  But maybe I'm missing something - is there a
reason the execution of the action would be significantly different?

[not at google - send to devlin, 2014/06/02 22:20:19]

good point. renderer is tested.

+}
+
+// A version of the test with the flag off, in order to test that everything
+// still works as expected.
+class FlagOffActiveScriptControllerBrowserTest
+    : public ActiveScriptControllerBrowserTest {
+ private:
+  // Simply don't append the flag.
+  virtual void SetUpCommandLine(base::CommandLine* command_line) OVERRIDE {
+    ExtensionBrowserTest::SetUpCommandLine(command_line);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(FlagOffActiveScriptControllerBrowserTest,
+                       ScriptsExecuteWhenFlagAbsent) {
+  const char* kExtensionNames[] = {
+    "content_scripts_all_hosts",
+    "inject_scripts_all_hosts",
+  };
+  ActiveScriptTester testers[] = {
+    ActiveScriptTester(
+          kExtensionNames[0],
+          GetOrCreateExtension(ALL_HOSTS, CONTENT_SCRIPT),
+          browser(),
+          DOES_NOT_REQUIRE_CONSENT,
+          CONTENT_SCRIPT),
+      ActiveScriptTester(
+          kExtensionNames[1],
+          GetOrCreateExtension(ALL_HOSTS, EXECUTE_SCRIPT),
+          browser(),
+          DOES_NOT_REQUIRE_CONSENT,
+          EXECUTE_SCRIPT),
+  };
+
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/extensions/test_file.html"));
+
+  for (size_t i = 0u; i < arraysize(testers); ++i)
+    EXPECT_TRUE(testers[i].Verify()) << kExtensionNames[i];
+}
+
 }  // namespace extensions