CRASH at Tab::OnMouseReleased...

CRASH at Tab::OnMouseReleased

Some strange thing is happening that it crashes for view::HitTest, it can never
ENTER that function. It seems that the tab is being destroyed and as pkasting
stated on IRC: 
"Part of this may be because our retarded Views designs can't distinguish which
buttons are being held versus clicked in these sorts of cases (I filed a bug on
this about a year and a half ago, internally)"

When a tab has ended dragging (EndDrag), if something was being dragged and you
end it, it actually cleans the TabDelegate and assigns its value to 'freed
memory' which is 0xfeeefeee. Therefore it crashes while dragging, because the
object no longer exists. I couldn't do if (delegate_) cause that always returns
true since delegate has 0xfeeefeee. 

So I just changed the return type from void to bool for underlying EndDragImpl
and pumped it to tab.cc. That way, we can know if a tab is destroyed or not.

BUG=5819
(http://crbug.com/5819)

TEST=Dragging tabs around, closing while dragging, and closing tabs.

[Peter Kasting, 2008-12-24 03:28:28]

Let me see if I have this straight.  The issue is that calling
delegate_->EndDrag() may delete |delegate_|, in which case the call to it just
below will crash?

Perhaps a better fix than adding IsTabDrag() would be for EndDrag() to return
whether a drag was actually ended?  I dunno whether it should also not delete
the delegate (and instead rely on the caller to do so)... it probably depends on
the callers.  In either case, we can then just bail out of the click handler
when that succeeds.

[Mohamed Mansour (USE mhm), 2008-12-24 03:55:01]

Sorry if I wasn't clear.

On 2008/12/24 03:28:28, pkasting wrote:
> Let me see if I have this straight.  The issue is that calling
> delegate_->EndDrag() may delete |delegate_|, in which case the call to it just
> below will crash?

The original issue was that it will NEVER execute HitTest function, it crashes
right there at the spot. The work around is to check you are currently dragging
the tab. The solution I proposed was to expose IsTabDrag from the TabDelegate.

Now ... I was going to do the following, if(tabisnotdragging &&
middlemousebutton && hittest) closetab,  BUT I cannot do that, becuase the
delegate_->EndDrag() deleted |delegate_|, so my if statement crashed as well. So
what I did instead was to wrap the EndDrag with the IsTabDrag and return on the
spot.

> Perhaps a better fix than adding IsTabDrag() would be for EndDrag() to return
> whether a drag was actually ended?  I dunno whether it should also not delete
> the delegate (and instead rely on the caller to do so)... it probably depends
on
> the callers.  In either case, we can then just bail out of the click handler
> when that succeeds.
Would you rather let EndDrag() return a boolean? instead of a void? And if I
understand correctly, if that returns false, we then can close the tab and get
rid of IsTabDrag?

Either way works fine. You have the final word

[Peter Kasting, 2008-12-24 04:08:42]

On 2008/12/24 03:55:01, Mohamed Mansour wrote:
> On 2008/12/24 03:28:28, pkasting wrote:
> > Let me see if I have this straight.  The issue is that calling
> > delegate_->EndDrag() may delete |delegate_|, in which case the call to it
just
> > below will crash?
> 
> The original issue was that it will NEVER execute HitTest function, it crashes
> right there at the spot.

I don't understand this sentence.  It crashes right where at what spot?

Your original message implied that you have a bogus callstack and that part of
the issue is that you're calling member functions on a deleted object.  Is that
not what's actually happening?  _Something_ certainly sounds like stack or heap
corruption.  But Perhaps I am misinterpreting your original email.  I wasn't
sure on it, which is why I wrote this attempt at clarification.

If you can't get to the point where you understand what's going wrong, I
strongly advise not bandaiding it :)

[Mohamed Mansour (USE mhm), 2008-12-24 04:29:18]

On 2008/12/24 04:08:42, pkasting wrote:
> I don't understand this sentence.  It crashes right where at what spot?

Peter, I have never seen that before, the original crash was when it trying to
enter the HitTest function call when I initiate a drag. But it never enters
HitTest, it just crashes. 

So for this line:
if (event.IsMiddleMouseButton() && HitTest(event.location()))

IsMiddleMouseButton executes correctly => true, then it tries to call HitTest,
but it crashes. It is like, there is no such thing as function name HitTest :x 

That was the original problem for the crash. Maybe because there is no view when
you drag? I "really" don't know, if you can help me understand why it's crashing
for that part, it would be so helpful for me and I could learn a lot :(


> Your original message implied that you have a bogus callstack and that part of
> the issue is that you're calling member functions on a deleted object.  Is
that
> not what's actually happening?  _Something_ certainly sounds like stack or
heap
> corruption.  But Perhaps I am misinterpreting your original email.  I wasn't
> sure on it, which is why I wrote this attempt at clarification.
> 

My first attempt was to do this:
if (delegate_->IsTabDrag() && event.IsMiddleMouseButton() &&
HitTest(event.location()))

You suggested not to close tab when it is dragging, so that is what I did
initially. BUT then I realized that I was calling member functions on a deleted
object because EndDrag deleted the delegate. Maybe because the drag has been
completed.

> If you can't get to the point where you understand what's going wrong, I
> strongly advise not bandaiding it :)
I am not band-aiding it :) That is the solution I thought that would fix this
bug, I went through the trouble trying to figure out why the bloody function
HitTest does not exist while dragging (I couldn't figure it out0). Then I wanted
to check if a tab is being dragged, BUT  I realized that I cannot check after
the drag ended or it will Crash since the delegate has been deleted and calling
a member function from a deleted delegate is a no no. So I did the check at the
beginning.

I am helping out Chromium so I can help make it a better product, and in the
same time I could learn from the experts like you :)

[Peter Kasting, 2008-12-24 17:24:47]

On 2008/12/24 04:29:18, Mohamed Mansour wrote:
> the original crash was when it trying to
> enter the HitTest function call when I initiate a drag. But it never enters
> HitTest, it just crashes. 
> 
> So for this line:
> if (event.IsMiddleMouseButton() && HitTest(event.location()))
> 
> IsMiddleMouseButton executes correctly => true, then it tries to call HitTest,
> but it crashes. It is like, there is no such thing as function name HitTest :x


Crashing trying to call member functions or access member variables is usually a
sign that your object has been deleted.  After calling EndDrag(), take a look at
your object and see if its contents are trashed and/or |this| == NULL.

If so, we should first figure out if that's what _should_ happen; if yes, then
we should add a comment here warning people that EndDrag() may delete |this|,
and write code that bails if so (probably the "make EndDrag() return a bool"
idea I suggested); if no, then obviously we should fix that.

[Mohamed Mansour (USE mhm), 2008-12-25 01:47:49]

On 2008/12/24 17:24:47, pkasting wrote:
> Crashing trying to call member functions or access member variables is usually
a
> sign that your object has been deleted.  After calling EndDrag(), take a look
at
> your object and see if its contents are trashed and/or |this| == NULL.

Ah thanks Peter for this explanation :) I have checked |this| object, and its
not NULL, it is about to close. All the member variables are set to 0xfeefee
except closing_ which is set to true. That is what I need. I updated the patch
which only places !closing() if condition.

I believe this is sufficient, cause |closing_|  only returns true if EndDrag has
been successful. So if you look here: DraggedTabController::EndDragImpl, it
tries to destroy and clean up. I don't understand what happens once a drag
finishes and why it tries to destroy the object, but I guess it solves the
current crash.



> If so, we should first figure out if that's what _should_ happen; if yes, then
> we should add a comment here warning people that EndDrag() may delete |this|,
> and write code that bails if so (probably the "make EndDrag() return a bool"
> idea I suggested); if no, then obviously we should fix that.

All EndDrag does is cleaning up and destroying the object. We could return a
bool from |destroy_now| which itself is a boolean. Then we don't have to use the
closing() member function.

[Peter Kasting, 2008-12-26 06:36:22]

It might be good to find who worked on the tab dragging implementation and throw
this their way for review.

http://codereview.chromium.org/16246/diff/213/7
File chrome/browser/views/tabs/tab.cc (right):

http://codereview.chromium.org/16246/diff/213/7#newcode188
Line 188: // dragging tab is set to be closing.
Nit: This new comment is longer and more confusing than it needs to be.

I suggest leaving this whole block totally unchanged, and instead adding an "if
(closing()) return;" (on two lines) right after the EndDrag() call, with a
comment like "// In some cases, ending the drag will schedule the tab for
destruction; if so, bail immediately, since our members are already dead and we
shouldn't do anything else."  Except you should make that more or less
definitive depending on whether EndDrag() always destroys all Tabs, or just some
Tabs (e.g. tabs not currently in a TabStrip).

[sky, 2009-01-05 22:24:55]

http://codereview.chromium.org/16246/diff/604/220
File chrome/browser/views/tabs/tab.cc (right):

http://codereview.chromium.org/16246/diff/604/220#newcode186
Line 186: if (closing())
If the tab can be destroyed when invoking EndDrag (which it can be), then you
shouldn't access any fields, you'll just get random data which will some times
be true and sometimes be false. One solution is to calculate whether the tab
should be closed first, then pass that value into the EndDrag method. That way
the last thing this method does is invoke EndDrag.

[Peter Kasting, 2009-01-05 23:24:24]

On 2009/01/05 22:24:55, sky wrote:
> If the tab can be destroyed when invoking EndDrag (which it can be), then you
> shouldn't access any fields, you'll just get random data which will some times
> be true and sometimes be false.

Wow, I feel stupid for having suggested this, you're obviously correct.

I think either sky's method, or having EndDrag() return whether the tab was
closed, would be OK, as long as the logic all lives in one place.

[Mohamed Mansour (USE mhm), 2009-01-06 04:36:04]

Alright! I have updated the patch again, according to what sky said on irc:
"<sky__> Sure. Once an object is deleted the memory it used can be used by
another object. closing() just checks the field closed_, which means if Tab is
deleted you're accessing memory another object may be using. closed_ values is
totally random then." Made sense to me to approach it the following way.

I just made EndDrag return true if it is dragging. I tested it and it works.

If you want me to make all the underlying calls to bool for EndDragImpl, etc, I
don't think that is right. Doesn't make sense to return true/false based if the
tab is destroyed or not. 

This way seems to work and the bug is fixed. Any nits peter? ;p

[Peter Kasting, 2009-01-06 18:10:22]

http://codereview.chromium.org/16246/diff/222/607
File chrome/browser/views/tabs/tab.cc (right):

http://codereview.chromium.org/16246/diff/222/607#newcode181
Line 181: // In some cases, ending the drag will schedule the tab for
destruction; if
This comment is a little misleading in that it sounds as if EndDrag() returns
true only when the tab will be destroyed (and that sometimes we can EndDrag()
without the tab being destroyed), yet EndDrag() actually returns true any time
there was a drag.

It seems like there are two possibilities.
(1) Every time there was a drag, the tab will be destroyed.  If this is the
case, remove "In some cases" on the above comment.
(2) The tab might not be destroyed on drag end.  If this is the case, we should
probably change EndDrag() to only return true when the tab is actually
destroyed, which will require more plumbing.

In either case, we should also ensure that the resulting behavior at the end of
this is that middle-clicking while dragging either closes the tab or aborts the
drag (I'm not sure which is better), but not that it just completes the drag
(i.e. drops the tab where it is).

http://codereview.chromium.org/16246/diff/222/609
File chrome/browser/views/tabs/tab.h (right):

http://codereview.chromium.org/16246/diff/222/609#newcode68
Line 68: // Ends dragging a Tab. Returns true if successfull. |canceled| is true
if
Nit: Move this to the end of the description and change to "Returns whether a
drag was in progress".

http://codereview.chromium.org/16246/diff/222/609#newcode70
Line 70: virtual bool EndDrag(bool canceled) const = 0;
I wouldn't make this const.  It's physically const, but not really logically
const.

http://codereview.chromium.org/16246/diff/222/610
File chrome/browser/views/tabs/tab_strip.h (right):

http://codereview.chromium.org/16246/diff/222/610#newcode144
Line 144: virtual bool EndDrag(bool canceled) const;
Is this the only Tab::Delegate?  You should search to make sure you're not
missing others (although maybe the compiler would catch this one).

[Avi (use Gerrit), 2009-01-06 18:59:06]

http://codereview.chromium.org/16246/diff/222/609
File chrome/browser/views/tabs/tab.h (right):

http://codereview.chromium.org/16246/diff/222/609#newcode68
Line 68: // Ends dragging a Tab. Returns true if successfull. |canceled| is true
if
Nit: Typo. Successful.

[Mohamed Mansour (USE mhm), 2009-01-06 19:32:34]

http://codereview.chromium.org/16246/diff/222/607
File chrome/browser/views/tabs/tab.cc (right):

http://codereview.chromium.org/16246/diff/222/607#newcode181
Line 181: // In some cases, ending the drag will schedule the tab for
destruction; if
On 2009/01/06 18:10:22, pkasting wrote:
> This comment is a little misleading in that it sounds as if EndDrag() returns
> true only when the tab will be destroyed (and that sometimes we can EndDrag()
> without the tab being destroyed), yet EndDrag() actually returns true any time
> there was a drag.
> 
> It seems like there are two possibilities.
> (1) Every time there was a drag, the tab will be destroyed.  If this is the
> case, remove "In some cases" on the above comment.
> (2) The tab might not be destroyed on drag end.  If this is the case, we
should
> probably change EndDrag() to only return true when the tab is actually
> destroyed, which will require more plumbing.
> 
> In either case, we should also ensure that the resulting behavior at the end
of
> this is that middle-clicking while dragging either closes the tab or aborts
the
> drag (I'm not sure which is better), but not that it just completes the drag
> (i.e. drops the tab where it is).

Done. It is the second possibility. After doing some testing it doesn't destroy
the tab if your dragging along the toolbar, or dragging it to the same frame.
The only time it destroys is if your dragging to a new window or closing it.

I added the comment "and we shouldn't do anything else except drop the tab where
it is." If that is good enough?

http://codereview.chromium.org/16246/diff/222/609
File chrome/browser/views/tabs/tab.h (right):

http://codereview.chromium.org/16246/diff/222/609#newcode68
Line 68: // Ends dragging a Tab. Returns true if successfull. |canceled| is true
if
On 2009/01/06 18:10:22, pkasting wrote:
> Nit: Move this to the end of the description and change to "Returns whether a
> drag was in progress".

Done.

http://codereview.chromium.org/16246/diff/222/609#newcode70
Line 70: virtual bool EndDrag(bool canceled) const = 0;
On 2009/01/06 18:10:22, pkasting wrote:
> I wouldn't make this const.  It's physically const, but not really logically
> const.

Done.

http://codereview.chromium.org/16246/diff/222/610
File chrome/browser/views/tabs/tab_strip.h (right):

http://codereview.chromium.org/16246/diff/222/610#newcode144
Line 144: virtual bool EndDrag(bool canceled) const;
On 2009/01/06 18:10:22, pkasting wrote:
> Is this the only Tab::Delegate?  You should search to make sure you're not
> missing others (although maybe the compiler would catch this one).

Done. Jup, that is the only one, I made sure when I was changing it before. I
removed const as well since its not needed :)

[Peter Kasting, 2009-01-06 20:32:47]

LGTM with spelling nits

http://codereview.chromium.org/16246/diff/1011/243
File chrome/browser/views/tabs/dragged_tab_controller.h (right):

http://codereview.chromium.org/16246/diff/1011/243#newcode60
Line 60: // begun. Returns weather the tab has been destroyed.
Nit: "weather" -> "whether"

http://codereview.chromium.org/16246/diff/1011/243#newcode189
Line 189: // Does the work for EndDrag. Returns weather the tab has been
destroyed.
Same nit (or else don't add comment)

http://codereview.chromium.org/16246/diff/1011/240
File chrome/browser/views/tabs/tab.h (right):

http://codereview.chromium.org/16246/diff/1011/240#newcode69
Line 69: // other than the user releasing the mouse. Returns weather the tab has
been
Nit: "weather" -> "whether"

http://codereview.chromium.org/16246/diff/1011/239
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/16246/diff/1011/239#newcode1026
Line 1026: if (drag_controller_.get())
Nit: I might do

return drag_controller_.get() ? drag_controller_->EndDrag(canceled) : false;

[Mohamed Mansour (USE mhm), 2009-01-06 20:48:33]

Had a feeling that spelling was wrong :/ I will be more careful next time.

http://codereview.chromium.org/16246/diff/1011/243
File chrome/browser/views/tabs/dragged_tab_controller.h (right):

http://codereview.chromium.org/16246/diff/1011/243#newcode60
Line 60: // begun. Returns weather the tab has been destroyed.
On 2009/01/06 20:32:47, pkasting wrote:
> Nit: "weather" -> "whether"

Done.

http://codereview.chromium.org/16246/diff/1011/243#newcode189
Line 189: // Does the work for EndDrag. Returns weather the tab has been
destroyed.
On 2009/01/06 20:32:47, pkasting wrote:
> Same nit (or else don't add comment)

Done.

http://codereview.chromium.org/16246/diff/1011/240
File chrome/browser/views/tabs/tab.h (right):

http://codereview.chromium.org/16246/diff/1011/240#newcode69
Line 69: // other than the user releasing the mouse. Returns weather the tab has
been
On 2009/01/06 20:32:47, pkasting wrote:
> Nit: "weather" -> "whether"

Done.

http://codereview.chromium.org/16246/diff/1011/239
File chrome/browser/views/tabs/tab_strip.cc (right):

http://codereview.chromium.org/16246/diff/1011/239#newcode1026
Line 1026: if (drag_controller_.get())
On 2009/01/06 20:32:47, pkasting wrote:
> Nit: I might do
> 
> return drag_controller_.get() ? drag_controller_->EndDrag(canceled) : false;

Done.