Fixed Issue 141873. Crash when BrowserActionButton get disabled or hidden.

chrome/browser/ui/views/browser_action_view.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_action_view.h"
 
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/extensions/api/commands/command_service.h"
 #include "chrome/browser/extensions/api/commands/command_service_factory.h"
 #include "chrome/browser/extensions/extension_context_menu_model.h"
@@ skipping 41 matching lines @@
   return true;
 }
 
 BrowserActionView::BrowserActionView(const Extension* extension,
                                      Browser* browser,
                                      BrowserActionView::Delegate* delegate)
     : browser_(browser),
       delegate_(delegate),
       button_(NULL),
       extension_(extension) {
+  button_ = new BrowserActionButton(extension_, browser_, delegate_);
+  button_->set_drag_controller(delegate_);
+  AddChildView(button_);
+  button_->UpdateState();
 }
 
 BrowserActionView::~BrowserActionView() {
+  RemoveChildView(button_);
+  button_->Destroy();

[sky, 2012/08/10 19:01:27]

Why does BrowserActionButton need a Destroy()? Can't it do the cleanup it needs
in ~BrowserActionButton?

[yefimt, 2012/08/10 19:27:05]

It was the original code before my previous CL, so I just reverting it back to
how it was.

 }
 
 gfx::Canvas* BrowserActionView::GetIconWithBadge() {
   int tab_id = delegate_->GetCurrentTabId();
 
   SkBitmap icon =
       *button_->extension()->browser_action()->GetIcon(tab_id).ToSkBitmap();
 
   // Dim the icon if our button is disabled.
   if (!button_->IsEnabled(tab_id))
@@ skipping 14 matching lines @@
   // BrowserActionsContainer::RefreshBrowserActionViews(), whereas this can be
   // called before that when the initial bounds are set (and then not after,
   // since the bounds don't change).  So instead of setting the height from the
   // button's preferred size, we use IconHeight(), since that's how big the
   // button should be regardless of what it's displaying.
   gfx::Point offset = delegate_->GetViewContentOffset();
   button_->SetBounds(offset.x(), offset.y(), width() - offset.x(),
                      BrowserActionsContainer::IconHeight());
 }
 
-void BrowserActionView::ViewHierarchyChanged(bool is_add,
-                                             View* parent,
-                                             View* child) {
-  if (is_add && (child == this)) {
-    button_ = new BrowserActionButton(extension_, browser_, delegate_);
-    button_->set_drag_controller(delegate_);
-
-    AddChildView(button_);
-    button_->UpdateState();
-  }
-}
-
 void BrowserActionView::GetAccessibleState(ui::AccessibleViewState* state) {
   state->name = l10n_util::GetStringUTF16(
       IDS_ACCNAME_EXTENSIONS_BROWSER_ACTION);
   state->role = ui::AccessibilityTypes::ROLE_GROUPING;
 }
 
 gfx::Size BrowserActionView::GetPreferredSize() {
   return gfx::Size(BrowserActionsContainer::IconWidth(false),
                    BrowserActionsContainer::IconHeight());
 }
@@ skipping 102 matching lines @@
   menu_runner_.reset(new views::MenuRunner(menu_model_adapter.CreateMenu()));
 
   context_menu_ = menu_runner_->GetMenu();
   gfx::Point screen_loc;
   views::View::ConvertPointToScreen(this, &screen_loc);
   if (menu_runner_->RunMenuAt(GetWidget(), NULL, gfx::Rect(screen_loc, size()),
           views::MenuItemView::TOPLEFT, views::MenuRunner::HAS_MNEMONICS) ==
           views::MenuRunner::MENU_DELETED) {
     return;
   }
+  // BrowserActionButton could be deleted by this point,
+  // i.e. if user selected Disable from the menu.
 
-  menu_runner_.reset();

[sky, 2012/08/10 19:01:27]

Why are you removing this?

[yefimt, 2012/08/10 19:27:05]

It is crashing here if BrowserActionButton is deleted by this point. I added
this line in my previous CL because it looked reasonable for me, but it causes
crash now.

   SetButtonNotPushed();
   context_menu_ = NULL;
 }
 
 void BrowserActionButton::OnImageLoaded(const gfx::Image& image,
                                         const std::string& extension_id,
                                         int index) {
   browser_action_->CacheIcon(browser_action_->default_icon_path(), image);
 
   // Call back to UpdateState() because a more specific icon might have been set
@@ skipping 200 matching lines @@
 
   extensions::Command browser_action_command;
   if (!only_if_active || !command_service->GetBrowserActionCommand(
           extension_->id(),
           extensions::CommandService::ACTIVE_ONLY,
           &browser_action_command,
           NULL)) {
     GetFocusManager()->UnregisterAccelerator(*keybinding_.get(), this);
   }
 }