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

chrome/common/extensions/extension_action.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/common/extensions/extension_action.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "chrome/common/badge_util.h"
@@ skipping 43 matching lines @@
 
 
 int Width(const gfx::Image& image) {
   if (image.IsEmpty())
     return 0;
   return image.ToSkBitmap()->width();
 }
 
 }  // 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() {}
 
   IconAnimation* animation() {
     return &animation_;
   }
 
  private:
   virtual void AnimationEnded(const ui::Animation* animation) OVERRIDE {
     Done();
   }
 
   virtual void AnimationCanceled(const ui::Animation* animation) OVERRIDE {
     Done();
   }
 
   void Done() {
-    owner_->icon_animation_.erase(tab_id_);
-    // this will now have been deleted.
+    delete this;
   }
 
-  ExtensionAction* owner_;
-  int tab_id_;
   IconAnimation animation_;
 };
 
 const int ExtensionAction::kDefaultTabId = -1;
 
 ExtensionAction::IconAnimation::IconAnimation(
     ui::AnimationDelegate* delegate)
     // 100ms animation at 50fps (so 5 animation frames in total).
     : ui::LinearAnimation(100, 50, delegate) {}
 
@@ skipping 231 matching lines @@
   rect.fLeft += kPadding;
   rect.fRight -= kPadding;
   canvas->sk_canvas()->clipRect(rect);
   canvas->sk_canvas()->drawText(text.c_str(), text.size(),
                                 rect.fLeft + (rect.width() - text_width) / 2,
                                 rect.fTop + kTextSize + kTopTextPadding,
                                 *text_paint);
   canvas->Restore();
 }
 
+ExtensionAction::IconAnimationWrapper* ExtensionAction::GetIconAnimationWrapper(
+    int tab_id) const {
+  std::map<int, base::WeakPtr<IconAnimationWrapper> >::iterator it =
+      icon_animation_.find(tab_id);
+  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 (std::map<int, base::WeakPtr<IconAnimationWrapper> >::iterator it =
+           icon_animation_.begin(); it != icon_animation_.end();) {

[not at google - send to devlin, 2012/08/02 14:44:25]

nits:

you could re-use the iterator? saves some typing, if a bit strange.

In any case, I'd typedef std::map<int, base::WeakPtr<IconAnimationWrapper> > to
something.

[Jeffrey Yasskin, 2012/08/02 15:11:49]

Sure.
With the re-used iterator, this map type only appears twice, so I don't think
it's worth obscuring with a typedef.

+    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 {
-  std::map<int, linked_ptr<IconAnimationWrapper> >::const_iterator it =
-      icon_animation_.find(tab_id);
-  return (it != icon_animation_.end()) ? it->second->animation()->AsWeakPtr()
-                                       : base::WeakPtr<IconAnimation>();
+  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();
 }