210719 - M17rc1 Trunk [@ nsTreeBodyFrame::ScrollCallback] - Bookmark Manager persistently scrolls up/down if a bookmark is dragged into the 'scroll up'/'scroll down' area just above/below the bookmarks, and dropped there

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[mark.fitzgerald2]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624

When in the Bookmark Manager while it has a scroll bar on the right hand side,
selecting the bottom bookmark with the mouse, dragging it into the 'scroll up'
area between the bookmarks and the title bars, then releasing the bookmark
causes the bookmark list to persistently scroll up.

Reproducible: Always

Steps to Reproduce:
1. Open Bookmark Manager
2. Ensure that a scrollbar is on the right hand side
3. Hover the mouse over the last bookmark
4. Press and hold down the left mouse button in order to begin a 'drag'
operation on the bookmark
5. Drag the bookmark to the area between the top of the bookmarks view and the
bookmarks view title bars, causing the bookmarks view to begin scrolling up
6. Before the bookmarks view finishes scrolling to its top, release the bookmark
Actual Results:  
The bookmarks view continues to scroll up until it reaches the top.  Using the
down arrow or mouse wheel only temporarily moves the view back down; it will
continue scrolling up until it reaches the top again.  However, selecting a
bookmark in the view and dragging it will stop the 'persistent scroll up'
problem.  However, sometimes the browser crashes (a Talkback crash report has
been filed under my email address) when reproducing this bug.

Expected Results:  
Mozilla probably should have abandoned the drag and drop operation; perhaps it
should have instead dropped the bookmark at the top of the current bookmarks view.

Mozilla has crashed once in 1.4 RC2, and at least two other times since 1.2 with
this problem.  I have submitted several Talkback crash reports, a few of which
have my email addresses attached.

Comment 1

[mark.fitzgerald2]

When 1.4 RC2 crashed on this bug, Mozilla had accessed address 0x00000001. 
Aside from that, I do not know which thread or DLL caused the crash, nor do I
have any stack dumps.  If I can reproduce the crashing version of this bug
again, I will try to capture more details and post them as comments.

Comment 2

[mark.fitzgerald2]

Comment #4 in bug 156942, and bug 125375 seem to have points that are related to
this bug, but which do not completely overlap this bug.

Comment 3

[Kevin Brosnan]

I can reproduce the bug on both
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040329
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040329 Firefox/0.8+

While bug 156942 and bug 125375 maybe related neither of them mention crashing
which happens in this bug.

I crashed Mozilla 1.7b twice and submited the talkback data on 2004-03-29 before
20:21 PST in the comment section for talkback I listed bug 216617, a likely
dupe, as the issue.

Comment 4

[Ryuichi KUBUKI]

*** Bug 216617 has been marked as a duplicate of this bug. ***

Comment 5

[mark.fitzgerald2]

I confirm comment #3 by Kevin Brosnan on Mozilla 1.6.  Indeed, closing bookmarks
& browser while the (unstoppable) scrolling is in progress resulted in a crash.
 Perhaps my build does not have talkback -- I didn't get the opportunity to send
in crash info.

Comment 6

[Asa Dotzler [:asa]]

from Kevin's talkback:

nsTreeBodyFrame::ScrollCallback
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,
line 3855]
nsTimerImpl::Fire
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/xpcom/threads/nsTimerImpl.cpp,
line 383]
PL_HandleEvent
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 672]
PL_ProcessPendingEvents
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 610]
nsEventQueueImpl::ProcessPendingEvents
[c:/builds/tinderbox/Mozilla1.7b/WINNT_5.0_Clobber/mozilla/xpcom/threads/nsEventQueue.cpp,
line 395]

Comment 7

[Jon Henry]

Attachment: stack trace

Confirmed in Firefox on WinXP, this is the stack trace I see.

Comment 8

[Frank Wein [:mcsmurf]]

Similar crash was reported in Bug 170249, but that bug has been fixed a long
time ago.

Comment 9

[Anders Pedersen]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4)
Gecko/20030624
> Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4)
Gecko/20030624
> 
> When in the Bookmark Manager while it has a scroll bar on the right hand side,
> selecting the bottom bookmark with the mouse, dragging it into the 'scroll up'
> area between the bookmarks and the title bars, then releasing the bookmark
> causes the bookmark list to persistently scroll up.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. Open Bookmark Manager
> 2. Ensure that a scrollbar is on the right hand side
> 3. Hover the mouse over the last bookmark
> 4. Press and hold down the left mouse button in order to begin a 'drag'
> operation on the bookmark
> 5. Drag the bookmark to the area between the top of the bookmarks view and the
> bookmarks view title bars, causing the bookmarks view to begin scrolling up
> 6. Before the bookmarks view finishes scrolling to its top, release the bookmark
> Actual Results:  
> The bookmarks view continues to scroll up until it reaches the top.  Using the
> down arrow or mouse wheel only temporarily moves the view back down; it will
> continue scrolling up until it reaches the top again.  However, selecting a
> bookmark in the view and dragging it will stop the 'persistent scroll up'
> problem.  However, sometimes the browser crashes (a Talkback crash report has
> been filed under my email address) when reproducing this bug.
> 
> Expected Results:  
> Mozilla probably should have abandoned the drag and drop operation; perhaps it
> should have instead dropped the bookmark at the top of the current bookmarks view.
> 
> Mozilla has crashed once in 1.4 RC2, and at least two other times since 1.2 with
> this problem.  I have submitted several Talkback crash reports, a few of which
> have my email addresses attached.

Confirmed in

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040330
Microsoft Windows 2000 Pro 5.00.2195 SP4

Haven't crashed here yet, though.

Comment 10

[mark.fitzgerald2]

I also confirmed the bug on Mozilla 1.7beta on my work PC, a Pentium 4 Win2k SP2
machine.  However, I could not get Mozilla 1.7beta to crash on that PC.

Also, scrolling down by dropping a bookmark from Bookmark Manager into the
'scroll down' area produces the analagous problem in the opposite scroll
direction.  (I changed the description accordingly.)

Comment 11

[Anders Pedersen]

(In reply to comment #9)
> (In reply to comment #0)
> > User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4)
> Gecko/20030624
> > Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4)
> Gecko/20030624
> > 
> > When in the Bookmark Manager while it has a scroll bar on the right hand side,
> > selecting the bottom bookmark with the mouse, dragging it into the 'scroll up'
> > area between the bookmarks and the title bars, then releasing the bookmark
> > causes the bookmark list to persistently scroll up.
> > 
> > Reproducible: Always
> > 
> > Steps to Reproduce:
> > 1. Open Bookmark Manager
> > 2. Ensure that a scrollbar is on the right hand side
> > 3. Hover the mouse over the last bookmark
> > 4. Press and hold down the left mouse button in order to begin a 'drag'
> > operation on the bookmark
> > 5. Drag the bookmark to the area between the top of the bookmarks view and the
> > bookmarks view title bars, causing the bookmarks view to begin scrolling up
> > 6. Before the bookmarks view finishes scrolling to its top, release the bookmark
> > Actual Results:  
> > The bookmarks view continues to scroll up until it reaches the top.  Using the
> > down arrow or mouse wheel only temporarily moves the view back down; it will
> > continue scrolling up until it reaches the top again.  However, selecting a
> > bookmark in the view and dragging it will stop the 'persistent scroll up'
> > problem.  However, sometimes the browser crashes (a Talkback crash report has
> > been filed under my email address) when reproducing this bug.
> > 
> > Expected Results:  
> > Mozilla probably should have abandoned the drag and drop operation; perhaps it
> > should have instead dropped the bookmark at the top of the current bookmarks
view.
> > 
> > Mozilla has crashed once in 1.4 RC2, and at least two other times since 1.2 with
> > this problem.  I have submitted several Talkback crash reports, a few of which
> > have my email addresses attached.
> 
> Confirmed in
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040330
> Microsoft Windows 2000 Pro 5.00.2195 SP4
> 
> Haven't crashed here yet, though.


Well, it crashed when i clicked "x" to close the bookmark manager while the
autoscrolling upwards still was happening.

talkbackid: TB8797K

Comment 12

[mark.fitzgerald2]

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #0)
> > > [original bug repro steps]
> > 
> > Confirmed in
> > 
> > Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7b) Gecko/20040330
> > Microsoft Windows 2000 Pro 5.00.2195 SP4
> > 
> > Haven't crashed here yet, though.
> 
> Well, it crashed when i clicked "x" to close the bookmark manager while the
> autoscrolling upwards still was happening.
> 
> talkbackid: TB8797K

Nice reproduction idea - thanks!  I've expanded on it below...

I get something as bad as, or perhaps worse than a crash on Mozilla 1.6 on my
home PC (Win2k SP2 on AMD Athlon).  Reproduction steps:
1) drag bookmark to scroll up/down area
2) drop bookmark
3) while bookmarks are scrolling up/down, hit the 'X' in the top right of
   Bookmark Manager

Results:
1) Cannot reopen Bookmark Manager
2) Bookmarks menu in browser, if taller than screen height, does not
   autoscroll.  To scroll it, move mouse on and off the up/down arrow, or
   click on the up/down arrow manually.
3) After closing all Mozilla windows, a mozilla.exe process lingers, doing
   nothing obvious, and chewing 0% CPU.  The existence of this process,
   however, prevents Mozilla from being reopened.  Workaround: kill the errant
   mozilla.exe, then start a fresh Mozilla process.

Comment 13

[mark.fitzgerald2]

This is probably the same bug as 207343, which Anders filed a month before
210719.  I'll try to reproduce 207343 and see if it 'feels' the same once I have
another free moment.  :)

Comment 14

[Anders Pedersen]

(In reply to comment #13)
> This is probably the same bug as 207343, which Anders filed a month before
> 210719.  I'll try to reproduce 207343 and see if it 'feels' the same once I have
> another free moment.  :)

Yeah, was just about to mention that one.. Extremely similar :)

Comment 15

[Ryuichi KUBUKI]

(In reply to comment #13)
> This is probably the same bug as 207343, which Anders filed a month before
> 210719.  I'll try to reproduce 207343 and see if it 'feels' the same once I have
> another free moment.  :)

Bug 207343 is for bookmark sidebar, and this bug 210719 is about bookmark
manager. By the way, I'm from duped Bug 216617, and the fact that apparently it
got suddenly responsive for these bugs after 1 year is very amusing :-)

Comment 16

[Andreas Kunz]

I think I also saw this in MailNews a few times. Somehow endless scrolling began
and Mozilla crashed as soon as I closed the window. Unfortunately no stack from it.
So this looks not like separate bookmarks/sidebar/etc. bugs to me, but more like
a scrollbar/tree bug. 

Comment 17

[Andreas Kunz]

The problem seems to be that when you cancel the drag by an unusual action (by
pressing ESC as in bug 207343 or releasing where it cannot be dropped),
nsTreeBodyFrame::OnDragExit is not called, so CloseCallback is not called and
the scrolling timer keeps on firing.

On the other hand, nsTreeBodyFrame::ScrollCallback has a mechanism in place to
stop scrolling even in this case, it stops scrolling when arrived at the bottom:
self->CanAutoScroll(self->mDropRow)
should get false then and the timer is cancelled.

However, CanAutoScroll still returns "true" although scrolling has obviously
arrived at the bottom:
if (aRowIndex > 0 && aRowIndex < mRowCount - 1)
evaluates to "true" because aRowIndex is 333 (in my case) and (mRowCount - 1) is
355, so it assumes it can still scroll on.

The problem is that aRowIndex (= mDropRow) does not reflect the current drop
position when the drop action was aborted unexpectedly.
Regular OnDragExit would have set "mDropRow = -1;", thus stopping the infinite
scrolling. Dragging on without pressing ESC (or whatever aborts dragging
irregularly) would cause OnDragOver to be called, which in turn re-computes
mDropRow by calling ComputeDropPosition.

Bug 125301 is about ESC not triggering OnDragExit and basically is the cause for
bug 207343.
This bug, however, is also caused by OnDragExit not being called, but bug 125301
obviously will not fix it. So after all, they might be no duplicates (apart from
when a solution is found within tree scrolling code).

I think that's all I can do here.

Comment 18

[Jan Varga [:janv]]

Yeah, not calling onDragExit can cause various problems.

Comment 19

[Jay Patel [:jay]]

Seems like we have a good idea of what's happening here...and solid set of steps
to reproduce.  Marking topcrash+.  What's the chance of getting a fix for 1.7?

Comment 20

[Andreas Kunz]

Attachment: patch

Cancel the timer in the destructor of class Slots (created using ideas by
Mnyromyr).
This *does* prevent the crash without harming anything.
However, this is my first try for a crash fix, so I'm not sure at all if this
is something we should do, if the placement of the destructor in
nsTreeBodyFrame.h is ok and if this is to be considered a wallpaper or a real
fix.

Comment 21

[Jan Varga [:janv]]

I think it's a good solution. r=varga

Comment 22

[Andreas Kunz]

Comment on attachment 148420 [details] [diff] [review]
patch

Requesting sr= from shaver.

Comment 23

[Mike Shaver (:shaver emeritus)]

Comment on attachment 148420 [details] [diff] [review]
patch

sr=shaver.  (Please mark r= in the patch-flags!)

Comment 24

[Andreas Kunz]

Comment on attachment 148420 [details] [diff] [review]
patch

Requesting approval for 1.7. This is low risk and fixes a topcrash.

Comment 25

[chris hofmann]

Comment on attachment 148420 [details] [diff] [review]
patch

a=chofmann for 1.7

Comment 26

[Andreas Kunz]

Fix checked into trunk at 2004-05-13 11:06 by timeless.

This patch does _not_ fix the (almost) unstoppable scrolling, but that is bug
125301 (I'll add this effect to its summary to avoid dupes). 
You're invited to verify that this patch fixes the crash using tomorrow's nightlies.

Unfortunately this patch does not fit on the branch because the tree refactoring
from bug 221619 did not happen there! I'll try to find an equivalent patch, but
cannot promise anything.

Comment 27

[Andreas Kunz]

Attachment: patch for branch

This is probably the patch for the 1.7 branch; it now resides directly in the
nsTreeBodyFrame destructor.
I haven't built this patch yet because I have no branch tree.

Comment 28

[Andreas Kunz]

Comment on attachment 148454 [details] [diff] [review]
patch for branch

Ok, I just built a Mozilla tree downloaded from
ftp://ftp.mozilla.org/pub/mozilla.org/mozilla/nightly/2004-04-13-07-trunk/ with
this patch and it builds fine and the crash is fixed.
2004-04-13-07-trunk is about a day before the 1.7 branch was cut and a few days
before the tree refactoring went in.
Please feel free to check this in as soon as it has all the reviews.

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 148454 [details] [diff] [review]
patch for branch

a=asa (on behalf of drivers) for checkin to 1.7

Comment 30

[Andreas Kunz]

Fixed. And talkback also doesn't show any more incidents.

Comment 31

[David Epstein]

Verified on the 1.7 branch. No crashing from drag operation. I was able to
reproduce the continual herky-jerky upward scrolling, but rarely.

Comment 32

[Ryuichi KUBUKI]

(In reply to comment #31)
> Verified on the 1.7 branch. No crashing from drag operation. I was able to
> reproduce the continual herky-jerky upward scrolling, but rarely.

I could reproduce it on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.7.3) Gecko/20040923 Firefox/0.10, but I could close & reopen the bookmark
manager to remove persistent scroll-up without crash. But persistent
scroll-up/down out of controll is not fixed at all, it should be reopened &
fixed IMHO.

Comment 33

[Jan Varga [:janv]]

I think it has something to do with the bug on windows, when the drag session is
not correctly cancelled after pressing the ESC key.

Comment 34

[Andreas Kunz]

(In reply to comment #32)
> it should be reopened

Well, this bug is (at least now) a critical topcrash bug (you can also see that
in the subject where the crash location is indicated:
"[@nsTreeBodyFrame::ScrollCallback]"). So reopening would mean it crashes again.
In comment #17 I explained which combination of other bugs cause this one, so
_they_ should be fixed, there isn't really an additional or separate bug causing
the infinite scrolling.
However, currently there doesn't seem to exist an open bug mentioning the
scrolling in its subject, so it might be difficult to find the other ones and
dupe possible new bug reports...

Comment 35

[Ryuichi KUBUKI]

(In reply to comment #34)
> (In reply to comment #32)
> > it should be reopened
> 
> Well, this bug is (at least now) a critical topcrash bug (you can also see that
> in the subject where the crash location is indicated:
> "[@nsTreeBodyFrame::ScrollCallback]"). So reopening would mean it crashes again.
> In comment #17 I explained which combination of other bugs cause this one, so
> _they_ should be fixed, there isn't really an additional or separate bug causing
> the infinite scrolling.
> However, currently there doesn't seem to exist an open bug mentioning the
> scrolling in its subject, so it might be difficult to find the other ones and
> dupe possible new bug reports...

Thanks, then bug 125301 is what we were talking about.