Fix a crash when activating a select element inside a page

action popup.

With this change, select elements still don't work correctly
with page actions: when you try to use them, the page action
popup disappears. However, at least now, it doesn't crash.

BUG=27576
TEST=Install extension in related bug. Navigate to any site
and click page action. Browser should not crash.

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

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

3 files changed, 69 insertions, 12 deletions

diff --git a/chrome/browser/views/browser_bubble.h b/chrome/browser/views/browser_bubble.h
index 74928f3..eaf3d0b 100644
--- a/chrome/browser/views/browser_bubble.h
+++ b/chrome/browser/views/browser_bubble.h
@@ -19,11 +19,11 @@ class BrowserBubble {
   class Delegate {
    public:
     // Called when the Browser Window that this bubble is attached to moves.
-    virtual void BubbleBrowserWindowMoved(BrowserBubble* bubble) = 0;
+    virtual void BubbleBrowserWindowMoved(BrowserBubble* bubble) {}
 
     // Called with the Browser Window that this bubble is attached to is
     // about to close.
-    virtual void BubbleBrowserWindowClosing(BrowserBubble* bubble) = 0;
+    virtual void BubbleBrowserWindowClosing(BrowserBubble* bubble) {}
 
     // Called when the bubble became active / got focus.
     virtual void BubbleGotFocus(BrowserBubble* bubble) {}
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index f0931e4..a0033b6 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -1294,7 +1294,9 @@ LocationBarView::PageActionImageView::PageActionImageView(
       profile_(profile),
       page_action_(page_action),
       current_tab_id_(-1),
-      preview_enabled_(false) {
+      preview_enabled_(false),
+      popup_(NULL),
+      ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   Extension* extension = profile->GetExtensionsService()->GetExtensionById(
       page_action->extension_id(), false);
   DCHECK(extension);
@@ -1318,16 +1320,13 @@ LocationBarView::PageActionImageView::PageActionImageView(
 LocationBarView::PageActionImageView::~PageActionImageView() {
   if (tracker_)
     tracker_->StopTrackingImageLoad();
+
+  if (popup_)
+    HidePopup();
 }
 
 void LocationBarView::PageActionImageView::ExecuteAction(int button) {
   if (page_action_->has_popup()) {
-    gfx::Point origin;
-    View::ConvertPointToScreen(this, &origin);
-    gfx::Rect rect = bounds();
-    rect.set_x(origin.x());
-    rect.set_y(origin.y());
-
     // In tests, GetLastActive could return NULL, so we need to have
     // a fallback.
     // TODO(erikkay): Find a better way to get the Browser that this
@@ -1335,8 +1334,18 @@ void LocationBarView::PageActionImageView::ExecuteAction(int button) {
     Browser* browser = BrowserList::GetLastActiveWithProfile(profile_);
     if (!browser)
       browser = BrowserList::FindBrowserWithProfile(profile_);
-    ExtensionPopup::Show(page_action_->popup_url(), browser, rect,
-                         BubbleBorder::TOP_RIGHT);
+
+    // Always hide the current popup. Only one popup at a time.
+    HidePopup();
+
+    gfx::Point origin;
+    View::ConvertPointToScreen(this, &origin);
+    gfx::Rect rect = bounds();
+    rect.set_x(origin.x());
+    rect.set_y(origin.y());
+    popup_ = ExtensionPopup::Show(page_action_->popup_url(), browser, rect,
+                                  BubbleBorder::TOP_RIGHT);
+    popup_->set_delegate(this);
   } else {
     ExtensionBrowserEventRouter::GetInstance()->PageActionExecuted(
         profile_, page_action_->extension_id(), page_action_->id(),
@@ -1440,6 +1449,38 @@ void LocationBarView::PageActionImageView::UpdateVisibility(
   SetVisible(visible);
 }
 
+void LocationBarView::PageActionImageView::BubbleBrowserWindowClosing(
+    BrowserBubble* bubble) {
+  HidePopup();
+}
+
+void LocationBarView::PageActionImageView::BubbleLostFocus(
+    BrowserBubble* bubble) {
+  if (!popup_)
+    return;
+
+  MessageLoop::current()->PostTask(FROM_HERE,
+      method_factory_.NewRunnableMethod(
+          &LocationBarView::PageActionImageView::HidePopup));
+}
+
+void LocationBarView::PageActionImageView::HidePopup() {
+  if (!popup_)
+    return;
+
+  // This sometimes gets called via a timer (See BubbleLostFocus), so clear
+  // the method factory. in case one is pending.
+  method_factory_.RevokeAll();
+
+  // Save the popup in a local since destroying it calls BubbleLostFocus,
+  // which will try to call HidePopup() again.
+  ExtensionPopup* closing_popup = popup_;
+  popup_ = NULL;
+
+  closing_popup->DetachFromBrowser();
+  delete closing_popup;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // LocationBarView, LocationBar implementation:
 
diff --git a/chrome/browser/views/location_bar_view.h b/chrome/browser/views/location_bar_view.h
index 3a57e26..91ca81b 100644
--- a/chrome/browser/views/location_bar_view.h
+++ b/chrome/browser/views/location_bar_view.h
@@ -11,11 +11,13 @@
 
 #include "app/gfx/font.h"
 #include "base/gfx/rect.h"
+#include "base/task.h"
 #include "chrome/browser/autocomplete/autocomplete_edit.h"
 #include "chrome/browser/extensions/image_loading_tracker.h"
 #include "chrome/browser/location_bar.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/toolbar_model.h"
+#include "chrome/browser/views/browser_bubble.h"
 #include "chrome/browser/views/info_bubble.h"
 #include "views/controls/image_view.h"
 #include "views/controls/label.h"
@@ -31,6 +33,7 @@
 class BubblePositioner;
 class CommandUpdater;
 class ExtensionAction;
+class ExtensionPopup;
 class GURL;
 class Profile;
 
@@ -359,7 +362,8 @@ class LocationBarView : public LocationBar,
   // PageActionImageView is used to display the icon for a given PageAction
   // and notify the extension when the icon is clicked.
   class PageActionImageView : public LocationBarImageView,
-                              public ImageLoadingTracker::Observer {
+                              public ImageLoadingTracker::Observer,
+                              public BrowserBubble::Delegate {
    public:
     PageActionImageView(LocationBarView* owner,
                         Profile* profile,
@@ -385,6 +389,10 @@ class LocationBarView : public LocationBar,
     // Overridden from ImageLoadingTracker.
     virtual void OnImageLoaded(SkBitmap* image, size_t index);
 
+    // Overridden from BrowserBubble::Delegate
+    virtual void BubbleBrowserWindowClosing(BrowserBubble* bubble);
+    virtual void BubbleLostFocus(BrowserBubble* bubble);
+
     // Called to notify the PageAction that it should determine whether to be
     // visible or hidden. |contents| is the TabContents that is active, |url|
     // is the current page URL.
@@ -394,6 +402,9 @@ class LocationBarView : public LocationBar,
     void ExecuteAction(int button);
 
    private:
+    // Hides the active popup, if there is one.
+    void HidePopup();
+
     // The location bar view that owns us.
     LocationBarView* owner_;
 
@@ -425,6 +436,11 @@ class LocationBarView : public LocationBar,
     // is briefly shown even if it hasn't been enabled by it's extension.
     bool preview_enabled_;
 
+    // The current popup and the button it came from.  NULL if no popup.
+    ExtensionPopup* popup_;
+
+    ScopedRunnableMethodFactory<PageActionImageView> method_factory_;
+
     DISALLOW_COPY_AND_ASSIGN(PageActionImageView);
   };
   friend class PageActionImageView;