Removal of an incorrect assert during the close sequence of ExtensionPopup views. If the popup view is closing due to a change of focus event, then it is possible for an extra reference to be held onto the popup during ExtensionPopup::Close.

Bug=None
Test=ExtensionApiTest.Popup

Review URL: http://codereview.chromium.org/2852002

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

2 files changed, 36 insertions, 10 deletions

diff --git a/chrome/browser/views/extensions/extension_popup.cc b/chrome/browser/views/extensions/extension_popup.cc
index 95ac21e..4ad6acc 100644
--- a/chrome/browser/views/extensions/extension_popup.cc
+++ b/chrome/browser/views/extensions/extension_popup.cc
@@ -76,7 +76,8 @@ ExtensionPopup::ExtensionPopup(ExtensionHost* host,
       border_view_(NULL),
       popup_chrome_(chrome),
       observer_(observer),
-      anchor_position_(arrow_location) {
+      anchor_position_(arrow_location),
+      instance_lifetime_(new InternalRefCounter()){
   AddRef();  // Balanced in Close();
   set_delegate(this);
   host->view()->SetContainer(this);
@@ -395,14 +396,21 @@ void ExtensionPopup::Close() {
   closing_ = true;
   DetachFromBrowser();
 
-  ExtensionPopup::Observer* observer = observer_;
-
-  if (observer)
-    observer->ExtensionPopupIsClosing(this);
+  if (observer_)
+    observer_->ExtensionPopupIsClosing(this);
 
-  DCHECK(HasOneRef()) << "Unexpected extra reference to ExtensionPopup.";
   Release();  // Balanced in ctor.
+}
 
-  if (observer)
-    observer->ExtensionPopupClosed(this);
+void ExtensionPopup::Release() {
+  bool final_release = instance_lifetime_->HasOneRef();
+  instance_lifetime_->Release();
+  if (final_release) {
+    DCHECK(closing_) << "ExtensionPopup to be destroyed before being closed.";
+    ExtensionPopup::Observer* observer = observer_;
+    delete this;
+
+    if (observer)
+      observer->ExtensionPopupClosed(this);
+  }
 }
diff --git a/chrome/browser/views/extensions/extension_popup.h b/chrome/browser/views/extensions/extension_popup.h
index ae8e68c..2139971 100644
--- a/chrome/browser/views/extensions/extension_popup.h
+++ b/chrome/browser/views/extensions/extension_popup.h
@@ -26,8 +26,7 @@ class Widget;
 class ExtensionPopup : public BrowserBubble,
                        public BrowserBubble::Delegate,
                        public NotificationObserver,
-                       public ExtensionView::Container,
-                       public base::RefCounted<ExtensionPopup> {
+                       public ExtensionView::Container {
  public:
   // Observer to ExtensionPopup events.
   class Observer {
@@ -143,6 +142,18 @@ class ExtensionPopup : public BrowserBubble,
   virtual void OnExtensionMouseLeave(ExtensionView* view) { }
   virtual void OnExtensionPreferredSizeChanged(ExtensionView* view);
 
+  // Export the refrence-counted interface required for use as template
+  // arguments for RefCounted.  ExtensionPopup does not inherit from RefCounted
+  // because it must override the behaviour of Release.
+  void AddRef() { instance_lifetime_->AddRef(); }
+  static bool ImplementsThreadSafeReferenceCounting() {
+    return InternalRefCounter::ImplementsThreadSafeReferenceCounting();
+  }
+
+  // Implements the standard RefCounted<T>::Release behaviour, except
+  // signals Observer::ExtensionPopupClosed after final release.
+  void Release();
+
   // The min/max height of popups.
   static const int kMinWidth;
   static const int kMinHeight;
@@ -199,6 +210,13 @@ class ExtensionPopup : public BrowserBubble,
   // the position of the pop-up in relation to |relative_to_|.
   BubbleBorder::ArrowLocation anchor_position_;
 
+  // ExtensionPopup's lifetime is managed via reference counting, but it does
+  // not expose the RefCounted interface.  Instead, the lifetime is tied to
+  // this member variable.
+  class InternalRefCounter : public base::RefCounted<InternalRefCounter> {
+  };
+  InternalRefCounter* instance_lifetime_;
+
   DISALLOW_COPY_AND_ASSIGN(ExtensionPopup);
 };