Clicking on slider with hidden thumb crashes browser [@ nsSliderFrame::HandlePress ]

RESOLVED FIXED

Status

()

Core
XUL
--
critical
RESOLVED FIXED
15 years ago
6 years ago

People

(Reporter: Chuck Musser, Assigned: David Hyatt)

Tracking

({crash, testcase})

Trunk
x86
All
crash, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

15 years ago
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
theme.
(Reporter)

Comment 1

15 years ago
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"?>

<window
  id="hiddenthumb" title="Hidden Thumb Crash Test"
   style="height: 375px; width: 250px"
  xmlns:html="http://www.w3.org/1999/xhtml"
  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">

<box height="100">
  <slider orient="vertical">
     <thumb/>
  </slider>
  <slider orient="vertical">
     <thumb style="display:none"/>
  </slider>
</box>
</window>

Comment 2

15 years ago
Created attachment 107156 [details]
Testcase from reporter

Comment 3

15 years ago
confirming using build 20021119 on Win2k.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash, stackwanted, testcase
OS: Linux → All
Whiteboard: TB14253891X
nsSliderFrame::HandlePress
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsSliderFrame.cpp, line 1166]
nsFrame::HandleEvent
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsFrame.cpp, line 979]
nsSliderFrame::HandleEvent
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsSliderFrame.cpp, line 733]
PresShell::HandleEventInternal
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6246]
PresShell::HandleEvent
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6136]
nsViewManager::HandleEvent
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 2209]
nsView::HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 304]
nsViewManager::DispatchEvent
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 1949]
HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 83]
nsWindow::DispatchEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1073]
nsWindow::DispatchWindowEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1090]
nsWindow::DispatchMouseEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 5283]
ChildWindow::DispatchMouseEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 5538]
nsWindow::ProcessMessage
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 4065]
nsWindow::WindowProc
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1339]
USER32.dll + 0x2a2b8 (0x77e3a2b8)
USER32.dll + 0x45b1 (0x77e145b1)
USER32.dll + 0xa752 (0x77e1a752)
nsAppShellService::Run
[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]
WinMainCRTStartup()
KERNEL32.dll + 0x2847c (0x77ea847c) 

Updated

15 years ago
Keywords: stackwanted
Summary: Clicking on slider with hidden thumb crashes browser → Clicking on slider with hidden thumb crashes browser [@ nsSliderFrame::HandlePress ]
Whiteboard: TB14253891X

Comment 5

15 years ago
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

15 years ago
Created attachment 107196 [details]
null check (diff -wu)

for your viewing pleasure

Comment 7

15 years ago
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
bug
Attachment #107195 - Flags: superreview?(hyatt)
Attachment #107195 - Flags: review?(varga)

Comment 8

15 years ago
Comment on attachment 107195 [details] [diff] [review]
null check (diff -u)

looks good
r=varga
Attachment #107195 - Flags: review?(varga) → review+
Comment on attachment 107195 [details] [diff] [review]
null check (diff -u)

> NS_IMETHODIMP
> 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
thumbFrame->GetRect?

>+  
>+  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).

/be
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? ;-).

/be

Comment 11

15 years ago
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.
Attachment #107195 - Attachment is obsolete: true
Attachment #107196 - Attachment is obsolete: true

Comment 12

15 years ago
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.)
Attachment #107205 - Flags: superreview+
Attachment #107205 - Flags: review+

Comment 13

15 years ago
Created attachment 107206 [details]
diff -wu

Comment 14

15 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 15

15 years ago
I do not crash anymore using build 2002112305 on Linux.
(Reporter)

Comment 16

15 years ago
Works for me as well. Thanks for the speedy fix!

Comment 17

15 years ago
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)
Attachment #107195 - Flags: superreview?(hyatt) → superreview?

Comment 18

15 years ago
Comment on attachment 107195 [details] [diff] [review]
null check (diff -u)

Sigh.
Attachment #107195 - Flags: superreview?

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: shrir → xptoolkit.widgets
Crash Signature: [@ nsSliderFrame::HandlePress ]
You need to log in before you can comment on or make changes to this bug.