Fixes tab contents crash. This changes a number of things in dragged

tab controller:

. DraggedTabController now installs itself as the delegate of the
  TabContents immediately. It needs to do this so that it can stay in
  sync if the selected TabContents of the NavigationController
  changes.
. DraggedTabController forwards changes to the selected TabContents to
  the original delegate. This needs to be done so that the original
  delegate can do any cleanup it needs to do. For example, Browser
  needs to know if the tab contents changes so that it can remove
  entries from its internal mapping of what TabContents need to be
  updated. Similarly the TabStripModel needs to install a listener on
  the new TabContents type.
. If the tab is destroyed while dragging we shouldn't set the
  attached_tabstrip_ to NULL. Doing so results in invoking
  source_tabstrip_->DestroyDraggedSourceTab(source_tab_), which is not
  what should happen.

These changes are subtle, so give them a good think.

BUG=6369
TEST=this is covered by chrome bot tests. But be sure and exercise tab
  dragging in as many permutations as you can to make sure this
  doesn't break anything.

Review URL: http://codereview.chromium.org/19026

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@8731 0039d316-1c4b-4281-b951-d872f2087c98

1 files changed, 40 insertions, 13 deletions

diff --git a/chrome/browser/views/tabs/dragged_tab_controller.cc b/chrome/browser/views/tabs/dragged_tab_controller.cc
index 6334c7b..4d35ad8 100644
--- a/chrome/browser/views/tabs/dragged_tab_controller.cc
+++ b/chrome/browser/views/tabs/dragged_tab_controller.cc
@@ -399,16 +399,23 @@ void DraggedTabController::NavigationStateChanged(const TabContents* source,
 void DraggedTabController::ReplaceContents(TabContents* source,
                                            TabContents* new_contents) {
   DCHECK(dragged_contents_ == source);
-  source->set_delegate(NULL);
-  new_contents->set_delegate(this);
 
   // If we're attached to a TabStrip, we need to tell the TabStrip that this
   // TabContents was replaced.
-  if (attached_tabstrip_ && attached_tabstrip_->model() && dragged_contents_) {
-    int index =
-        attached_tabstrip_->model()->GetIndexOfTabContents(dragged_contents_);
-    if (index != TabStripModel::kNoTab)
-      attached_tabstrip_->model()->ReplaceTabContentsAt(index, new_contents);
+  if (attached_tabstrip_ && dragged_contents_) {
+    if (original_delegate_) {
+      original_delegate_->ReplaceContents(source, new_contents);
+      // ReplaceContents on the original delegate is going to reset the delegate
+      // for us. We need to unset original_delegate_ here so that
+      // ChangeDraggedContents doesn't attempt to restore the delegate to the
+      // wrong value.
+      original_delegate_ = NULL;
+    } else if (attached_tabstrip_->model()) {
+      int index =
+          attached_tabstrip_->model()->GetIndexOfTabContents(dragged_contents_);
+      if (index != TabStripModel::kNoTab)
+        attached_tabstrip_->model()->ReplaceTabContentsAt(index, new_contents);
+    }
   }
 
   // Update our internal state.
@@ -557,12 +564,22 @@ void DraggedTabController::ChangeDraggedContents(TabContents* new_contents) {
     NotificationService::current()->RemoveObserver(this,
       NOTIFY_TAB_CONTENTS_DESTROYED,
       Source<TabContents>(dragged_contents_));
+    if (original_delegate_)
+      dragged_contents_->set_delegate(original_delegate_);
   }
+  original_delegate_ = NULL;
   dragged_contents_ = new_contents;
   if (dragged_contents_) {
     NotificationService::current()->AddObserver(this,
       NOTIFY_TAB_CONTENTS_DESTROYED,
       Source<TabContents>(dragged_contents_));
+
+    // We need to be the delegate so we receive messages about stuff,
+    // otherwise our dragged_contents() may be replaced and subsequently
+    // collected/destroyed while the drag is in process, leading to
+    // nasty crashes.
+    original_delegate_ = dragged_contents_->delegate();
+    dragged_contents_->set_delegate(this);
   }
 }
 
@@ -820,11 +837,7 @@ void DraggedTabController::Detach() {
   if (view_.get())
     view_->Detach(photobooth_.get());
 
-  // We need to be the delegate so we receive messages about stuff,
-  // otherwise our dragged_contents() may be replaced and subsequently
-  // collected/destroyed while the drag is in process, leading to
-  // nasty crashes.
-  original_delegate_ = dragged_contents_->delegate();
+  // Detaching resets the delegate, but we still want to be the delegate.
   dragged_contents_->set_delegate(this);
 
   attached_tabstrip_ = NULL;
@@ -922,6 +935,12 @@ Tab* DraggedTabController::GetTabMatchingDraggedContents(
 }
 
 bool DraggedTabController::EndDragImpl(EndDragType type) {
+  // WARNING: this may be invoked multiple times. In particular, if deletion
+  // occurs after a delay (as it does when the tab is released in the original
+  // tab strip) and the navigation controller/tab contents is deleted before
+  // the animation finishes, this is invoked twice. The second time through
+  // type == TAB_DESTROYED.
+
   bring_to_front_timer_.Stop();
 
   // Hide the current dock controllers.
@@ -947,6 +966,8 @@ bool DraggedTabController::EndDragImpl(EndDragType type) {
         destroy_now = CompleteDrag();
       }
     }
+    if (dragged_contents_ && dragged_contents_->delegate() == this)
+      dragged_contents_->set_delegate(original_delegate_);
   } else {
     // If we get here it means the NavigationController is going down. Don't
     // attempt to do any cleanup other than resetting the delegate (if we're
@@ -954,8 +975,14 @@ bool DraggedTabController::EndDragImpl(EndDragType type) {
     if (dragged_contents_ && dragged_contents_->delegate() == this)
       dragged_contents_->set_delegate(NULL);
     dragged_contents_ = NULL;
-    attached_tabstrip_ = NULL;
   }
+
+  // The delegate of the dragged contents should have been reset. Unset the
+  // original delegate so that we don't attempt to reset the delegate when
+  // deleted.
+  DCHECK(!dragged_contents_ || dragged_contents_->delegate() != this);
+  original_delegate_ = NULL;
+
   // If we're not destroyed now, we'll be destroyed asynchronously later.
   if (destroy_now)
     source_tabstrip_->DestroyDragController();