Closed Bug 181463 Opened 20 years ago Closed 20 years ago

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

Categories

(Core :: XUL, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: chuckie, Assigned: hyatt)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files, 2 obsolete files)

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.
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>
Attached file Testcase from reporter
confirming using build 20021119 on Win2k.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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) 
Keywords: stackwanted
Summary: Clicking on slider with hidden thumb crashes browser → Clicking on slider with hidden thumb crashes browser [@ nsSliderFrame::HandlePress ]
Whiteboard: TB14253891X
Attached patch null check (diff -u) (obsolete) — Splinter Review
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)
Attached file null check (diff -wu) (obsolete) —
for your viewing pleasure
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 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
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 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+
Attached file diff -wu
checked in
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
I do not crash anymore using build 2002112305 on Linux.
Works for me as well. Thanks for the speedy fix!
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 on attachment 107195 [details] [diff] [review]
null check (diff -u)

Sigh.
Attachment #107195 - Flags: superreview?
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.