Fix a race condition when an IconAnimation is still running at browser shutdown.

Because Extensions are refcounted, they can be destroyed from
~ProfileIOData on the IO thread instead of the UI thread.  If an
animation is running at that time, and the ExtensionAction owns its
animations, then the animation's destructor will CHECK-fail while
destroying its Timer, because the Timer has bound itself to the UI
thread.

After this patch, IconAnimationWrappers own themselves, and use
WeakPtrs to indirectly remove themselves from the ExtensionAction.  To
limit the size of the ExtensionAction's map, we remove all NULLs each
time we observe a single animation as having finished.

BUG=133141


Review URL: https://chromiumcodereview.appspot.com/10823142

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

chrome/browser/extensions/api/extension_action/script_badge_apitest.cc

@@ -5,30 +5,51 @@
 #include "chrome/browser/extensions/browser_event_router.h"
 #include "chrome/browser/extensions/extension_apitest.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/location_bar_controller.h"
+#include "chrome/browser/extensions/tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/tab_contents/tab_contents.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_action.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/web_contents.h"
+#include "net/base/mock_host_resolver.h"
+#include "net/test/test_server.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 using extensions::Extension;
 
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, ScriptBadge) {
+class ScriptBadgeApiTest : public ExtensionApiTest {
+ protected:
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    ExtensionApiTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitch(switches::kEnableScriptBadges);
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(ScriptBadgeApiTest, Basics) {
   ASSERT_TRUE(test_server()->Start());
   ASSERT_TRUE(RunExtensionTest("script_badge/basics")) << message_;
   const Extension* extension = GetSingleLoadedExtension();
   ASSERT_TRUE(extension) << message_;
-  const ExtensionAction* script_badge = extension->script_badge();
+  ExtensionAction* script_badge = extension->script_badge();
   ASSERT_TRUE(script_badge);
+  const extensions::LocationBarController* location_bar_controller =
+      chrome::GetActiveTabContents(browser())->extension_tab_helper()->
+      location_bar_controller();
 
   const int tab_id = SessionID::IdForTab(
       chrome::GetActiveTabContents(browser()));
   EXPECT_EQ(GURL(extension->GetResourceURL("default_popup.html")),
             script_badge->GetPopupUrl(tab_id));
 
+  EXPECT_FALSE(script_badge->GetIsVisible(tab_id));
+  EXPECT_THAT(location_bar_controller->GetCurrentActions(),
+              testing::ElementsAre());
+
   {
     ResultCatcher catcher;
     // Tell the extension to update the script badge state.
@@ -49,4 +70,15 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, ScriptBadge) {
         browser()->profile(), *script_badge, tab_id);
     EXPECT_TRUE(catcher.GetNextResult());
   }
+
+  {
+    ResultCatcher catcher;
+    // Visit a non-extension page so the extension can run a content script and
+    // cause the script badge to be animated in.
+    ui_test_utils::NavigateToURL(browser(), test_server()->GetURL(""));
+    ASSERT_TRUE(catcher.GetNextResult());
+  }
+  EXPECT_TRUE(script_badge->GetIsVisible(tab_id));
+  EXPECT_THAT(location_bar_controller->GetCurrentActions(),
+              testing::ElementsAre(script_badge));
 }

chrome/browser/extensions/script_badge_controller.cc

@@ -186,6 +186,7 @@ bool ScriptBadgeController::MarkExtensionExecuting(
   if (!script_badge)
     return false;
 
+  script_badge->SetIsVisible(SessionID::IdForTab(tab_contents_), true);
   script_badge->RunIconAnimation(SessionID::IdForTab(tab_contents_));
 
   return true;

chrome/common/extensions/extension.cc

@@ -823,10 +823,10 @@ scoped_ptr<ExtensionAction> Extension::LoadExtensionActionHelper(
     string16* error) {
   scoped_ptr<ExtensionAction> result(new ExtensionAction(id(), action_type));
 
-  // Page actions are hidden/disabled by default, and browser actions are
+  // Page/script actions are hidden/disabled by default, and browser actions are
   // visible/enabled by default.
   result->SetIsVisible(ExtensionAction::kDefaultTabId,
-                       action_type != ExtensionAction::TYPE_PAGE);
+                       action_type == ExtensionAction::TYPE_BROWSER);
 
   if (manifest_version_ == 1) {
     const ListValue* icons = NULL;
@@ -2394,8 +2394,6 @@ bool Extension::LoadScriptBadge(string16* error) {
             IDR_EXTENSIONS_FAVICON).ToSkBitmap());
   }
 
-  script_badge_->SetIsVisible(ExtensionAction::kDefaultTabId, true);
-
   return true;
 }
 

chrome/common/extensions/extension_action.cc

@@ -61,15 +61,15 @@ int Width(const gfx::Image& image) {
 
 }  // namespace
 
-// Wraps an IconAnimation and implements its ui::AnimationDelegate to erase the
-// animation from a map when the animation ends or is cancelled, causing itself
-// and its owned IconAnimation to be deleted.
-class ExtensionAction::IconAnimationWrapper : public ui::AnimationDelegate {
+// Wraps an IconAnimation and implements its ui::AnimationDelegate to delete
+// itself when the animation ends or is cancelled, causing its owned
+// IconAnimation to be destroyed.
+class ExtensionAction::IconAnimationWrapper
+    : public ui::AnimationDelegate,
+      public base::SupportsWeakPtr<IconAnimationWrapper> {
  public:
-  IconAnimationWrapper(ExtensionAction* owner, int tab_id)
-      : owner_(owner),
-        tab_id_(tab_id),
-        ALLOW_THIS_IN_INITIALIZER_LIST(animation_(this)) {}
+  IconAnimationWrapper()
+      : ALLOW_THIS_IN_INITIALIZER_LIST(animation_(this)) {}
 
   virtual ~IconAnimationWrapper() {}
 
@@ -87,12 +87,9 @@ class ExtensionAction::IconAnimationWrapper : public ui::AnimationDelegate {
   }
 
   void Done() {
-    owner_->icon_animation_.erase(tab_id_);
-    // this will now have been deleted.
+    delete this;
   }
 
-  ExtensionAction* owner_;
-  int tab_id_;
   IconAnimation animation_;
 };
 
@@ -344,26 +341,47 @@ void ExtensionAction::PaintBadge(gfx::Canvas* canvas,
   canvas->Restore();
 }
 
-base::WeakPtr<ExtensionAction::IconAnimation> ExtensionAction::GetIconAnimation(
+ExtensionAction::IconAnimationWrapper* ExtensionAction::GetIconAnimationWrapper(
     int tab_id) const {
-  std::map<int, linked_ptr<IconAnimationWrapper> >::const_iterator it =
+  std::map<int, base::WeakPtr<IconAnimationWrapper> >::iterator it =
       icon_animation_.find(tab_id);
-  return (it != icon_animation_.end()) ? it->second->animation()->AsWeakPtr()
-                                       : base::WeakPtr<IconAnimation>();
+  if (it == icon_animation_.end())
+    return NULL;
+  if (it->second)
+    return it->second;
+
+  // Take this opportunity to remove all the NULL IconAnimationWrappers from
+  // icon_animation_.
+  icon_animation_.erase(it);
+  for (it = icon_animation_.begin(); it != icon_animation_.end();) {
+    if (it->second) {
+      ++it;
+    } else {
+      // The WeakPtr is null; remove it from the map.
+      icon_animation_.erase(it++);
+    }
+  }
+  return NULL;
+}
+
+base::WeakPtr<ExtensionAction::IconAnimation> ExtensionAction::GetIconAnimation(
+    int tab_id) const {
+  IconAnimationWrapper* wrapper = GetIconAnimationWrapper(tab_id);
+  return wrapper ? wrapper->animation()->AsWeakPtr()
+      : base::WeakPtr<IconAnimation>();
 }
 
 gfx::Image ExtensionAction::ApplyIconAnimation(int tab_id,
                                                const gfx::Image& orig) const {
-  std::map<int, linked_ptr<IconAnimationWrapper> >::const_iterator it =
-      icon_animation_.find(tab_id);
-  if (it == icon_animation_.end())
+  IconAnimationWrapper* wrapper = GetIconAnimationWrapper(tab_id);
+  if (wrapper == NULL)
     return orig;
-  return gfx::Image(it->second->animation()->Apply(*orig.ToSkBitmap()));
+  return gfx::Image(wrapper->animation()->Apply(*orig.ToSkBitmap()));
 }
 
 void ExtensionAction::RunIconAnimation(int tab_id) {
   IconAnimationWrapper* icon_animation =
-      new IconAnimationWrapper(this, tab_id);
-  icon_animation_[tab_id] = make_linked_ptr(icon_animation);
+      new IconAnimationWrapper();
+  icon_animation_[tab_id] = icon_animation->AsWeakPtr();
   icon_animation->animation()->Start();
 }

chrome/common/extensions/extension_action.h

@@ -235,6 +235,13 @@ class ExtensionAction {
   void RunIconAnimation(int tab_id);
 
  private:
+  class IconAnimationWrapper;
+
+  // Finds the icon animation wrapper for a tab, if any.  If the animation for
+  // this tab has recently completed, also removes up any other dead wrappers
+  // from the map.
+  IconAnimationWrapper* GetIconAnimationWrapper(int tab_id) const;
+
   // If the icon animation is running on tab |tab_id|, applies it to
   // |orig| and returns the result. Otherwise, just returns |orig|.
   gfx::Image ApplyIconAnimation(int tab_id, const gfx::Image& orig) const;
@@ -279,8 +286,13 @@ class ExtensionAction {
   std::map<int, SkColor> badge_text_color_;
   std::map<int, bool> visible_;
 
-  class IconAnimationWrapper;
-  std::map<int, linked_ptr<IconAnimationWrapper> > icon_animation_;
+  // IconAnimationWrappers own themselves so that even if the Extension and
+  // ExtensionAction are destroyed on a non-UI thread, the animation will still
+  // only be touched from the UI thread.  When an animation finishes, it deletes
+  // itself, which causes the WeakPtr in this map to become NULL.
+  // GetIconAnimationWrapper() removes NULLs to prevent the map from growing
+  // without bound.
+  mutable std::map<int, base::WeakPtr<IconAnimationWrapper> > icon_animation_;
 
   std::string default_icon_path_;
 

chrome/test/data/extensions/api_test/script_badge/basics/background.js

@@ -7,4 +7,22 @@ chrome.scriptBadge.onClicked.addListener(function(windowId) {
   chrome.test.notifyPass();
 });
 
+chrome.webNavigation.onCompleted.addListener(function(details) {
+    if (details.url.indexOf("http://") === 0) {
+      // Executing script causes an icon animation to run. Originally, if
+      // this animation was still running when the browser shut down, it
+      // would cause a crash due to the Extension being destroyed on the
+      // IO thread, and transitively causing a UI-only object to be
+      // destroyed there.
+      chrome.tabs.executeScript(
+          details.tabId,
+          {
+            // The setTimeout allows the ScriptBadgeController to
+            // reliably notice that a content script has run.
+            "code": ("window.setTimeout(function() {" +
+                     "  chrome.test.notifyPass();}, 1)")
+          });
+    }
+  });
+
 chrome.test.notifyPass();

chrome/test/data/extensions/api_test/script_badge/basics/manifest.json

@@ -6,7 +6,7 @@
     "scripts": ["background.js"]
   },
   "permissions": [
-    "tabs", "http://*/*"
+    "tabs", "webNavigation", "http://*/*"
   ],
   "script_badge": {
     "default_popup": "default_popup.html"