305825 - Rapid Right then Left Click in Scroll Bar Crashes [@nsLayoutUtils::TranslateWidgetToView]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jerry Baker]

20050823 trunk (Firefox)

I do not believe this happened in earlier builds (<20050821)

Sometimes a rapid right and then left click in the scroll gutter causes a crash.
See the following Talkbacks for stack:

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB8685496Q
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB8685442Z
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB8685423Z

Comment 1

[Hermann Schwab]

looking at the Talkbacks TB8685496Q TB8685442Z TB8685423Z maybe fallout from 
Bug 296036 Fix event coordinate handling

Stack signature:
nsLayoutUtils::TranslateWidgetToView 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsLayoutUtils.cpp,
line 490]
nsLayoutUtils::GetEventCoordinatesRelativeTo 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsLayoutUtils.cpp,
line 457]
nsSliderFrame::HandlePress 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSliderFrame.cpp,
line 954]
nsFrame::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/generic/nsFrame.cpp,
line 980]
nsSliderFrame::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSliderFrame.cpp,
line 575]
PresShell::HandleEventInternal 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6264]
PresShell::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6055]
nsViewManager::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp,
line 2553]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp,
line 2245]
HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/view/src/nsView.cpp, line
174]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1061]
nsWindow::DispatchMouseEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 5805]
ChildWindow::DispatchMouseEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 6056]
nsWindow::WindowProc 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1243]
USER32.dll + 0x8734 (0x77d48734)
USER32.dll + 0x8816 (0x77d48816)
USER32.dll + 0x89cd (0x77d489cd)
USER32.dll + 0x8a10 (0x77d48a10)
nsAppShell::Run 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsAppShell.cpp,
line 159]
nsAppStartup::Run 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 146]
main 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/browser/app/nsBrowserApp.cpp,
line 61]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 2

[Martijn Wargers (dead)]

I crashes two times, with this backtrace. I suspect it is a regression from bug
296036.

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase

This testcase crashes 100% reliably for me when following the steps in the
testcase.

Doesn't crash with 2005-08-22 trunk build, crashes with 2005-08-23 trunk build,
so very likely a regression from bug 296036.

Comment 4

[Hermann Schwab]

(In reply to comment #3)
> Created an attachment (id=194856) [edit]
> testcase
> 
> This testcase crashes 100% reliably for me when following the steps in the
> testcase.

no crash and no action seen using
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050904 Firefox/1.6a1

wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050813 SeaMonkey/1.0a

On the 2nd click background for the word 'text' gets white, moving out it gets
red again, moving in it gets white again, all moving done with mousekey still
pressed. 

Comment 5

[Eli Friedman]

The reason for the crash is that the view for the event's widget is destroyed
before we are finished processing the event; I think that it's a bug when that
happens.  In the testcase, the destruction occurs at
http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#1595
when we reflow before we've finished processing the event.

It seems a bit strange to be doing that.  I've been trying to figure out why
that code is there.  It seems this code was checked in with Bug 56167 but it's
not mentioned in that bug.  The code was updated a couple times since then in
bug 36849 and Bug 244366.  Maybe someone who knows more about this code could
comment.

Anyway, I tried removing that call to FlushPendingEvents, and I don't see any
problems.

I haven't reproduced the crash in nsSliderFrame yet; I think it's only loosely
related.  I'll try to figure out what's going on with that one.

Another possibility that should fix all crashes in TranslateWidgetToView would
be to require all events to be fired using coordinates relative to a root
widget.  This would fix the problem because the root view can't disappear the
same way a view in a document can.  I guess we should do that at some point
anyway.  I'm not sure what sorts of regressions changing that might cause.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Another option, which might be the best one if it works, is for nsEvent to hold
a strong reference to the widget.

Comment 7

[Eli Friedman]

Sorry if I didn't make it clear; the view is destroyed, leaving the widget
without a view.  Therefore, since there is no view associated with the widget,
the call to GetViewFor returns null, so a null pointer dereference occurs.  We
could aviod crashing by checking for that in TranslateWidgetToView, but that
wouldn't give us valid coordinates.  I was exploring solutions that would allow
TranslateWidgetToView to return the correct coordinates.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think in general it is possible to have events lying around when the
presentation is gone, so we should have a fallback for that worst case.

There's also the problem of what happens if there's a reflow or scroll event
between the creation of a mouse event and when someone looks at it ... should
the coordinates reflect the old positions of elements, or the new positions.

For the not-worst-case it makes sense to make things relative to a root widget,
at least.

Comment 9

[Eli Friedman]

Attachment: Bulletproofing patch

This patch prevents crashing in TranslateWidgetToView.

Comment 10

[Eli Friedman]

Would you mind checking this in?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in. We need to see about you getting a CVS account ... assuming you want
one?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

closing, although we need to think about what the coordinates should be in this
situation.