From f5a077e1fd10c454f60621ad0b8ace6e9f0c3a9b Mon Sep 17 00:00:00 2001 From: "tc@google.com" Date: Fri, 10 Jul 2009 23:10:42 +0000 Subject: [PATCH] Fix a crash that happens if a tab is closed while we're in the middle of a drag originating from the tab. The problem is that the tab gets deleted out from under the drag operation which is happening in a nested message loop. To work around this, if we're in the middle of a drag and we get a tab close request, delay the tab close until after the drag operation is finished. BUG=16280 Review URL: http://codereview.chromium.org/149466 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@20436 0039d316-1c4b-4281-b951-d872f2087c98 --- base/base_drag_source.cc | 5 ++- base/base_drag_source.h | 10 ++++++ chrome/browser/tab_contents/tab_contents.cc | 7 ++++ .../browser/tab_contents/tab_contents_view.h | 8 +++++ .../browser/tab_contents/web_drag_source.cc | 5 --- .../tab_contents/tab_contents_view_win.cc | 35 ++++++++++++++++--- .../tab_contents/tab_contents_view_win.h | 19 ++++++++++ 7 files changed, 79 insertions(+), 10 deletions(-) diff --git a/base/base_drag_source.cc b/base/base_drag_source.cc index bdaab1eba93e8..bc81395525746 100644 --- a/base/base_drag_source.cc +++ b/base/base_drag_source.cc @@ -7,7 +7,7 @@ /////////////////////////////////////////////////////////////////////////////// // BaseDragSource, public: -BaseDragSource::BaseDragSource() : ref_count_(0) { +BaseDragSource::BaseDragSource() : ref_count_(0), cancel_drag_(false) { } /////////////////////////////////////////////////////////////////////////////// @@ -15,6 +15,9 @@ BaseDragSource::BaseDragSource() : ref_count_(0) { HRESULT BaseDragSource::QueryContinueDrag(BOOL escape_pressed, DWORD key_state) { + if (cancel_drag_) + return DRAGDROP_S_CANCEL; + if (escape_pressed) { OnDragSourceCancel(); return DRAGDROP_S_CANCEL; diff --git a/base/base_drag_source.h b/base/base_drag_source.h index dea89aff02288..3a4a94cd1cd8a 100644 --- a/base/base_drag_source.h +++ b/base/base_drag_source.h @@ -23,6 +23,13 @@ class BaseDragSource : public IDropSource { BaseDragSource(); virtual ~BaseDragSource() { } + // Stop the drag operation at the next chance we get. This doesn't + // synchronously stop the drag (since Windows is controlling that), + // but lets us tell Windows to cancel the drag the next chance we get. + void CancelDrag() { + cancel_drag_ = true; + } + // IDropSource implementation: HRESULT __stdcall QueryContinueDrag(BOOL escape_pressed, DWORD key_state); HRESULT __stdcall GiveFeedback(DWORD effect); @@ -40,6 +47,9 @@ class BaseDragSource : public IDropSource { private: LONG ref_count_; + // Set to true if we want to cancel the drag operation. + bool cancel_drag_; + DISALLOW_EVIL_CONSTRUCTORS(BaseDragSource); }; diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc index 9832fa4e4ee2a..bcbb09b652e0e 100644 --- a/chrome/browser/tab_contents/tab_contents.cc +++ b/chrome/browser/tab_contents/tab_contents.cc @@ -1904,6 +1904,13 @@ void TabContents::UpdateInspectorSettings(const std::wstring& raw_settings) { } void TabContents::Close(RenderViewHost* rvh) { + // If we close the tab while we're in the middle of a drag, we'll crash. + // Instead, cancel the drag and close it as soon as the drag ends. + if (view()->IsDoingDrag()) { + view()->CancelDragAndCloseTab(); + return; + } + // Ignore this if it comes from a RenderViewHost that we aren't showing. if (delegate() && rvh == render_view_host()) delegate()->CloseContents(this); diff --git a/chrome/browser/tab_contents/tab_contents_view.h b/chrome/browser/tab_contents/tab_contents_view.h index 6acbb302ce7bd..b8e949b5279be 100644 --- a/chrome/browser/tab_contents/tab_contents_view.h +++ b/chrome/browser/tab_contents/tab_contents_view.h @@ -138,6 +138,14 @@ class TabContentsView : public RenderViewHostDelegate::View { return preferred_width_; } + // If we try to close the tab while a drag is in progress, we crash. These + // methods allow the tab contents to determine if a drag is in progress and + // postpone the tab closing. + virtual bool IsDoingDrag() const { + return false; + } + virtual void CancelDragAndCloseTab() {} + protected: TabContentsView() {} // Abstract interface. diff --git a/chrome/browser/tab_contents/web_drag_source.cc b/chrome/browser/tab_contents/web_drag_source.cc index d61f3ba217091..8a9ce8bdd37c3 100644 --- a/chrome/browser/tab_contents/web_drag_source.cc +++ b/chrome/browser/tab_contents/web_drag_source.cc @@ -37,11 +37,6 @@ WebDragSource::WebDragSource(gfx::NativeWindow source_wnd, : BaseDragSource(), source_wnd_(source_wnd), render_view_host_(render_view_host) { - // In an effort to try to track down http://crbug.com/12524 we now CHECK - // when a NULL render_view_host is passed to us. I think this is what is - // happening but it is hard to tell since the minidump is not helpful in this - // case. At least we can then rule that out. - CHECK(render_view_host_); } void WebDragSource::OnDragSourceCancel() { diff --git a/chrome/browser/views/tab_contents/tab_contents_view_win.cc b/chrome/browser/views/tab_contents/tab_contents_view_win.cc index 0026c6b5630d8..7b6aabb691ca2 100644 --- a/chrome/browser/views/tab_contents/tab_contents_view_win.cc +++ b/chrome/browser/views/tab_contents/tab_contents_view_win.cc @@ -8,6 +8,7 @@ #include "app/gfx/canvas_paint.h" #include "app/os_exchange_data.h" +#include "base/time.h" #include "chrome/browser/bookmarks/bookmark_drag_data.h" #include "chrome/browser/browser.h" // TODO(beng): this dependency is awful. #include "chrome/browser/browser_process.h" @@ -54,7 +55,8 @@ TabContentsView* TabContentsView::Create(TabContents* tab_contents) { TabContentsViewWin::TabContentsViewWin(TabContents* tab_contents) : TabContentsView(tab_contents), ignore_next_char_event_(false), - focus_manager_(NULL) { + focus_manager_(NULL), + close_tab_after_drag_ends_(false) { last_focused_view_storage_id_ = views::ViewStorage::GetSharedInstance()->CreateStorageID(); } @@ -67,6 +69,8 @@ TabContentsViewWin::~TabContentsViewWin() { views::ViewStorage* view_storage = views::ViewStorage::GetSharedInstance(); if (view_storage->RetrieveView(last_focused_view_storage_id_) != NULL) view_storage->RemoveView(last_focused_view_storage_id_); + + DCHECK(!drag_source_.get()); } void TabContentsViewWin::Unparent() { @@ -174,8 +178,8 @@ void TabContentsViewWin::StartDragging(const WebDropData& drop_data) { if (!drop_data.plain_text.empty()) data->SetString(drop_data.plain_text); - scoped_refptr drag_source( - new WebDragSource(GetNativeView(), tab_contents()->render_view_host())); + drag_source_ = new WebDragSource(GetNativeView(), + tab_contents()->render_view_host()); DWORD effects; @@ -183,9 +187,15 @@ void TabContentsViewWin::StartDragging(const WebDropData& drop_data) { // updates while in the system DoDragDrop loop. bool old_state = MessageLoop::current()->NestableTasksAllowed(); MessageLoop::current()->SetNestableTasksAllowed(true); - DoDragDrop(data, drag_source, DROPEFFECT_COPY | DROPEFFECT_LINK, &effects); + DoDragDrop(data, drag_source_, DROPEFFECT_COPY | DROPEFFECT_LINK, &effects); MessageLoop::current()->SetNestableTasksAllowed(old_state); + drag_source_ = NULL; + if (close_tab_after_drag_ends_) { + close_tab_timer_.Start(base::TimeDelta::FromMilliseconds(0), this, + &TabContentsViewWin::CloseTab); + } + if (tab_contents()->render_view_host()) tab_contents()->render_view_host()->DragSourceSystemDragEnded(); } @@ -334,6 +344,19 @@ void TabContentsViewWin::RestoreFocus() { } } +bool TabContentsViewWin::IsDoingDrag() const { + return drag_source_.get() != NULL; +} + +void TabContentsViewWin::CancelDragAndCloseTab() { + DCHECK(IsDoingDrag()); + // We can't close the tab while we're in the drag and + // |drag_source_->CancelDrag()| is async. Instead, set a flag to cancel + // the drag and when the drag nested message loop ends, close the tab. + drag_source_->CancelDrag(); + close_tab_after_drag_ends_ = true; +} + void TabContentsViewWin::UpdateDragCursor(bool is_drop_target) { drop_target_->set_is_drop_target(is_drop_target); } @@ -422,6 +445,10 @@ views::FocusManager* TabContentsViewWin::GetFocusManager() { return focus_manager_; } +void TabContentsViewWin::CloseTab() { + tab_contents()->Close(tab_contents()->render_view_host()); +} + void TabContentsViewWin::ShowContextMenu(const ContextMenuParams& params) { // Allow delegates to handle the context menu operation first. if (tab_contents()->delegate()->HandleContextMenu(params)) diff --git a/chrome/browser/views/tab_contents/tab_contents_view_win.h b/chrome/browser/views/tab_contents/tab_contents_view_win.h index 4b5ade194a104..b2739dc0d0316 100644 --- a/chrome/browser/views/tab_contents/tab_contents_view_win.h +++ b/chrome/browser/views/tab_contents/tab_contents_view_win.h @@ -7,12 +7,14 @@ #include "base/gfx/size.h" #include "base/scoped_ptr.h" +#include "base/timer.h" #include "chrome/browser/tab_contents/tab_contents_view.h" #include "views/widget/widget_win.h" class RenderViewContextMenuWin; class SadTabView; struct WebDropData; +class WebDragSource; class WebDropTarget; // Windows-specific implementation of the TabContentsView. It is a HWND that @@ -47,6 +49,8 @@ class TabContentsViewWin : public TabContentsView, virtual void SetInitialFocus(); virtual void StoreFocus(); virtual void RestoreFocus(); + virtual bool IsDoingDrag() const; + virtual void CancelDragAndCloseTab(); // Backend implementation of RenderViewHostDelegate::View. virtual void ShowContextMenu(const ContextMenuParams& params); @@ -60,6 +64,9 @@ class TabContentsViewWin : public TabContentsView, virtual views::FocusManager* GetFocusManager(); private: + // A helper method for closing the tab. + void CloseTab(); + // Windows events ------------------------------------------------------------ // Overrides from WidgetWin. @@ -115,6 +122,18 @@ class TabContentsViewWin : public TabContentsView, // accessible when unparented. views::FocusManager* focus_manager_; + // |drag_source_| is our callback interface passed to the system when we + // want to initiate a drag and drop operation. We use it to tell if a + // drag operation is happening. + scoped_refptr drag_source_; + + // Set to true if we want to close the tab after the system drag operation + // has finished. + bool close_tab_after_drag_ends_; + + // Used to close the tab after the stack has unwound. + base::OneShotTimer close_tab_timer_; + DISALLOW_COPY_AND_ASSIGN(TabContentsViewWin); };