Bug 78910 - ScrollbarThemeComposite::thumbPosition uses the result of a divide by zero
Summary: ScrollbarThemeComposite::thumbPosition uses the result of a divide by zero
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-17 10:06 PST by Levi Weintraub
Modified: 2012-02-22 12:57 PST (History)
9 users (show)

See Also:


Attachments
Patch (1.79 KB, patch)
2012-02-17 10:31 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (1.60 KB, patch)
2012-02-17 13:59 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (1.85 KB, patch)
2012-02-22 11:32 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-02-17 10:06:24 PST
While testing on the subpixellayout branch, I discovered several tests that assign a float NaN to an int that's used as a size. We get NaN in ScrollbarThemeComposite::thumbPosition when usedTotalSize(scrollbar) == scrollbar->visibleSize(), and we end up dividing by zero. I'm going to catch this case and assign the result to be 1, unless there are any objections.

Example failing test: scrollbars/listbox-scrollbar-combinations.html
Comment 1 Levi Weintraub 2012-02-17 10:31:16 PST
Created attachment 127608 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-17 10:47:12 PST
How is this possible.  I thought divide by zero crashed on x86?
Comment 3 Levi Weintraub 2012-02-17 10:51:11 PST
(In reply to comment #2)
> How is this possible.  I thought divide by zero crashed on x86?

Integer divide by zero does... floating point returns NaN.
Comment 4 Levi Weintraub 2012-02-17 10:53:27 PST
(In reply to comment #3)
> (In reply to comment #2)
> > How is this possible.  I thought divide by zero crashed on x86?
> 
> Integer divide by zero does... floating point returns NaN.

Actually, 0.0/0.0 == NaN. If the numerator is positive or negative, you get an appropriately signed infinity.
Comment 5 Eric Seidel (no email) 2012-02-17 10:56:14 PST
Can we write a test for this case?
Comment 6 Eric Seidel (no email) 2012-02-17 10:56:31 PST
I see, there already is a test.  But this change doens't fix that test?
Comment 7 Levi Weintraub 2012-02-17 11:01:15 PST
(In reply to comment #6)
> I see, there already is a test.  But this change doens't fix that test?

There are several tests that do this (I put an assert in to test). This value ends up in an IntSize with a giant negative number, which eventually will just get maxed with zero. In subpixel code, we have assertions guarding against this sort of thing, so I want to fix the source.
Comment 8 Eric Seidel (no email) 2012-02-17 11:16:38 PST
Comment on attachment 127608 [details]
Patch

OK.  How do we actually fix the FIXME?
Comment 9 Levi Weintraub 2012-02-17 11:23:10 PST
(In reply to comment #8)
> (From update of attachment 127608 [details])
> OK.  How do we actually fix the FIXME?

I was hoping Antonio could chime in on that. He reviewed the original code.

CC-ing jamesr and smfr to see if they have any insight.
Comment 10 James Robinson 2012-02-17 11:27:48 PST
Comment on attachment 127608 [details]
Patch

Why 1 instead of 0?

I think this would be cleaner as a separate check for usedTotalSize(scrollbar) == scrollbar->visibleSize()
Comment 11 Levi Weintraub 2012-02-17 11:35:29 PST
(In reply to comment #10)
> (From update of attachment 127608 [details])
> Why 1 instead of 0?
> 
> I think this would be cleaner as a separate check for usedTotalSize(scrollbar) == scrollbar->visibleSize()

I'm happy to make those changes. Is it expected that usedTotalSize(scrollbar) would ever equal scrollbar->visibleSize()?
Comment 12 Antonio Gomes 2012-02-17 12:03:37 PST
(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 127608 [details] [details])
> > OK.  How do we actually fix the FIXME?
> 
> I was hoping Antonio could chime in on that. He reviewed the original code.
> 
> CC-ing jamesr and smfr to see if they have any insight.

Hi Levi. sorry about the delay...

would you mind to point to the original revision that introduced the problem?
Comment 13 Levi Weintraub 2012-02-17 12:37:40 PST
(In reply to comment #12)
> Hi Levi. sorry about the delay...
> 
> would you mind to point to the original revision that introduced the problem?

No worries. I was actually wrong, as you reviewed r91547, which didn't actually introduce the possibility of a divide by zero. That actually came from r76831.

Anyways, I'll adopt James' strategy for a fix unless someone more familiar with this code has some advice. I plead mostly ignorance, but want to avoid the current bad behavior.
Comment 14 Levi Weintraub 2012-02-17 12:38:39 PST
Comment on attachment 127608 [details]
Patch

Removing the r+. I'll use James' advice for now.
Comment 15 Levi Weintraub 2012-02-17 13:59:20 PST
Created attachment 127650 [details]
Patch
Comment 16 Eric Seidel (no email) 2012-02-17 14:29:11 PST
Comment on attachment 127650 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127650&action=review

> Source/WebCore/platform/ScrollbarThemeComposite.cpp:230
> +        if (usedTotalSize(scrollbar) == scrollbar->visibleSize())
> +            return 1;

You should add a why comment to explain this.  Another way to check this would be to use a local for used - visible and check against 0.
Comment 17 Levi Weintraub 2012-02-22 11:32:50 PST
Created attachment 128253 [details]
Patch
Comment 18 Eric Seidel (no email) 2012-02-22 11:59:26 PST
Comment on attachment 128253 [details]
Patch

Seems reasonable.  I'm slightly surprised we can't detect this any way through a layout test.
Comment 19 WebKit Review Bot 2012-02-22 12:57:29 PST
Comment on attachment 128253 [details]
Patch

Clearing flags on attachment: 128253

Committed r108541: <http://trac.webkit.org/changeset/108541>
Comment 20 WebKit Review Bot 2012-02-22 12:57:35 PST
All reviewed patches have been landed.  Closing bug.