Fix a declarativeContent crash on pages with iframes, and make removing rules revert their actions.

BUG=172011

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

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

3 files changed, 91 insertions, 26 deletions

diff --git a/chrome/browser/extensions/api/declarative_content/content_rules_registry.cc b/chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
index cff0379..63b8947 100644
--- a/chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
+++ b/chrome/browser/extensions/api/declarative_content/content_rules_registry.cc
@@ -222,8 +222,18 @@ std::string ContentRulesRegistry::RemoveRulesImpl(
     for (std::map<int, std::set<ContentRule*> >::iterator
              it = active_rules_.begin();
          it != active_rules_.end(); ++it) {
-      // Has no effect if the rule wasn't present.
-      it->second.erase(rule);
+      if (ContainsKey(it->second, rule)) {
+        content::WebContents* tab;
+        if (!ExtensionTabUtil::GetTabById(
+                 it->first, profile_, true, NULL, NULL, &tab, NULL)) {
+          LOG(DFATAL) << "Tab id " << it->first
+                      << " still in active_rules_, but tab has been destroyed";
+          continue;
+        }
+        ContentAction::ApplyInfo apply_info = {profile_, tab};
+        rule->actions().Revert(rule->extension_id(), base::Time(), &apply_info);
+        it->second.erase(rule);
+      }
     }
 
     // Remove reference to actual rule.
diff --git a/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc b/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
index 04ecc9a..1c9b3af 100644
--- a/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
+++ b/chrome/browser/extensions/api/declarative_content/declarative_content_apitest.cc
@@ -55,10 +55,10 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest, Overview) {
       "var ShowPageAction = chrome.declarativeContent.ShowPageAction;\n"
       "\n"
       "var rule0 = {\n"
-      "  conditions: [new PageStateMatcher({pageUrl: {hostPrefix: "
-      "\"test1\"}}),\n"
-      "               new PageStateMatcher({css: "
-      "[\"input[type='password']\"]})],\n"
+      "  conditions: [new PageStateMatcher({\n"
+      "                   pageUrl: {hostPrefix: \"test1\"}}),\n"
+      "               new PageStateMatcher({\n"
+      "                   css: [\"input[type='password']\"]})],\n"
       "  actions: [new ShowPageAction()]\n"
       "}\n"
       "\n"
@@ -99,7 +99,8 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest, Overview) {
   // Give the mutation observer a chance to run and send back the
   // matching-selector update.
   ASSERT_TRUE(content::ExecuteScript(tab, std::string()));
-  EXPECT_TRUE(page_action->GetIsVisible(tab_id));
+  EXPECT_TRUE(page_action->GetIsVisible(tab_id))
+      << "Adding a matching element should show the page action.";
 
   // Remove it again to make sure that reverts the action.
   ASSERT_TRUE(content::ExecuteScript(
@@ -107,6 +108,67 @@ IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest, Overview) {
   // Give the mutation observer a chance to run and send back the
   // matching-selector update.
   ASSERT_TRUE(content::ExecuteScript(tab, std::string()));
+  EXPECT_FALSE(page_action->GetIsVisible(tab_id))
+      << "Removing the matching element should hide the page action again.";
+}
+
+// This tests against a renderer crash that was present during development.
+IN_PROC_BROWSER_TEST_F(DeclarativeContentApiTest,
+                       AddExtensionMatchingExistingTabWithDeadFrames) {
+  ext_dir_.WriteManifest(kDeclarativeContentManifest);
+  ext_dir_.WriteFile(
+      FILE_PATH_LITERAL("background.js"),
+      "var PageStateMatcher = chrome.declarativeContent.PageStateMatcher;\n"
+      "var ShowPageAction = chrome.declarativeContent.ShowPageAction;\n"
+      "var onPageChanged = chrome.declarativeContent.onPageChanged;\n"
+      "\n"
+      "function setRules(rules, responseString) {\n"
+      "  onPageChanged.removeRules(undefined, function() {\n"
+      "    onPageChanged.addRules(rules, function() {\n"
+      "      window.domAutomationController.send(responseString);\n"
+      "    });\n"
+      "  });\n"
+      "};\n");
+  content::WebContents* const tab =
+      browser()->tab_strip_model()->GetWebContentsAt(0);
+  const int tab_id = ExtensionTabUtil::GetTabId(tab);
+
+  ASSERT_TRUE(content::ExecuteScript(
+      tab, "document.body.innerHTML = '<iframe src=\"http://test2\">';"));
+  // Replace the iframe to destroy its WebFrame.
+  ASSERT_TRUE(content::ExecuteScript(
+      tab, "document.body.innerHTML = '<span class=\"foo\">';"));
+
+  const Extension* extension = LoadExtension(ext_dir_.unpacked_path());
+  ASSERT_TRUE(extension);
+  const ExtensionAction* page_action = ExtensionActionManager::Get(
+      browser()->profile())->GetPageAction(*extension);
+  ASSERT_TRUE(page_action);
+  EXPECT_FALSE(page_action->GetIsVisible(tab_id));
+
+  EXPECT_EQ("rule0",
+            ExecuteScriptInBackgroundPage(
+                extension->id(),
+                "setRules([{\n"
+                "  conditions: [new PageStateMatcher({\n"
+                "                   css: [\"span[class=foo]\"]})],\n"
+                "  actions: [new ShowPageAction()]\n"
+                "}], 'rule0');\n"));
+  // Give the renderer a chance to apply the rules change and notify the
+  // browser.
+  ASSERT_TRUE(content::ExecuteScript(tab, std::string()));
+
+  EXPECT_FALSE(tab->IsCrashed());
+  EXPECT_TRUE(page_action->GetIsVisible(tab_id))
+      << "Loading an extension when an open page matches its rules "
+      << "should show the page action.";
+
+  EXPECT_EQ("removed",
+            ExecuteScriptInBackgroundPage(
+                extension->id(),
+                "onPageChanged.removeRules(undefined, function() {\n"
+                "  window.domAutomationController.send('removed');\n"
+                "});\n"));
   EXPECT_FALSE(page_action->GetIsVisible(tab_id));
 }
 
diff --git a/chrome/renderer/extensions/content_watcher.cc b/chrome/renderer/extensions/content_watcher.cc
index b52a083..b41dce5 100644
--- a/chrome/renderer/extensions/content_watcher.cc
+++ b/chrome/renderer/extensions/content_watcher.cc
@@ -59,39 +59,32 @@ void ContentWatcher::OnWatchPages(
   if (new_css_selectors == css_selectors_)
     return;
 
+  matching_selectors_.clear();
   css_selectors_ = new_css_selectors;
 
-  for (std::map<WebKit::WebFrame*,
-                std::vector<base::StringPiece> >::iterator
-           it = matching_selectors_.begin();
-       it != matching_selectors_.end(); ++it) {
-    WebKit::WebFrame* frame = it->first;
-    if (!css_selectors_.empty())
-      EnsureWatchingMutations(frame);
-
-    // Make sure to replace the contents of it->second because it contains
-    // dangling StringPieces that referred into the old css_selectors_ content.
-    it->second = FindMatchingSelectors(frame);
-  }
-
   // For each top-level frame, inform the browser about its new matching set of
   // selectors.
-  struct NotifyVisitor : public content::RenderViewVisitor {
-    explicit NotifyVisitor(ContentWatcher* watcher) : watcher_(watcher) {}
+  struct WatchSelectors : public content::RenderViewVisitor {
+    explicit WatchSelectors(ContentWatcher* watcher) : watcher_(watcher) {}
     virtual bool Visit(content::RenderView* view) OVERRIDE {
+      for (WebKit::WebFrame* frame = view->GetWebView()->mainFrame(); frame;
+           frame = frame->traverseNext(/*wrap=*/false)) {
+        if (!watcher_->css_selectors_.empty())
+          watcher_->EnsureWatchingMutations(frame);
+
+        watcher_->matching_selectors_[frame] =
+            watcher_->FindMatchingSelectors(frame);
+      }
       watcher_->NotifyBrowserOfChange(view->GetWebView()->mainFrame());
       return true;  // Continue visiting.
     }
     ContentWatcher* watcher_;
   };
-  NotifyVisitor visitor(this);
+  WatchSelectors visitor(this);
   content::RenderView::ForEach(&visitor);
 }
 
 void ContentWatcher::DidCreateDocumentElement(WebKit::WebFrame* frame) {
-  // Make sure the frame is represented in the matching_selectors_ map.
-  matching_selectors_[frame];
-
   if (!css_selectors_.empty()) {
     EnsureWatchingMutations(frame);
   }