Fixes use after free during drag and drop.

Fixes use after free during drag and drop.

Apparently under windows requesting focus can trigger capture lost,
which caused all sorts of problems with tab dragging. When you've
dragged enough to initiate a real tab drag we save focus. The code
never expected saving focus to trigger a capture lost (which stops tab
dragging), which triggered the crash. The fix is to make the code
allow for saving focus to delete the TabDragController.

I'm also converting from storing a raw View* for the previously focused view to a ViewStorage id. This way if the previously focused view is deleted no crashes.

I had to add a bit of infrastructure for the test.

BUG=275931
TEST=covered by test now.
R=ben@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221456

[Ben Goodger (Google), 2013-09-04 03:15:35]

patch is malformed


On Tue, Sep 3, 2013 at 4:44 PM, <sky@chromium.org> wrote:

> Reviewers: Ben Goodger (Google),
>
> Description:
> Fixes use after free during drag and drop.
>
> Apparently under windows requesting focus can trigger capture lost,
> which caused all sorts of problems with tab dragging. When you've
> dragged enough to initiate a real tab drag we save focus. The code
> never expected saving focus to trigger a capture lost (which stops tab
> dragging), which triggered the crash. The fix is to make the code
> allow for saving focus to delete the TabDragController.
>
> I'm also converting from storing a raw View* for the previously focused
> view to
> a ViewStorage id. This way if the previously focused view is deleted no
> crashes.
>
> I had to add a bit of infrastructure for the test.
>
> BUG=275931
> TEST=covered by test now.
> R=ben@chromium.org
>
> Please review this at
https://codereview.chromium.**org/23523018/<https://codereview.chromium.org/2...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/browser/ui/views/frame/**browser_frame.cc
>   M chrome/browser/ui/views/frame/**browser_frame_aura.cc
>   M chrome/browser/ui/views/frame/**browser_frame_win.cc
>   M chrome/browser/ui/views/frame/**desktop_browser_frame_aura.h
>   M chrome/browser/ui/views/frame/**desktop_browser_frame_aura.cc
>   M chrome/browser/ui/views/frame/**native_browser_frame.h
>   A chrome/browser/ui/views/frame/**native_browser_frame_factory.h
>   A chrome/browser/ui/views/frame/**native_browser_frame_factory.**cc
>   A chrome/browser/ui/views/frame/**native_browser_frame_factory_**aura.cc
>   A chrome/browser/ui/views/frame/**native_browser_frame_factory_**win.cc
>   M chrome/browser/ui/views/tabs/**tab_drag_controller.h
>   M chrome/browser/ui/views/tabs/**tab_drag_controller.cc
>   M chrome/browser/ui/views/tabs/**tab_drag_controller_**
> interactive_uitest.cc
>   M chrome/chrome_browser_ui.gypi
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2013-09-04 14:07:21]

GAH! Try agian.

[Ben Goodger (Google), 2013-09-04 17:08:59]

lgtm

[commit-bot: I haz the power, 2013-09-04 17:15:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/23523018/46001

[commit-bot: I haz the power, 2013-09-04 17:15:49]

Failed to apply patch for chrome/browser/ui/views/tabs/tab_drag_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/tabs/tab_drag_controller.cc
  Hunk #1 succeeded at 43 (offset -1 lines).
  Hunk #2 succeeded at 361 (offset -1 lines).
  Hunk #3 FAILED at 385.
  Hunk #4 succeeded at 500 (offset -3 lines).
  Hunk #5 FAILED at 756.
  2 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/tabs/tab_drag_controller.cc.rej

Patch:       chrome/browser/ui/views/tabs/tab_drag_controller.cc
Index: chrome/browser/ui/views/tabs/tab_drag_controller.cc
diff --git a/chrome/browser/ui/views/tabs/tab_drag_controller.cc
b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
index
7ceb529be3ded59d679e6b2daa5f974741f046e8..197d9229799c087f49f17b8101a2e72a802fafdd
100644
--- a/chrome/browser/ui/views/tabs/tab_drag_controller.cc
+++ b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
@@ -44,6 +44,7 @@
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/screen.h"
+#include "ui/views/focus/view_storage.h"
 #include "ui/views/widget/root_view.h"
 #include "ui/views/widget/widget.h"
 
@@ -361,7 +362,8 @@ TabDragController::TabDragController()
       screen_(NULL),
       host_desktop_type_(chrome::HOST_DESKTOP_TYPE_NATIVE),
       offset_to_width_ratio_(0),
-      old_focused_view_(NULL),
+      old_focused_view_id_(
+          views::ViewStorage::GetInstance()->CreateStorageID()),
       last_move_screen_loc_(0),
       started_drag_(false),
       active_(true),
@@ -383,6 +385,9 @@ TabDragController::TabDragController()
 
 TabDragController::~TabDragController() {
   CHECK(!saving_focus_);
+
+  views::ViewStorage::GetInstance()->RemoveView(old_focused_view_id_);
+
   if (instance_ == this)
     instance_ = NULL;
 
@@ -501,8 +506,16 @@ void TabDragController::Drag(const gfx::Point&
point_in_screen) {
     if (!CanStartDrag(point_in_screen))
       return;  // User hasn't dragged far enough yet.
 
+    // On windows SaveFocus() may trigger a capture lost, which destroys us.
+    {
+      bool destroyed = false;
+      destroyed_ = &destroyed;
+      SaveFocus();
+      if (destroyed)
+        return;
+      destroyed_ = NULL;
+    }
     started_drag_ = true;
-    SaveFocus();
     Attach(source_tabstrip_, gfx::Point());
     if (detach_into_browser_ && static_cast<int>(drag_data_.size()) ==
         GetModel(source_tabstrip_)->count()) {
@@ -746,24 +759,25 @@ void TabDragController::UpdateDockInfo(const gfx::Point&
point_in_screen) {
 }
 
 void TabDragController::SaveFocus() {
-  DCHECK(!old_focused_view_);  // This should only be invoked once.
   DCHECK(source_tabstrip_);
-  old_focused_view_ = source_tabstrip_->GetFocusManager()->GetFocusedView();
-  // TODO(sky): used in tracking crash; see 275931.
-  const char* old_focused_view_class_name =
-      old_focused_view_ ? old_focused_view_->GetClassName() : NULL;
-  base::debug::Alias(old_focused_view_class_name);
-  const int old_focused_view_id =
-      old_focused_view_ ? old_focused_view_->id() : 0;
-  base::debug::Alias(&old_focused_view_id);
-  base::AutoReset<bool> setter(&saving_focus_, true);
+  views::View* focused_view =
+      source_tabstrip_->GetFocusManager()->GetFocusedView();
+  if (focused_view)
+    views::ViewStorage::GetInstance()->StoreView(old_focused_view_id_,
+                                                 focused_view);
   source_tabstrip_->GetFocusManager()->SetFocusedView(source_tabstrip_);
+  // WARNING: we may have been deleted.
 }
 
 void TabDragController::RestoreFocus() {
-  if (old_focused_view_ && attached_tabstrip_ == source_tabstrip_)
-    old_focused_view_->GetFocusManager()->SetFocusedView(old_focused_view_);
-  old_focused_view_ = NULL;
+  if (attached_tabstrip_ != source_tabstrip_)
+    return;
+  views::View* old_focused_view =
+      views::ViewStorage::GetInstance()->RetrieveView(
+      old_focused_view_id_);
+  if (!old_focused_view)
+    return;
+  old_focused_view->GetFocusManager()->SetFocusedView(old_focused_view);
 }
 
 bool TabDragController::CanStartDrag(const gfx::Point& point_in_screen) const {

[commit-bot: I haz the power, 2013-09-04 18:16:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/23523018/2001

[commit-bot: I haz the power, 2013-09-04 18:24:08]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-05 15:32:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sky@chromium.org/23523018/2001

[commit-bot: I haz the power, 2013-09-05 18:09:38]

Change committed as 221456