[Mac] Make the extension popup a singleton instance to avoid maintaining multiple pointer references to a class that, by design, can only have one popup open at a time.

The bug fixed in this change has to do with PageActionView having a dirty pointer to a popup if it was closed via losing its key state. Once a notification like EXTENSION_HOST_VIEW_SHOULD_CLOSE was fired, then the PageActionView would assume the pointer was valid and call on dirty memory.

TEST=none
BUG=29492,33590
Review URL: http://codereview.chromium.org/561013

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

10 files changed, 69 insertions, 92 deletions

diff --git a/chrome/browser/cocoa/extensions/browser_actions_controller.h b/chrome/browser/cocoa/extensions/browser_actions_controller.h
index 1d38da6..8daf35e 100644
--- a/chrome/browser/cocoa/extensions/browser_actions_controller.h
+++ b/chrome/browser/cocoa/extensions/browser_actions_controller.h
@@ -42,9 +42,6 @@ extern NSString* const kBrowserActionsChangedNotification;
 
   // The order of the BrowserActionButton objects within the dictionary.
   scoped_nsobject<NSMutableArray> buttonOrder_;
-
-  // The controller for the popup displayed if a browser action has one. Weak.
-  ExtensionPopupController* popupController_;
 }
 
 // Initializes the controller given the current browser and container view that
@@ -59,10 +56,6 @@ extern NSString* const kBrowserActionsChangedNotification;
 // Update the display of all buttons.
 - (void)update;
 
-// Returns the controller used to display the popup being shown. If no popup is
-// currently open, then nil is returned.
-- (ExtensionPopupController*)popup;
-
 // Marks the container view for redraw. Called by the extension service
 // notification bridge.
 - (void)browserActionVisibilityHasChanged;
diff --git a/chrome/browser/cocoa/extensions/browser_actions_controller.mm b/chrome/browser/cocoa/extensions/browser_actions_controller.mm
index 3d5cad9..e28d69f 100644
--- a/chrome/browser/cocoa/extensions/browser_actions_controller.mm
+++ b/chrome/browser/cocoa/extensions/browser_actions_controller.mm
@@ -66,9 +66,9 @@ class ExtensionsServiceObserverBridge : public NotificationObserver {
         break;
       }
       case NotificationType::EXTENSION_HOST_VIEW_SHOULD_CLOSE: {
-        ExtensionPopupController* popup = [owner_ popup];
-        if (popup && Details<ExtensionHost>([popup host]) == details)
-          [[owner_ popup] close];
+        ExtensionPopupController* popup = [ExtensionPopupController popup];
+        if (popup && ![popup isClosing])
+          [popup close];
 
         break;
       }
@@ -119,10 +119,6 @@ class ExtensionsServiceObserverBridge : public NotificationObserver {
   }
 }
 
-- (ExtensionPopupController*)popup {
-  return popupController_;
-}
-
 - (void)browserActionVisibilityHasChanged {
   [containerView_ setNeedsDisplay:YES];
 }
@@ -227,7 +223,7 @@ class ExtensionsServiceObserverBridge : public NotificationObserver {
   }
 
   ExtensionAction* action = [sender extension]->browser_action();
-  if (action->HasPopup(tabId) && !popupController_) {
+  if (action->HasPopup(tabId)) {
     NSString* extensionId = base::SysUTF8ToNSString([sender extension]->id());
     // If the extension ID is not valid UTF-8, then the NSString will be nil
     // and an exception will be thrown when calling objectForKey below, hosing
@@ -245,33 +241,16 @@ class ExtensionsServiceObserverBridge : public NotificationObserver {
     // Adjust the anchor point to be at the center of the browser action button.
     arrowPoint.x += kBrowserActionWidth / 2;
 
-    popupController_ = [ExtensionPopupController
-            showURL:action->GetPopupUrl(tabId)
-          inBrowser:browser_
-         anchoredAt:arrowPoint
-      arrowLocation:kTopRight];
-
-    [[NSNotificationCenter defaultCenter]
-        addObserver:self
-           selector:@selector(popupWillClose:)
-               name:NSWindowWillCloseNotification
-             object:[popupController_ window]];
+    [ExtensionPopupController showURL:action->GetPopupUrl(tabId)
+                            inBrowser:browser_
+                           anchoredAt:arrowPoint
+                        arrowLocation:kTopRight];
   } else {
     ExtensionBrowserEventRouter::GetInstance()->BrowserActionExecuted(
        profile_, action->extension_id(), browser_);
   }
 }
 
-// Nil out the weak popup controller reference.
-- (void)popupWillClose:(NSNotification*)notification {
-  DCHECK([notification object] == [popupController_ window]);
-  [[NSNotificationCenter defaultCenter]
-      removeObserver:self
-                name:NSWindowWillCloseNotification
-              object:[popupController_ window]];
-  popupController_ = nil;
-}
-
 - (int)currentTabId {
   TabContents* selected_tab = browser_->GetSelectedTabContents();
   if (!selected_tab)
diff --git a/chrome/browser/cocoa/extensions/extension_popup_controller.h b/chrome/browser/cocoa/extensions/extension_popup_controller.h
index 8085918..1c37c7a 100644
--- a/chrome/browser/cocoa/extensions/extension_popup_controller.h
+++ b/chrome/browser/cocoa/extensions/extension_popup_controller.h
@@ -21,7 +21,9 @@ class ExtensionHost;
 // user has clicked on a browser action button. It instantiates the extension
 // popup view showing the content and resizes the window to accomodate any size
 // changes as they occur.
-// There can only be one browser action popup open at a time.
+//
+// There can only be one browser action popup open at a time, so a static
+// variable holds a reference to the current popup.
 @interface ExtensionPopupController : NSWindowController<NSWindowDelegate> {
  @private
   // The native extension view retrieved from the extension host. Weak.
@@ -42,10 +44,6 @@ class ExtensionHost;
   scoped_ptr<ExtensionHost> host_;
 }
 
-// Returns a pointer to the extension host object associated with this
-// browser action.
-- (ExtensionHost*)host;
-
 // Starts the process of showing the given popup URL. Instantiates an
 // ExtensionPopupController with the parent window retrieved from |browser|, a
 // host for the popup created by the extension process manager specific to the
@@ -58,6 +56,14 @@ class ExtensionHost;
                            inBrowser:(Browser*)browser
                           anchoredAt:(NSPoint)anchoredAt
                        arrowLocation:(BubbleArrowLocation)arrowLocation;
+
+// Returns the controller used to display the popup being shown. If no popup is
+// currently open, then nil is returned. Static because only one extension popup
+// window can be open at a time.
++ (ExtensionPopupController*)popup;
+
+// Whether the popup is in the process of closing (via Core Animation).
+- (BOOL)isClosing;
 @end
 
 @interface ExtensionPopupController(TestingAPI)
diff --git a/chrome/browser/cocoa/extensions/extension_popup_controller.mm b/chrome/browser/cocoa/extensions/extension_popup_controller.mm
index 662f93f..4606a92 100644
--- a/chrome/browser/cocoa/extensions/extension_popup_controller.mm
+++ b/chrome/browser/cocoa/extensions/extension_popup_controller.mm
@@ -27,6 +27,9 @@ const CGFloat kMaxHeight = 600;
 // The duration for any animations that might be invoked by this controller.
 const NSTimeInterval kAnimationDuration = 0.2;
 
+// There should only be one extension popup showing at one time. Keep a
+// reference to it here.
+static ExtensionPopupController* gPopup;
 }  // namespace
 
 @interface ExtensionPopupController(Private)
@@ -100,19 +103,16 @@ const NSTimeInterval kAnimationDuration = 0.2;
 
 - (void)windowWillClose:(NSNotification *)notification {
   [[NSNotificationCenter defaultCenter] removeObserver:self];
-  [self autorelease];
-}
-
-- (ExtensionHost*)host {
-  return host_.get();
+  [gPopup autorelease];
+  gPopup = nil;
 }
 
 - (void)windowDidResignKey:(NSNotification *)notification {
   NSWindow* window = [self window];
   DCHECK_EQ([notification object], window);
+  // If the window isn't visible, it is already closed, and this notification
+  // has been sent as part of the closing operation, so no need to close.
   if ([window isVisible]) {
-    // If the window isn't visible, it is already closed, and this notification
-    // has been sent as part of the closing operation, so no need to close.
     [self close];
   }
 }
@@ -122,10 +122,15 @@ const NSTimeInterval kAnimationDuration = 0.2;
   [super close];
 }
 
+- (BOOL)isClosing {
+  return [static_cast<InfoBubbleWindow*>([self window]) isClosing];
+}
+
 + (ExtensionPopupController*)showURL:(GURL)url
                            inBrowser:(Browser*)browser
                           anchoredAt:(NSPoint)anchoredAt
                        arrowLocation:(BubbleArrowLocation)arrowLocation {
+  DCHECK([NSThread isMainThread]);
   DCHECK(browser);
   if (!browser)
     return nil;
@@ -141,15 +146,29 @@ const NSTimeInterval kAnimationDuration = 0.2;
   if (!host)
     return nil;
 
+  // Make absolutely sure that no popups are leaked.
+  if (gPopup) {
+    if ([[gPopup window] isVisible])
+      [gPopup close];
+
+    [gPopup autorelease];
+    gPopup = nil;
+  }
+  DCHECK(!gPopup);
+
   // Takes ownership of |host|. Also will autorelease itself when the popup is
   // closed, so no need to do that here.
-  ExtensionPopupController* popup = [[ExtensionPopupController alloc]
+  gPopup = [[ExtensionPopupController alloc]
       initWithHost:host
       parentWindow:browser->window()->GetNativeHandle()
         anchoredAt:anchoredAt
      arrowLocation:arrowLocation];
 
-  return popup;
+  return gPopup;
+}
+
++ (ExtensionPopupController*)popup {
+  return gPopup;
 }
 
 - (void)extensionViewFrameChanged {
diff --git a/chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm b/chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm
index 08fbbaa..16f2fcf 100644
--- a/chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm
+++ b/chrome/browser/cocoa/extensions/extension_popup_controller_unittest.mm
@@ -63,19 +63,18 @@ class ExtensionPopupControllerTest : public CocoaTest {
     profile_.reset(new ExtensionTestingProfile());
     profile_->InitExtensionProfile();
     browser_.reset(new Browser(Browser::TYPE_NORMAL, profile_.get()));
-    popup_controller_.reset(
-        [ExtensionPopupController showURL:GURL("http://google.com")
-                                inBrowser:browser_.get()
-                               anchoredAt:NSZeroPoint
-                            arrowLocation:kTopRight]);
+    [ExtensionPopupController showURL:GURL("http://google.com")
+                            inBrowser:browser_.get()
+                           anchoredAt:NSZeroPoint
+                        arrowLocation:kTopRight];
   }
   virtual void TearDown() {
     profile_->ShutdownExtensionProfile();
+    [[ExtensionPopupController popup] close];
     CocoaTest::TearDown();
   }
 
  protected:
-  scoped_nsobject<ExtensionPopupController> popup_controller_;
   scoped_ptr<Browser> browser_;
   scoped_ptr<ExtensionTestingProfile> profile_;
 };
@@ -84,7 +83,7 @@ TEST_F(ExtensionPopupControllerTest, DISABLED_Basics) {
   // TODO(andybons): Better mechanisms for mocking out the extensions service
   // and extensions for easy testing need to be implemented.
   // http://crbug.com/28316
-  EXPECT_TRUE(popup_controller_.get());
+  EXPECT_TRUE([ExtensionPopupController popup]);
 }
 
 }  // namespace
diff --git a/chrome/browser/cocoa/info_bubble_window.h b/chrome/browser/cocoa/info_bubble_window.h
index ab23b10..b0d05c5 100644
--- a/chrome/browser/cocoa/info_bubble_window.h
+++ b/chrome/browser/cocoa/info_bubble_window.h
@@ -15,16 +15,11 @@
   BOOL delayOnClose_;
 }
 
-@property BOOL delayOnClose;
-
-@end
-
-// Methods to only be used by unittests.
-@interface InfoBubbleWindow (UnitTest)
-
 // Returns YES if the window is in the process of closing.
 // Can't use "windowWillClose" notification because that will be sent
 // after the closing animation has completed.
 - (BOOL)isClosing;
 
+@property BOOL delayOnClose;
+
 @end
diff --git a/chrome/browser/cocoa/info_bubble_window.mm b/chrome/browser/cocoa/info_bubble_window.mm
index 84ad0b4..80c0b8d 100644
--- a/chrome/browser/cocoa/info_bubble_window.mm
+++ b/chrome/browser/cocoa/info_bubble_window.mm
@@ -115,9 +115,8 @@ const NSTimeInterval kOrderOutAnimationDuration = 0.15;
 // Called by InfoBubbleWindowCloser when the window is to be really closed
 // after the fading animation is complete.
 - (void)finishCloseAfterAnimation {
-  if (closing_) {
+  if (closing_)
     [super close];
-  }
 }
 
 // Adds animation for info bubbles being ordered to the front.
@@ -151,9 +150,8 @@ const NSTimeInterval kOrderOutAnimationDuration = 0.15;
 // If the window is currently animating a close, block all UI events to the
 // window.
 - (void)sendEvent:(NSEvent*)theEvent {
-  if (!closing_) {
+  if (!closing_)
     [super sendEvent:theEvent];
-  }
 }
 
 - (BOOL)isClosing {
diff --git a/chrome/browser/cocoa/location_bar_view_mac.h b/chrome/browser/cocoa/location_bar_view_mac.h
index be784b5..363188c 100644
--- a/chrome/browser/cocoa/location_bar_view_mac.h
+++ b/chrome/browser/cocoa/location_bar_view_mac.h
@@ -237,7 +237,6 @@ class LocationBarViewMac : public AutocompleteEditController,
     PageActionImageView() : owner_(NULL),
                             profile_(NULL),
                             page_action_(NULL),
-                            popup_controller_(nil),
                             tracker_(NULL),
                             current_tab_id_(-1),
                             preview_enabled_(false) {}
@@ -247,7 +246,6 @@ class LocationBarViewMac : public AutocompleteEditController,
     virtual void Observe(NotificationType type,
                          const NotificationSource& source,
                          const NotificationDetails& details);
-    void HidePopup();
 
     // The location bar view that owns us.
     LocationBarViewMac* owner_;
@@ -263,9 +261,6 @@ class LocationBarViewMac : public AutocompleteEditController,
     typedef std::map<std::string, SkBitmap> PageActionMap;
     PageActionMap page_action_icons_;
 
-    // The controller for the popup displayed if a page action has one. Weak.
-    ExtensionPopupController* popup_controller_;
-
     // The object that is waiting for the image loading to complete
     // asynchronously.
     ImageLoadingTracker* tracker_;
diff --git a/chrome/browser/cocoa/location_bar_view_mac.mm b/chrome/browser/cocoa/location_bar_view_mac.mm
index ec592cb..2547c27 100644
--- a/chrome/browser/cocoa/location_bar_view_mac.mm
+++ b/chrome/browser/cocoa/location_bar_view_mac.mm
@@ -562,7 +562,6 @@ LocationBarViewMac::PageActionImageView::PageActionImageView(
     : owner_(owner),
       profile_(profile),
       page_action_(page_action),
-      popup_controller_(nil),
       current_tab_id_(-1),
       preview_enabled_(false) {
   Extension* extension = profile->GetExtensionsService()->GetExtensionById(
@@ -627,11 +626,10 @@ bool LocationBarViewMac::PageActionImageView::OnMousePressed(NSRect bounds) {
     // Adjust the anchor point to be at the center of the page action icon.
     arrowPoint.x += [GetImage() size].width / 2;
 
-    popup_controller_ = [ExtensionPopupController
-            showURL:page_action_->GetPopupUrl(current_tab_id_)
-          inBrowser:BrowserList::GetLastActive()
-         anchoredAt:arrowPoint
-      arrowLocation:kTopRight];
+    [ExtensionPopupController showURL:page_action_->GetPopupUrl(current_tab_id_)
+                            inBrowser:BrowserList::GetLastActive()
+                           anchoredAt:arrowPoint
+                        arrowLocation:kTopRight];
   } else {
     ExtensionBrowserEventRouter::GetInstance()->PageActionExecuted(
         profile_, page_action_->extension_id(), page_action_->id(),
@@ -738,23 +736,19 @@ void LocationBarViewMac::PageActionImageView::Observe(
     const NotificationSource& source,
     const NotificationDetails& details) {
   switch (type.value) {
-    case NotificationType::EXTENSION_HOST_VIEW_SHOULD_CLOSE:
-      if (popup_controller_ &&
-          Details<ExtensionHost>([popup_controller_ host]) == details) {
-        HidePopup();
-      }
+    case NotificationType::EXTENSION_HOST_VIEW_SHOULD_CLOSE: {
+      ExtensionPopupController* popup = [ExtensionPopupController popup];
+      if (popup && ![popup isClosing])
+        [popup close];
+
       break;
+    }
     default:
       NOTREACHED() << "Unexpected notification";
       break;
   }
 }
 
-void LocationBarViewMac::PageActionImageView::HidePopup() {
-  [popup_controller_ close];
-  popup_controller_ = nil;
-}
-
 // PageActionViewList-----------------------------------------------------------
 
 void LocationBarViewMac::PageActionViewList::DeleteAll() {
diff --git a/chrome/browser/cocoa/toolbar_controller.mm b/chrome/browser/cocoa/toolbar_controller.mm
index 59b3c47..6ffa93e 100644
--- a/chrome/browser/cocoa/toolbar_controller.mm
+++ b/chrome/browser/cocoa/toolbar_controller.mm
@@ -618,8 +618,7 @@ class PrefObserverBridge : public NotificationObserver {
   // subtract the threshold width, which represents the space that the
   // (optional) home button, omnibar, go button and page/wrench buttons take up
   // when no Browser Actions are displayed. This is to prevent the Browser
-  // Action buttons from pushing the elements to the left of them too much to
-  // the left.
+  // Action buttons from pushing the elements to the left of them too far over.
   CGFloat availableWidth = std::max(0.0f,
       curWidth - reloadFrame.origin.x + NSWidth(reloadFrame) -
           kHideBrowserActionThresholdWidth);
@@ -636,7 +635,7 @@ class PrefObserverBridge : public NotificationObserver {
     delta *= -1;
   } else if (visibleCount == buttonCount) {
     // The number of available slots is greater than the number of displayed
-    // buttons and all buttons are already displayed.
+    // buttons then all buttons are already displayed.
     return;
   }
   int arrayOffset = hide ? -1 : 0;