248407 - Crash when mouse-dragging scrollbar-thumb to beginning with scrollbar[curpos="0"] thumb{display:none;} in css [@nsSliderFrame::HandleEvent]

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040611 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040611 Firefox/0.8.0+

See upcoming testcase. 
This is a xul window, with a scrollbar.
Dragging the scrollbar with the mouse to the beginning makes the browser crash.
This happens because I have this in the stylesheet:
scrollbar[curpos="0"] thumb{display:none;}

Reproducible: Always
Steps to Reproduce:
1. See details
2.
3.

Actual Results:  
Crash

Expected Results:  
No crash

Comment 1

[Martijn Wargers (dead)]

Attachment: Drag the scrollbar thumb to the top to get the crash

Oh forgot. This is a talkback ID: TB162650W.
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB162650W

Comment 2

[Kenan]

confirming: Talkback: TB171028M

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is almost certainly the usual problem -- attributes being set synchronously
from within layout.

I strongly suspect this is not an issue on trunk due to async processing of
style changes, and I know we have existing bugs on the sync attribute sets...

Comment 4

[Stephen Donner [:stephend] Not actively reading bugmail]

Here's the full stack, and yes, this still does happen on trunk builds, with
build 2004-11-23 on Windows XP, Seamonkey trunk.

nsSliderFrame::HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsSliderFrame.cpp,
line 473]
PresShell::HandleEventInternal 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6007]
PresShell::HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 5844]
nsViewManager::HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2402]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2131]
HandleEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 166]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1078]
nsWindow::DispatchWindowEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1095]
nsWindow::DispatchMouseEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5329]
ChildWindow::DispatchMouseEvent 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5581]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 4023]
nsWindow::WindowProc 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1356]
USER32.dll + 0x8709 (0x77d48709)
USER32.dll + 0x87eb (0x77d487eb)
USER32.dll + 0x89a5 (0x77d489a5)
USER32.dll + 0x89e8 (0x77d489e8)
nsAppShell::Run 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsAppShell.cpp,
line 159]
nsAppStartup::Run 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/components/startup/src/nsAppStartup.cpp,
line 216]
main1 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1331]
main 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1802]
WinMain 
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1828]
WinMainCRTStartup()
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fun.

Maybe we should make the presshell's deferred attr change stuff work for
HandleEvent like it does for reflow?  That is, process the deferred changes when
we unwind from the outermost HandleEvent?

Comment 6

[[e-mail]]

Actually sliders assume their thumbs have frames, so setting thumb {
display:none; } will cause a crash sooner or later. This is why scrollbar.xml
uses <thumb xbl:inherits="collapsed=disabled"> instead of hidden. Martijn, does
your CSS work if you use thumb { visibility: collapse; } instead?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Actually sliders assume their thumbs have frames

Then we should either disable styling of thumbs altogether or enforce the
desired display value in ua.css.

Comment 8

[Martijn Wargers (dead)]

(In reply to comment #6)
> Martijn, does your CSS work if you use thumb { visibility: collapse; } instead?
Yes, then it's working without crashing.

Comment 9

[[e-mail]]

Attachment: Proposed patch

Comment 10

[[e-mail]]

Comment on attachment 167014 [details] [diff] [review]
Proposed patch

Like this? (I thought xul.css would be more appropriate than ua.css). Sorry for
the stringbundle stuff, remind me not to check that in.

Comment 11

[Boris Zbarsky [:bzbarsky]]

xul.css is part of ua.css, so yes, it's the right place.

Except, of course, it may make more sense to do this in the scoped stylesheet of
the binding... but I don't know what happens with scrollbars when one mucks with
the binding on thumb.  I would have expected the thumb binding to be set in the
scrollbar scoped stylesheet, with !important, myself...

Comment 12

[[e-mail]]

(In reply to comment #11)
>the scoped stylesheet of the binding
Each theme (including third-party themes) has its own stylesheet...

Comment 13

[Boris Zbarsky [:bzbarsky]]

A binding can have multiple scoped stylesheets.... one from the theme, and one
from our chrome, say.

Again, that's just an option; I don't know the relevant code well enough to know
whether that's warranted.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167014 [details] [diff] [review]
Proposed patch

Anyway, r+sr=bzbarsky for this...

Comment 15

[[e-mail]]

Fix checked in.

Comment 16

[:deanis74]

(In reply to comment #10)
> (From update of attachment 167014 [details] [diff] [review])
> Sorry for the stringbundle stuff, remind me not to check that in.

You checked it in.

Comment 17

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using the testcase at
https://bugzilla.mozilla.org/attachment.cgi?id=151565&action=view.  No longer
crashing with build 2004-12-06-06 on Windows XP.

Comment 18

[jag (Peter Annema)]

Backed out the stringbundle stuff.

Comment 19

[Martijn Wargers (dead)]

Attachment: patch for Firefox

This was never fixed for Firefox. So, it should also be fixed for that one,
not?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 194838 [details] [diff] [review]
patch for Firefox

Probably want this crash fix on the branch...

Comment 21

[Boris Zbarsky [:bzbarsky]]

Checked in the Firefox patch on the trunk.

Comment 22

[Scott MacGregor]

Triage team: this fix has been in the trunk for seamonkey since December of
2004, so it has had plenty of testing and baking time there. 

Comment 23

[Asa Dotzler [:asa]]

Time is short for 1.8b4. If this isn't landed today, it's not going to make the
train.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Checked in the Firefox patch on the 1.8 branch.

Comment 25

[Tracy Walker [:tracy]]

verified on Firefox 1.4 -mozilla1.8 branch- Win : 2005-09-07