diff options
author | sky@chromium.org <sky@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-07-21 00:08:19 +0000 |
---|---|---|
committer | sky@chromium.org <sky@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2011-07-21 00:08:19 +0000 |
commit | b05a08b74150acf8fc021263355a7a3e0d389f6c (patch) | |
tree | 06c193911fed406da4ab282a8c2c9b3abf17d958 | |
parent | 5701bb0620cf5f2c49767adcb2c1faa1b229285e (diff) | |
download | chromium_src-b05a08b74150acf8fc021263355a7a3e0d389f6c.zip chromium_src-b05a08b74150acf8fc021263355a7a3e0d389f6c.tar.gz chromium_src-b05a08b74150acf8fc021263355a7a3e0d389f6c.tar.bz2 |
Fixes crash in menu code. The crash occurs because
RenderViewContextMenuView may try to delete the MenuItemView while
it's running the nested message loop, which means many frames have
deleted objects on them so that when the stack unwinds there's
trouble. This is really just a temporary solution. The right fix is
57890, but as the branch point is close I'm going with this solution
for now.
BUG=89751 57890
TEST=none
R=jcivelli@chromium.org
Review URL: http://codereview.chromium.org/7465002
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@93293 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc | 12 | ||||
-rw-r--r-- | chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h | 4 | ||||
-rw-r--r-- | views/controls/menu/menu_item_view.h | 4 | ||||
-rw-r--r-- | views/controls/menu/menu_runner.cc | 92 | ||||
-rw-r--r-- | views/controls/menu/menu_runner.h | 49 | ||||
-rw-r--r-- | views/views.gyp | 2 |
6 files changed, 156 insertions, 7 deletions
diff --git a/chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc b/chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc index 577db98..9070005 100644 --- a/chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc +++ b/chrome/browser/ui/views/tab_contents/render_view_context_menu_views.cc @@ -14,6 +14,7 @@ #include "views/accelerator.h" #include "views/controls/menu/menu_item_view.h" #include "views/controls/menu/menu_model_adapter.h" +#include "views/controls/menu/menu_runner.h" //////////////////////////////////////////////////////////////////////////////// // RenderViewContextMenuViews, public: @@ -21,14 +22,16 @@ RenderViewContextMenuViews::RenderViewContextMenuViews( TabContents* tab_contents, const ContextMenuParams& params) - : RenderViewContextMenu(tab_contents, params) { + : RenderViewContextMenu(tab_contents, params), + menu_(NULL) { } RenderViewContextMenuViews::~RenderViewContextMenuViews() { } void RenderViewContextMenuViews::RunMenuAt(int x, int y) { - menu_->RunMenuAt(source_tab_contents_->view()->GetTopLevelNativeWindow(), + menu_runner_->RunMenuAt( + source_tab_contents_->view()->GetTopLevelNativeWindow(), NULL, gfx::Rect(gfx::Point(x, y), gfx::Size()), views::MenuItemView::TOPLEFT, true); } @@ -40,7 +43,7 @@ void RenderViewContextMenuViews::SetExternal() { #endif void RenderViewContextMenuViews::UpdateMenuItemStates() { - menu_delegate_->BuildMenu(menu_.get()); + menu_delegate_->BuildMenu(menu_); } //////////////////////////////////////////////////////////////////////////////// @@ -48,7 +51,8 @@ void RenderViewContextMenuViews::UpdateMenuItemStates() { void RenderViewContextMenuViews::PlatformInit() { menu_delegate_.reset(new views::MenuModelAdapter(&menu_model_)); - menu_.reset(new views::MenuItemView(menu_delegate_.get())); + menu_ = new views::MenuItemView(menu_delegate_.get()); + menu_runner_.reset(new views::MenuRunner(menu_)); UpdateMenuItemStates(); } diff --git a/chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h b/chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h index 2daa957..023a6c0 100644 --- a/chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h +++ b/chrome/browser/ui/views/tab_contents/render_view_context_menu_views.h @@ -13,6 +13,7 @@ namespace views { class MenuItemView; class MenuModelAdapter; +class MenuRunner; } // namespace views class RenderViewContextMenuViews : public RenderViewContextMenu { @@ -40,7 +41,8 @@ class RenderViewContextMenuViews : public RenderViewContextMenu { private: // The context menu itself and its contents. scoped_ptr<views::MenuModelAdapter> menu_delegate_; - scoped_ptr<views::MenuItemView> menu_; + views::MenuItemView* menu_; // Owned by menu_runner_. + scoped_ptr<views::MenuRunner> menu_runner_; DISALLOW_COPY_AND_ASSIGN(RenderViewContextMenuViews); }; diff --git a/views/controls/menu/menu_item_view.h b/views/controls/menu/menu_item_view.h index 3f952f7..ba2a365 100644 --- a/views/controls/menu/menu_item_view.h +++ b/views/controls/menu/menu_item_view.h @@ -114,8 +114,8 @@ class MenuItemView : public View { virtual ~MenuItemView(); // Overridden from View: - virtual bool GetTooltipText(const gfx::Point& p, std::wstring* tooltip) - OVERRIDE; + virtual bool GetTooltipText(const gfx::Point& p, + std::wstring* tooltip) OVERRIDE; virtual void GetAccessibleState(ui::AccessibleViewState* state) OVERRIDE; // Returns the preferred height of menu items. This is only valid when the diff --git a/views/controls/menu/menu_runner.cc b/views/controls/menu/menu_runner.cc new file mode 100644 index 0000000..78cb005 --- /dev/null +++ b/views/controls/menu/menu_runner.cc @@ -0,0 +1,92 @@ +// Copyright (c) 2011 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 "views/controls/menu/menu_runner.h" + +namespace views { + +// Manages the menu. To destroy a Holder invoke Release(). Release() deletes +// immediately if the menu isn't showing. If the menu is showing Release() +// cancels the menu and when the nested RunMenuAt() call returns, deletes itself +// and the menu. +class MenuRunner::Holder { + public: + explicit Holder(MenuItemView* menu); + + // See description above class for details. + void Release(); + + // Runs the menu. + void RunMenuAt(gfx::NativeWindow parent, + MenuButton* button, + const gfx::Rect& bounds, + MenuItemView::AnchorPosition anchor, + bool has_mnemonics); + + private: + ~Holder(); + + scoped_ptr<MenuItemView> menu_; + + // Are we in run waiting for it to return? + bool running_; + + // Set if |running_| and Release() has been invoked. + bool delete_after_run_; + + DISALLOW_COPY_AND_ASSIGN(Holder); +}; + +MenuRunner::Holder::Holder(MenuItemView* menu) + : menu_(menu), + running_(false), + delete_after_run_(false) { +} + +void MenuRunner::Holder::Release() { + if (running_) { + // The menu is running a nested message loop, we can't delete it now + // otherwise the stack would be in a really bad state (many frames would + // have deleted objects on them). Instead cancel the menu, when it returns + // Holder will delete itself. + delete_after_run_ = true; + menu_->Cancel(); + } else { + delete this; + } +} + +void MenuRunner::Holder::RunMenuAt(gfx::NativeWindow parent, + MenuButton* button, + const gfx::Rect& bounds, + MenuItemView::AnchorPosition anchor, + bool has_mnemonics) { + running_ = true; + menu_->RunMenuAt(parent, button, bounds, anchor, has_mnemonics); + if (delete_after_run_) { + delete this; + return; + } + running_ = false; +} + +MenuRunner::Holder::~Holder() { +} + +MenuRunner::MenuRunner(MenuItemView* menu) : holder_(new Holder(menu)) { +} + +MenuRunner::~MenuRunner() { + holder_->Release(); +} + +void MenuRunner::RunMenuAt(gfx::NativeWindow parent, + MenuButton* button, + const gfx::Rect& bounds, + MenuItemView::AnchorPosition anchor, + bool has_mnemonics) { + holder_->RunMenuAt(parent, button, bounds, anchor, has_mnemonics); +} + +} // namespace views diff --git a/views/controls/menu/menu_runner.h b/views/controls/menu/menu_runner.h new file mode 100644 index 0000000..4892b1e --- /dev/null +++ b/views/controls/menu/menu_runner.h @@ -0,0 +1,49 @@ +// Copyright (c) 2011 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. + +#ifndef VIEWS_CONTROLS_MENU_MENU_RUNNER_H_ +#define VIEWS_CONTROLS_MENU_MENU_RUNNER_H_ +#pragma once + +#include "base/basictypes.h" +#include "base/memory/scoped_ptr.h" +#include "views/controls/menu/menu_item_view.h" + +namespace views { + +class MenuButton; + +// MenuRunner handles the lifetime of the root MenuItemView. MenuItemView runs a +// nested message loop, which means care must be taken when the MenuItemView +// needs to be deleted. MenuRunner makes sure the menu is deleted after the +// nested message loop completes. +// +// MenuRunner can be deleted at any time and will correctly handle deleting the +// underlying menu. +// +// TODO: this is a work around for 57890. If we fix it this class shouldn't be +// needed. +class MenuRunner { + public: + explicit MenuRunner(MenuItemView* menu); + ~MenuRunner(); + + // Runs the menu. + void RunMenuAt(gfx::NativeWindow parent, + MenuButton* button, + const gfx::Rect& bounds, + MenuItemView::AnchorPosition anchor, + bool has_mnemonics); + + private: + class Holder; + + Holder* holder_; + + DISALLOW_COPY_AND_ASSIGN(MenuRunner); +}; + +} // namespace views + +#endif // VIEWS_CONTROLS_MENU_MENU_RUNNER_H_ diff --git a/views/views.gyp b/views/views.gyp index ab1ff5a..92859ae 100644 --- a/views/views.gyp +++ b/views/views.gyp @@ -134,6 +134,8 @@ 'controls/menu/menu_item_view_win.cc', 'controls/menu/menu_model_adapter.cc', 'controls/menu/menu_model_adapter.h', + 'controls/menu/menu_runner.cc', + 'controls/menu/menu_runner.h', 'controls/menu/menu_scroll_view_container.cc', 'controls/menu/menu_scroll_view_container.h', 'controls/menu/menu_separator.h', |