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

7 files changed, 79 insertions, 10 deletions

diff --git a/base/base_drag_source.cc b/base/base_drag_source.cc
index bdaab1e..bc81395 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 dea89af..3a4a94c 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 9832fa4..bcbb09b 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 6acbb30..b8e949b 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 d61f3ba2..8a9ce8b 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 0026c6b..7b6aabb 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<WebDragSource> 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 4b5ade1..b2739dc 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<WebDragSource> 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<TabContentsViewWin> close_tab_timer_;
+
   DISALLOW_COPY_AND_ASSIGN(TabContentsViewWin);
 };