Last Comment Bug 181463 - Clicking on slider with hidden thumb crashes browser [@ nsSliderFrame::HandlePress ]
: Clicking on slider with hidden thumb crashes browser [@ nsSliderFrame::Handle...
: crash, testcase
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: David Hyatt
: Neil Deakin
Depends on:
  Show dependency treegraph
Reported: 2002-11-22 07:43 PST by Chuck Musser
Modified: 2011-06-09 14:58 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Testcase from reporter (555 bytes, application/vnd.mozilla.xul+xml)
2002-11-22 09:56 PST, Olivier Cahagne
no flags Details
null check (diff -u) (1.51 KB, patch)
2002-11-22 16:46 PST, John Morrison
jvarga: review+
Details | Diff | Splinter Review
null check (diff -wu) (915 bytes, text/plain)
2002-11-22 16:47 PST, John Morrison
no flags Details
null-check, use ternary inside if, whitespace cleanup (diff -u) (1.50 KB, patch)
2002-11-22 19:24 PST, John Morrison
jrgmorrison: review+
jrgmorrison: superreview+
Details | Diff | Splinter Review
diff -wu (1011 bytes, text/plain)
2002-11-22 19:34 PST, John Morrison
no flags Details

Description Chuck Musser 2002-11-22 07:43:34 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.3a) Gecko/20021122
Build Identifier: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:1.3a) Gecko/20021122

Clicking any area of a XUL <slider> whose <thumb>'s CSS "display" property is
"none" crashes the browser. 

I'll include a testcase that has two sliders: one with a visible thumb and the
other with a hidden thumb. The hidden thumb slider is the problem.

Reproducible: Always

Steps to Reproduce:
1. Load the testcase XUL document
2. Click any part of the slider on the right.
Actual Results:  
The browser crashed and the talkback window appeared.

Expected Results:  
The behavior should probably be the same as when the thumb is visible: dragging
the (invisible) thumb or clicking above or below it should updates the curpos
attribute and move the thumb.

Talkback crash ID is TB14253891X. Unfortunately, I can't find a core file on my
system. I have a small testcase, which I'll attach shortly.

As to the question "why would anyone want a slider with an invisible thumb?!",
my little application restyles the slider and thumb to be a moveable fret marker
on a guitar fretboard. For open or mute notes, another button in the UI makes
the marker disappear and reappear. It really does make sense, trust me. The
testcase uses no special styling and I've reproduced the problem with the Modern
Comment 1 Chuck Musser 2002-11-22 07:57:54 PST
Bugzilla's "Create a New Attachment" didn't work, so here's the testcase:

<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>
<?xml-stylesheet href="chrome://leftychord/skin/" type="text/css"?>

  id="hiddenthumb" title="Hidden Thumb Crash Test"
   style="height: 375px; width: 250px"

<box height="100">
  <slider orient="vertical">
  <slider orient="vertical">
     <thumb style="display:none"/>
Comment 2 Olivier Cahagne 2002-11-22 09:56:29 PST
Created attachment 107156 [details]
Testcase from reporter
Comment 3 Olivier Cahagne 2002-11-22 09:57:49 PST
confirming using build 20021119 on Win2k.
Comment 4 (gone - use instead) 2002-11-22 14:46:23 PST
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsSliderFrame.cpp, line 1166]
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsFrame.cpp, line 979]
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsSliderFrame.cpp, line 733]
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6246]
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6136]
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 2209]
nsView::HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 304]
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 1949]
HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 83]
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1073]
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1090]
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 5283]
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 5538]
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 4065]
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1339]
USER32.dll + 0x2a2b8 (0x77e3a2b8)
USER32.dll + 0x45b1 (0x77e145b1)
USER32.dll + 0xa752 (0x77e1a752)
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 472]
main1 [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1557]
main [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1905]
WinMain [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1925]
KERNEL32.dll + 0x2847c (0x77ea847c) 
Comment 5 John Morrison 2002-11-22 16:46:33 PST
Created attachment 107195 [details] [diff] [review]
null check (diff -u)

match the other null-check spackle fixes that have been applied to that file
(and clean up some indentation and line-wrap while I'm there)
Comment 6 John Morrison 2002-11-22 16:47:38 PST
Created attachment 107196 [details]
null check (diff -wu)

for your viewing pleasure
Comment 7 John Morrison 2002-11-22 16:50:14 PST
Comment on attachment 107195 [details] [diff] [review]
null check (diff -u)

This also includes some whitespace cleanup. There is a diff -wu version on the
Comment 8 Jan Varga [:janv] 2002-11-22 16:55:16 PST
Comment on attachment 107195 [details] [diff] [review]
null check (diff -u)

looks good
Comment 9 Brendan Eich [:brendan] 2002-11-22 18:10:59 PST
Comment on attachment 107195 [details] [diff] [review]
null check (diff -u)

> nsSliderFrame::HandlePress(nsIPresContext* aPresContext,
>-                     nsGUIEvent*     aEvent,
>-                     nsEventStatus*  aEventStatus)
>+                           nsGUIEvent*     aEvent,
>+                           nsEventStatus*  aEventStatus)
> {
>   PRBool isHorizontal = IsHorizontal();

Get rid of this variable (see below).  Unless for some odd, uncommented reason,
you need to sample it here, before calling mFrames.FirstChild() or

>+  nscoord change = 1;
>+  if ((isHorizontal && aEvent->point.x < thumbRect.x) ||
>+      (!isHorizontal && aEvent->point.y < thumbRect.y))

That is to say:

+  if (IsHorizontal() ? aEvent->point.x < thumbRect.x)
		      : aEvent->point.y < thumbRect.y)

Fix that, and sr=brendan (I bet hyatt won't mind my jumping in here).

Comment 10 Brendan Eich [:brendan] 2002-11-22 18:12:31 PST
Oops, extra right paren in my ?: condition -- but you get the idea (where's
emacs electric-c-mode in this textarea when i need it? ;-).

Comment 11 John Morrison 2002-11-22 19:24:23 PST
Created attachment 107205 [details] [diff] [review]
null-check, use ternary inside if, whitespace cleanup (diff -u)

No, there's no side effect in calling IsHorizontal(), so lose the temporary
and modify the if statement. Thanks.
Comment 12 John Morrison 2002-11-22 19:26:46 PST
Comment on attachment 107205 [details] [diff] [review]
null-check, use ternary inside if, whitespace cleanup (diff -u)

(Hmm, this may wind up looking like r=jrgm,sr=jrgm
but it's r=varga,sr=brendan.)
Comment 13 John Morrison 2002-11-22 19:34:54 PST
Created attachment 107206 [details]
diff -wu
Comment 14 John Morrison 2002-11-22 21:21:09 PST
checked in
Comment 15 Olivier Cahagne 2002-11-23 06:29:23 PST
I do not crash anymore using build 2002112305 on Linux.
Comment 16 Chuck Musser 2002-11-23 15:31:28 PST
Works for me as well. Thanks for the speedy fix!
Comment 17 John Morrison 2002-11-24 21:05:45 PST
Comment on attachment 107195 [details] [diff] [review]
null check (diff -u)

Hmm, the patch manager doesn't automagically remove review requests from
obsolete bugs (so do it manually)
Comment 18 John Morrison 2002-11-24 21:07:22 PST
Comment on attachment 107195 [details] [diff] [review]
null check (diff -u)


Note You need to log in before you can comment on or make changes to this bug.