Add Alias and Checks to TabDragController to isolate crash

Have TabDragController perform checks in GetTabStripForWindow, along with aliasing of pointers, in order to track down the cause of a crash.

BUG=585017
Committed: https://crrev.com/cc9ee3eb04c7cdf3719ec4914b3d14feae80e18c
Cr-Commit-Position: refs/heads/master@{#379102}

[jonross, 2016-02-23 22:19:19]

jonross@chromium.org changed reviewers:
+ ananta@chromium.org, sky@chromium.org

[jonross, 2016-02-23 22:19:19]

After speaking with ananta@ he does not believe that the crash is occurring due
to delayed touch processing.

It appears though that a window/view is being deleted in the middle of a drag
operation. So TabDragController is attempting to access deleted TabStrips.

Without reproduction steps I'm proposing adding some CHECK for TabStrip deletion
so that we can detect this earlier. Then fix the actual cause.

Thoughts?

[sky, 2016-02-24 16:41:47]

I don't think this is it either. TabDragController::attached_tabstrip_ owns the
TabDragController.

[jonross, 2016-02-24 16:52:30]

On 2016/02/24 16:41:47, sky wrote:
> I don't think this is it either. TabDragController::attached_tabstrip_ owns
the
> TabDragController.

True, which is why I would expect TabDragController to be destroyed when
attached_tabstrip_ is.

From the dump TabStrip::ContinueDrag appears to be called from a TabStrip other
than attached_tabstrip_/source_tabstrip_.

[sky, 2016-02-24 20:41:41]

On Wed, Feb 24, 2016 at 8:52 AM,  <jonross@chromium.org> wrote:
> On 2016/02/24 16:41:47, sky wrote:
>> I don't think this is it either. TabDragController::attached_tabstrip_
>> owns
> the
>> TabDragController.
>
> True, which is why I would expect TabDragController to be destroyed when
> attached_tabstrip_ is.
>
> From the dump TabStrip::ContinueDrag appears to be called from a TabStrip
> other
> than attached_tabstrip_/source_tabstrip_.

What makes you think that? Even if that were the case, that doesn't
explain the crash.

If other->controller() is bogus, then that means it's the other window
that is in a bad state, not the one being dragged. The other window
here being the window we're attempting to attach to.

  -Scott

>
> https://codereview.chromium.org/1726743003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jonross, 2016-03-01 21:39:32]

On 2016/02/24 20:41:41, sky wrote:
> On Wed, Feb 24, 2016 at 8:52 AM,  <mailto:jonross@chromium.org> wrote:
> > On 2016/02/24 16:41:47, sky wrote:
> >> I don't think this is it either. TabDragController::attached_tabstrip_
> >> owns
> > the
> >> TabDragController.
> >
> > True, which is why I would expect TabDragController to be destroyed when
> > attached_tabstrip_ is.
> >
> > From the dump TabStrip::ContinueDrag appears to be called from a TabStrip
> > other
> > than attached_tabstrip_/source_tabstrip_.
> 
> What makes you think that? Even if that were the case, that doesn't
> explain the crash.

Different pointer values for the tabstrip calling ContinueDrag vs those of
attached_tabstrip_/source_tabstrip_ in the dump.

> 
> If other->controller() is bogus, then that means it's the other window
> that is in a bad state, not the one being dragged. The other window
> here being the window we're attempting to attach to.
> 
>   -Scott
> 

True. I could instead add a check in TabDragController::GetTabStripForWindow to
verify that other_tabstrip->controller() is valid to confirm that.

Additionally I could update IsCompatibleWith to verify there that
other->controller() is valid. Which would check if there is an issue with
attached_tabstrip_/source_tabstrip_

Thoughts?

[sky, 2016-03-02 00:18:22]

On Tue, Mar 1, 2016 at 1:39 PM,  <jonross@chromium.org> wrote:
> On 2016/02/24 20:41:41, sky wrote:
>> On Wed, Feb 24, 2016 at 8:52 AM, <mailto:jonross@chromium.org> wrote:
>> > On 2016/02/24 16:41:47, sky wrote:
>> >> I don't think this is it either. TabDragController::attached_tabstrip_
>> >> owns
>> > the
>> >> TabDragController.
>> >
>> > True, which is why I would expect TabDragController to be destroyed when
>> > attached_tabstrip_ is.
>> >
>> > From the dump TabStrip::ContinueDrag appears to be called from a
>> > TabStrip
>> > other
>> > than attached_tabstrip_/source_tabstrip_.
>>
>> What makes you think that? Even if that were the case, that doesn't
>> explain the crash.
>
> Different pointer values for the tabstrip calling ContinueDrag vs those of
> attached_tabstrip_/source_tabstrip_ in the dump.

Are you sure the values are good? You can't always trust the data in a
dump. If you want to definitively know you need to use
base::Debug::Alias and put the values on the stack.

>
>>
>> If other->controller() is bogus, then that means it's the other window
>> that is in a bad state, not the one being dragged. The other window
>> here being the window we're attempting to attach to.
>>
>> -Scott
>>
>
> True. I could instead add a check in TabDragController::GetTabStripForWindow
> to
> verify that other_tabstrip->controller() is valid to confirm that.
>
> Additionally I could update IsCompatibleWith to verify there that
> other->controller() is valid. Which would check if there is an issue with
> attached_tabstrip_/source_tabstrip_
>
> Thoughts?

I like aliasing and checks in TabDragController. That way we'll know
for sure if we're in a bad state and won't try to call out with bogus
values.

  -Scott

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-03-02 00:19:39]

Another possibility is the TabDragController is getting destroyed
earlier, so that by the time we try to use members from
TabDragController it's bogus. I recommend adding checks for this too.

  -Scott

On Tue, Mar 1, 2016 at 4:18 PM, Scott Violet <sky@chromium.org> wrote:
> On Tue, Mar 1, 2016 at 1:39 PM,  <jonross@chromium.org> wrote:
>> On 2016/02/24 20:41:41, sky wrote:
>>> On Wed, Feb 24, 2016 at 8:52 AM, <mailto:jonross@chromium.org> wrote:
>>> > On 2016/02/24 16:41:47, sky wrote:
>>> >> I don't think this is it either. TabDragController::attached_tabstrip_
>>> >> owns
>>> > the
>>> >> TabDragController.
>>> >
>>> > True, which is why I would expect TabDragController to be destroyed when
>>> > attached_tabstrip_ is.
>>> >
>>> > From the dump TabStrip::ContinueDrag appears to be called from a
>>> > TabStrip
>>> > other
>>> > than attached_tabstrip_/source_tabstrip_.
>>>
>>> What makes you think that? Even if that were the case, that doesn't
>>> explain the crash.
>>
>> Different pointer values for the tabstrip calling ContinueDrag vs those of
>> attached_tabstrip_/source_tabstrip_ in the dump.
>
> Are you sure the values are good? You can't always trust the data in a
> dump. If you want to definitively know you need to use
> base::Debug::Alias and put the values on the stack.
>
>>
>>>
>>> If other->controller() is bogus, then that means it's the other window
>>> that is in a bad state, not the one being dragged. The other window
>>> here being the window we're attempting to attach to.
>>>
>>> -Scott
>>>
>>
>> True. I could instead add a check in TabDragController::GetTabStripForWindow
>> to
>> verify that other_tabstrip->controller() is valid to confirm that.
>>
>> Additionally I could update IsCompatibleWith to verify there that
>> other->controller() is valid. Which would check if there is an issue with
>> attached_tabstrip_/source_tabstrip_
>>
>> Thoughts?
>
> I like aliasing and checks in TabDragController. That way we'll know
> for sure if we're in a bad state and won't try to call out with bogus
> values.
>
>   -Scott

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jonross, 2016-03-03 18:54:10]

Description was changed from

==========
Have TabDragController observe TabStrip destruction

Somehow TabStrips are being destroyed while dragging is occurring. The cause is
currently not knonw.

This change has TabDragController observe the TabStrips it is interacting with,
and will crash upon their deletion. This should let us identify how this is
happening.

BUG=585017
==========

to

==========
Have TabDragController perform checks in GetTabStripForWindow, along with
aliasing of pointers, in order to track down the cause of a crash.

BUG=585017
==========

[jonross, 2016-03-03 18:55:15]

Removed the observing. Switched over to some more direct CHECKs within
GetTabStripForWindow.

base::debug::Alias added for these cases to improve information in dumps.

PTAL

[sky, 2016-03-03 20:57:46]

LGTM

[jonross, 2016-03-03 21:03:30]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2016-03-03 21:04:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1726743003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1726743003/20001

[commit-bot: I haz the power, 2016-03-03 21:55:31]

Description was changed from

==========
Have TabDragController perform checks in GetTabStripForWindow, along with
aliasing of pointers, in order to track down the cause of a crash.

BUG=585017
==========

to

==========
Have TabDragController perform checks in GetTabStripForWindow, along with
aliasing of pointers, in order to track down the cause of a crash.

BUG=585017
==========

[commit-bot: I haz the power, 2016-03-03 21:55:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-03 21:56:57]

Description was changed from

==========
Have TabDragController perform checks in GetTabStripForWindow, along with
aliasing of pointers, in order to track down the cause of a crash.

BUG=585017
==========

to

==========
Have TabDragController perform checks in GetTabStripForWindow, along with
aliasing of pointers, in order to track down the cause of a crash.

BUG=585017

Committed: https://crrev.com/cc9ee3eb04c7cdf3719ec4914b3d14feae80e18c
Cr-Commit-Position: refs/heads/master@{#379102}
==========

[commit-bot: I haz the power, 2016-03-03 21:56:58]

Patchset 2 (id:??) landed as
https://crrev.com/cc9ee3eb04c7cdf3719ec4914b3d14feae80e18c
Cr-Commit-Position: refs/heads/master@{#379102}