Mac: Fix crash when dragging tabs

Mac: Fix crash when dragging tabs

When dragging the only tab of a window, and there are other windows that are
potential targets, it was possible in some cases for the window to deallocate
while we still hold a weak reference to it.

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

[Robert Sesek, 2014-03-25 18:22:31]

I'm trying to understand why this change caused this crash, especially given the
crash stack. Do you understand it? I'm not quite following how the old
targetController can go away on line 218 (a JS close?).

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/tabs/...

[Andre, 2014-03-25 18:30:32]

On 2014/03/25 18:22:31, rsesek wrote:
> I'm trying to understand why this change caused this crash, especially given
the
> crash stack. Do you understand it? I'm not quite following how the old
> targetController can go away on line 218 (a JS close?).
> 
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/tabs/...

It's strange, I don't understand how the window controller can get deallocated
given the steps above.
And it apparently only crashes on 10.7.5, byt I was not able to repro on my
10.7.5 VM.
But I think retaining it during drag is the right thing to do, anything can
happen while running the event loop and there seems to be no advantage from
making it weak.

[Robert Sesek, 2014-03-25 21:09:12]

On 2014/03/25 18:30:32, Andre wrote:
> On 2014/03/25 18:22:31, rsesek wrote:
> > I'm trying to understand why this change caused this crash, especially given
> the
> > crash stack. Do you understand it? I'm not quite following how the old
> > targetController can go away on line 218 (a JS close?).
> > 
> >
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/tabs/...
> 
> It's strange, I don't understand how the window controller can get deallocated
> given the steps above.
> And it apparently only crashes on 10.7.5, byt I was not able to repro on my
> 10.7.5 VM.
> But I think retaining it during drag is the right thing to do, anything can
> happen while running the event loop and there seems to be no advantage from
> making it weak.

I was able to repro using modified steps:

1. Open chrome with two tabs
2. Drag out the first of the two tabs into a new window
3. Drag it back in as position 1

Repeat steps 2 and 3 once more if it doesn't crash. It crashes for me within 2
tries.

While it's probably fine to add these retains, I'd like to understand why this
is happening, because this kind of seems like papering over a fix (i.e. a deeper
issue may be present). Also these have been weak pointers since the beginning,
so I'm a bit hesitant to just change them without fully understanding the reason
for the crash.

[Andre, 2014-03-26 01:23:02]

On 2014/03/25 21:09:12, rsesek wrote:
> On 2014/03/25 18:30:32, Andre wrote:
> > On 2014/03/25 18:22:31, rsesek wrote:
> > > I'm trying to understand why this change caused this crash, especially
given
> > the
> > > crash stack. Do you understand it? I'm not quite following how the old
> > > targetController can go away on line 218 (a JS close?).
> > > 
> > >
> >
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/cocoa/tabs/...
> > 
> > It's strange, I don't understand how the window controller can get
deallocated
> > given the steps above.
> > And it apparently only crashes on 10.7.5, byt I was not able to repro on my
> > 10.7.5 VM.
> > But I think retaining it during drag is the right thing to do, anything can
> > happen while running the event loop and there seems to be no advantage from
> > making it weak.
> 
> I was able to repro using modified steps:
> 
> 1. Open chrome with two tabs
> 2. Drag out the first of the two tabs into a new window
> 3. Drag it back in as position 1
> 
> Repeat steps 2 and 3 once more if it doesn't crash. It crashes for me within 2
> tries.
> 
> While it's probably fine to add these retains, I'd like to understand why this
> is happening, because this kind of seems like papering over a fix (i.e. a
deeper
> issue may be present). Also these have been weak pointers since the beginning,
> so I'm a bit hesitant to just change them without fully understanding the
reason
> for the crash.

Thanks Robert.
I'm still not able to repro using your steps, but can repro using these steps:
1. Two windows, each with 1 tab (new tab page is ok).
2. Drag one of the tab up or down (not sideways) and very quickly release.
3. Repeat #2 many times.

I think this is what might be what is happening:
1. We drag the only tab of a window.
2. With the new code, we set that window controller as our targetController_
3. The window controller deallocates because it has no more tabs
4. The next time continueDrag: is invoked, we message the zombie
targetController_.

Usually #4 will happen before #3, so we get the chance to nil targetController_
first.
But sometimes the opposite can happen.

After further testing, I also found that the delayed removePlaceholder call
makes tab tear-off
a little less smooth (very hard to tell). I've fixed that as well.

I have uploaded a new fix that the handles the 3 different cases:
1. dragging the only tab of a window
2. dragging the tab horizontally
3. dragging the tab vertically

[Andre, 2014-03-26 01:29:30]

https://codereview.chromium.org/199663004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (left):

https://codereview.chromium.org/199663004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:191: // moving more
than 3 pixels.
I think this comment is outdated.
The distance is specified by kTearDistance above.

[Robert Sesek, 2014-03-26 16:17:18]

LGTM thanks for investigating.

[Andre, 2014-03-26 16:22:20]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-03-26 16:22:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andresantoso@chromium.org/199663004/80001

[commit-bot: I haz the power, 2014-03-27 01:35:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 01:35:54]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Andre, 2014-03-27 03:03:30]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-03-27 03:06:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andresantoso@chromium.org/199663004/80001

[commit-bot: I haz the power, 2014-03-27 03:09:53]

Change committed as 259773