Attempt at fixing crash. The latest crash indicates the user is...

Nukes TabStrip::CanUpdateDisplay. This appears to be the culprit for a
couple of crashes we're getting.

BUG=24132, 13707
TEST=thorougly test tab dragging on windows and make sure there are no
regressions.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30193

[Ben Goodger (Google), 2009-10-24 01:50:14]

By eyeballing the code can you see where it doesn't get updated in this
condition? I actually think that's wrong.
-Ben

On Fri, Oct 23, 2009 at 5:34 PM, <sky@chromium.org> wrote:

> Reviewers: Ben Goodger,
>
> Description:
> Attempt at fixing crash. The latest crash indicates the user is
> dragging tab 7, the tab strip has 8 tabs, but the model is empty. It
> looks like if the model is closing all the tab strip doesn't update
> itself, which could lead to this situation. I'm going to leave the
> debugging information in for another round before claiming this fixed.
>
> BUG=24132
> TEST=none
>
>
> Please review this at http://codereview.chromium.org/337013
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/views/tabs/tab_strip.cc
>
>
> Index: chrome/browser/views/tabs/tab_strip.cc
> ===================================================================
> --- chrome/browser/views/tabs/tab_strip.cc      (revision 29776)
> +++ chrome/browser/views/tabs/tab_strip.cc      (working copy)
> @@ -1192,6 +1192,12 @@
>   // the user is dragging.
>   if (IsAnimating() || tab->closing() || !HasAvailableDragActions())
>     return;
> +  if (model_->closing_all()) {
> +    // If the model is closing all the tabs, then we don't want to start a
> drag.
> +    // Otherwise the tab may close part way through the drag, or the tab
> may
> +    // not even be valid anymore.
> +    return;
> +  }
>   int index = GetIndexOfTab(tab);
>   if (!model_->ContainsIndex(index)) {
>     // It appears to be possible for a drag to start with an invalid tab.
>
>
>

[Ben Goodger (Google), 2009-10-24 03:20:51]

This fix is wrong. I know how to reproduce this bug.

On Fri, Oct 23, 2009 at 6:49 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> By eyeballing the code can you see where it doesn't get updated in this
> condition? I actually think that's wrong.
> -Ben
>
>
> On Fri, Oct 23, 2009 at 5:34 PM, <sky@chromium.org> wrote:
>
>> Reviewers: Ben Goodger,
>>
>> Description:
>> Attempt at fixing crash. The latest crash indicates the user is
>> dragging tab 7, the tab strip has 8 tabs, but the model is empty. It
>> looks like if the model is closing all the tab strip doesn't update
>> itself, which could lead to this situation. I'm going to leave the
>> debugging information in for another round before claiming this fixed.
>>
>> BUG=24132
>> TEST=none
>>
>>
>> Please review this at http://codereview.chromium.org/337013
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>>  M     chrome/browser/views/tabs/tab_strip.cc
>>
>>
>> Index: chrome/browser/views/tabs/tab_strip.cc
>> ===================================================================
>> --- chrome/browser/views/tabs/tab_strip.cc      (revision 29776)
>> +++ chrome/browser/views/tabs/tab_strip.cc      (working copy)
>> @@ -1192,6 +1192,12 @@
>>   // the user is dragging.
>>   if (IsAnimating() || tab->closing() || !HasAvailableDragActions())
>>     return;
>> +  if (model_->closing_all()) {
>> +    // If the model is closing all the tabs, then we don't want to start
>> a drag.
>> +    // Otherwise the tab may close part way through the drag, or the tab
>> may
>> +    // not even be valid anymore.
>> +    return;
>> +  }
>>   int index = GetIndexOfTab(tab);
>>   if (!model_->ContainsIndex(index)) {
>>     // It appears to be possible for a drag to start with an invalid tab.
>>
>>
>>
>

[Ben Goodger (Google), 2009-10-24 03:29:11]

This bug is 13707.
We should probably disable all key activated commands while dragging.

-Ben

On Fri, Oct 23, 2009 at 8:20 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> This fix is wrong. I know how to reproduce this bug.
>
>
> On Fri, Oct 23, 2009 at 6:49 PM, Ben Goodger (Google) <ben@chromium.org>wrote:
>
>> By eyeballing the code can you see where it doesn't get updated in this
>> condition? I actually think that's wrong.
>> -Ben
>>
>>
>> On Fri, Oct 23, 2009 at 5:34 PM, <sky@chromium.org> wrote:
>>
>>> Reviewers: Ben Goodger,
>>>
>>> Description:
>>> Attempt at fixing crash. The latest crash indicates the user is
>>> dragging tab 7, the tab strip has 8 tabs, but the model is empty. It
>>> looks like if the model is closing all the tab strip doesn't update
>>> itself, which could lead to this situation. I'm going to leave the
>>> debugging information in for another round before claiming this fixed.
>>>
>>> BUG=24132
>>> TEST=none
>>>
>>>
>>> Please review this at http://codereview.chromium.org/337013
>>>
>>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>>
>>> Affected files:
>>>  M     chrome/browser/views/tabs/tab_strip.cc
>>>
>>>
>>> Index: chrome/browser/views/tabs/tab_strip.cc
>>> ===================================================================
>>> --- chrome/browser/views/tabs/tab_strip.cc      (revision 29776)
>>> +++ chrome/browser/views/tabs/tab_strip.cc      (working copy)
>>> @@ -1192,6 +1192,12 @@
>>>   // the user is dragging.
>>>   if (IsAnimating() || tab->closing() || !HasAvailableDragActions())
>>>     return;
>>> +  if (model_->closing_all()) {
>>> +    // If the model is closing all the tabs, then we don't want to start
>>> a drag.
>>> +    // Otherwise the tab may close part way through the drag, or the tab
>>> may
>>> +    // not even be valid anymore.
>>> +    return;
>>> +  }
>>>   int index = GetIndexOfTab(tab);
>>>   if (!model_->ContainsIndex(index)) {
>>>     // It appears to be possible for a drag to start with an invalid tab.
>>>
>>>
>>>
>>
>

[sky, 2009-10-26 16:36:43]

Yes. If CanUpdateDisplay returns false, which it does if closing all,
then the tab strip is not updated on a detach. This appears
problematic for two reasons:

. Unload handlers make it so we invoke CloseAllTabs (which sets
closing_all_) while the frame is still visible and it appears possible
for events to get in the stream before we actually hide the frame.
. TabStripModel::InsertTabContentsAt sets closing_all_ to false. This
means that if closing_all_ was true, the tab strip was out of sync
with the model, and likely things are completely screwed.

I'm not sure which scenario is causing this crash. I suspect the first one.

I don't think this is related to doing a keyboard gesture while doing
a drag as this appears to happen when the drag is being initiated.

It seems like when we invoke CloseAllTabs we had better hide the
window, and explicitly disallow inserting tab contents when
closing_all_ is true. What do you think?

Feel free to stop by and chat about this one when you get in.

  -Scott

On Fri, Oct 23, 2009 at 6:49 PM, Ben Goodger (Google) <ben@chromium.org> wr=
ote:
> By eyeballing the code can you see where it doesn't get updated in this
> condition? I actually think that's wrong.
> -Ben
>
> On Fri, Oct 23, 2009 at 5:34 PM, <sky@chromium.org> wrote:
>>
>> Reviewers: Ben Goodger,
>>
>> Description:
>> Attempt at fixing crash. The latest crash indicates the user is
>> dragging tab 7, the tab strip has 8 tabs, but the model is empty. It
>> looks like if the model is closing all the tab strip doesn't update
>> itself, which could lead to this situation. I'm going to leave the
>> debugging information in for another round before claiming this fixed.
>>
>> BUG=3D24132
>> TEST=3Dnone
>>
>>
>> Please review this at http://codereview.chromium.org/337013
>>
>> SVN Base: svn://chrome-svn/chrome/trunk/src/
>>
>> Affected files:
>> =A0M =A0 =A0 chrome/browser/views/tabs/tab_strip.cc
>>
>>
>> Index: chrome/browser/views/tabs/tab_strip.cc
>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>> --- chrome/browser/views/tabs/tab_strip.cc =A0 =A0 =A0(revision 29776)
>> +++ chrome/browser/views/tabs/tab_strip.cc =A0 =A0 =A0(working copy)
>> @@ -1192,6 +1192,12 @@
>> =A0 // the user is dragging.
>> =A0 if (IsAnimating() || tab->closing() || !HasAvailableDragActions())
>> =A0 =A0 return;
>> + =A0if (model_->closing_all()) {
>> + =A0 =A0// If the model is closing all the tabs, then we don't want to =
start
>> a drag.
>> + =A0 =A0// Otherwise the tab may close part way through the drag, or th=
e tab
>> may
>> + =A0 =A0// not even be valid anymore.
>> + =A0 =A0return;
>> + =A0}
>> =A0 int index =3D GetIndexOfTab(tab);
>> =A0 if (!model_->ContainsIndex(index)) {
>> =A0 =A0 // It appears to be possible for a drag to start with an invalid=
 tab.
>>
>>
>
>

[Ben Goodger (Google), 2009-10-27 01:13:30]

LGTM