Fix DCHECK in BrowserActionsContainer::OnWidgetClosing

chrome/browser/ui/views/browser_actions_container.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/browser/ui/views/browser_actions_container.h"
 
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
@@ skipping 795 matching lines @@
     BrowserActionOverflowMenuController* controller) {
   DCHECK(controller == overflow_menu_);
   overflow_menu_ = NULL;
 }
 
 void BrowserActionsContainer::InspectPopup(ExtensionAction* action) {
   OnBrowserActionExecuted(GetBrowserActionView(action)->button(), true);
 }
 
 void BrowserActionsContainer::OnWidgetClosing(views::Widget* widget) {
-  DCHECK_EQ(popup_->GetWidget(), widget);
-  popup_->GetWidget()->RemoveObserver(this);
+  // Always remove this as an observer of a closing popup.
+  widget->RemoveObserver(this);
+  // If a new popup has been opened before this gets called, |popup_| will

[sky, 2012/03/02 20:53:50]

Can't this go back to a DCHECK now?

[stevenjb, 2012/03/02 21:29:13]

Since closing a window is independent from calls to HidePopup(), it seems
possible that OnWidgetClosing() might get called for window A after HidePopup()
gets called for window B, in which case popup_ might be NULL. This code should
be safe regardless. I'm not certain that a DCHECK would be.

+  // point to the new popup, so just return.
+  if (!popup_ || popup_->GetWidget() != widget)
+    return;
   popup_ = NULL;
   // |popup_button_| is NULL if the extension has been removed.
   if (popup_button_) {
     popup_button_->SetButtonNotPushed();
     popup_button_ = NULL;
   }
 }
 
 void BrowserActionsContainer::MoveBrowserAction(const std::string& extension_id,
                                                 size_t new_index) {
   ExtensionService* service = profile_->GetExtensionService();
   if (service) {
     const Extension* extension = service->GetExtensionById(extension_id, false);
     model_->MoveBrowserAction(extension, new_index);
     SchedulePaint();
   }
 }
 
 void BrowserActionsContainer::HidePopup() {
+  // Remove this as an observer and clear |popup_| and |popup_button_| here,
+  // since we might change them before OnWidgetClosing() gets called.
   if (popup_) {
+    popup_->GetWidget()->RemoveObserver(this);
     popup_->GetWidget()->Close();
-    // NULL out popup_button_ in case it's being deleted.
+    popup_ = NULL;
+  }
+  if (popup_button_) {
+    popup_button_->SetButtonNotPushed();
     popup_button_ = NULL;
   }
 }
 
 void BrowserActionsContainer::TestExecuteBrowserAction(int index) {
   BrowserActionButton* button = browser_action_views_[index]->button();
   OnBrowserActionExecuted(button, false);
 }
 
 void BrowserActionsContainer::TestSetIconVisibilityCount(size_t icons) {
@@ skipping 270 matching lines @@
   }
 }
 
 bool BrowserActionsContainer::ShouldDisplayBrowserAction(
     const Extension* extension) {
   // Only display incognito-enabled extensions while in incognito mode.
   return
       (!profile_->IsOffTheRecord() ||
        profile_->GetExtensionService()->IsIncognitoEnabled(extension->id()));
 }