Skip to content

Commit

Permalink
DCHECK while invoking extension infobar context menu for the second time
Browse files Browse the repository at this point in the history
DCHECK failure occurs while trying to destroy the 
ExtensionContextMenuModel object in InfoBarGtk without calling 
RefCounted::Release().

Since ExtensionContextMenuModel is a refcounted class, its object needs
to be managed correctly with RefCounted::AddRef() & RefCounted::Release().
Moving the ownership of Menu Model to the sub classes of InfoBarGtk
to achieve this.

BUG=172921
TEST= Context menu opens for 'Sandwich' infobar sample extension without crash for the second time.

Review URL: https://chromiumcodereview.appspot.com/12092047

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@180898 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
MHX348@motorola.com authored and MHX348@motorola.com committed Feb 6, 2013
1 parent c49bcb6 commit e086357
Show file tree
Hide file tree
Showing 6 changed files with 16 additions and 12 deletions.
8 changes: 4 additions & 4 deletions chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ Browser* ExtensionInfoBarGtk::GetBrowser() {
return BrowserWindowGtk::GetBrowserWindowForNativeWindow(parent)->browser();
}

ui::MenuModel* ExtensionInfoBarGtk::BuildMenuModel() {
ExtensionContextMenuModel* ExtensionInfoBarGtk::BuildMenuModel() {
const extensions::Extension* extension = delegate_->extension();
if (!extension->ShowConfigureContextMenus())
return NULL;
Expand All @@ -182,13 +182,13 @@ gboolean ExtensionInfoBarGtk::OnButtonPress(GtkWidget* widget,
if (event->button != 1)
return FALSE;

ui::MenuModel* model = BuildMenuModel();
if (!model)
context_menu_model_ = BuildMenuModel();
if (!context_menu_model_)
return FALSE;

gtk_chrome_button_set_paint_state(GTK_CHROME_BUTTON(widget),
GTK_STATE_ACTIVE);
ShowMenuWithModel(widget, this, model);
ShowMenuWithModel(widget, this, context_menu_model_);

return TRUE;
}
Expand Down
6 changes: 5 additions & 1 deletion chrome/browser/ui/gtk/infobars/extension_infobar_gtk.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "chrome/browser/ui/gtk/menu_gtk.h"
#include "ui/gfx/gtk_util.h"

class ExtensionContextMenuModel;
class ExtensionResource;
class ExtensionViewGtk;
class MenuGtk;
Expand Down Expand Up @@ -47,7 +48,7 @@ class ExtensionInfoBarGtk : public InfoBarGtk,

// Returns the context menu model for this extension. Can be NULL if
// extension context menus are disabled.
ui::MenuModel* BuildMenuModel();
ExtensionContextMenuModel* BuildMenuModel();

CHROMEGTK_CALLBACK_1(ExtensionInfoBarGtk, void, OnSizeAllocate,
GtkAllocation*);
Expand All @@ -74,6 +75,9 @@ class ExtensionInfoBarGtk : public InfoBarGtk,
// child properties. Reparenting becomes easier too.
GtkWidget* alignment_;

// The model for the current menu displayed.
scoped_refptr<ExtensionContextMenuModel> context_menu_model_;

base::WeakPtrFactory<ExtensionInfoBarGtk> weak_ptr_factory_;

DISALLOW_COPY_AND_ASSIGN(ExtensionInfoBarGtk);
Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/ui/gtk/infobars/infobar_gtk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,7 @@ void InfoBarGtk::AddLabelWithInlineLink(const string16& display_text,
void InfoBarGtk::ShowMenuWithModel(GtkWidget* sender,
MenuGtk::Delegate* delegate,
ui::MenuModel* model) {
menu_model_.reset(model);
menu_.reset(new MenuGtk(delegate, menu_model_.get()));
menu_.reset(new MenuGtk(delegate, model));
menu_->PopupForWidget(sender, 1, gtk_get_current_event_time());
}

Expand Down Expand Up @@ -289,7 +288,6 @@ void InfoBarGtk::PlatformSpecificOnCloseSoon() {
// We must close all menus and prevent any signals from being emitted while
// we are animating the info bar closed.
menu_.reset();
menu_model_.reset();
signals_.reset();
}

Expand Down
4 changes: 1 addition & 3 deletions chrome/browser/ui/gtk/infobars/infobar_gtk.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ class InfoBarGtk : public InfoBar,
size_t link_offset,
GCallback callback);

// Shows the menu with |model| with the context of |sender|. InfobarGtk takes
// ownership of the model.
// Shows the menu with |model| with the context of |sender|.
void ShowMenuWithModel(GtkWidget* sender,
MenuGtk::Delegate* delegate,
ui::MenuModel* model);
Expand Down Expand Up @@ -132,7 +131,6 @@ class InfoBarGtk : public InfoBar,

// The current menu displayed. Can be null. We own this on the base class so
// we can cancel the menu while we're closing.
scoped_ptr<ui::MenuModel> menu_model_;
scoped_ptr<MenuGtk> menu_;

DISALLOW_COPY_AND_ASSIGN(InfoBarGtk);
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/ui/gtk/infobars/translate_infobar_base_gtk.cc
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@ TranslateInfoBarDelegate* TranslateInfoBarBase::GetDelegate() {
}

void TranslateInfoBarBase::OnOptionsClicked(GtkWidget* sender) {
ShowMenuWithModel(sender, NULL, new OptionsMenuModel(GetDelegate()));
menu_model_.reset(new OptionsMenuModel(GetDelegate()));
ShowMenuWithModel(sender, NULL, menu_model_.get());
}

// TranslateInfoBarDelegate specific method:
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/ui/gtk/infobars/translate_infobar_base_gtk.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ class TranslateInfoBarBase : public InfoBarGtk {
// Changes the color of the background from normal to error color and back.
scoped_ptr<ui::SlideAnimation> background_color_animation_;

// The model for the current menu displayed.
scoped_ptr<ui::MenuModel> menu_model_;

DISALLOW_COPY_AND_ASSIGN(TranslateInfoBarBase);
};

Expand Down

0 comments on commit e086357

Please sign in to comment.