Bug 78910 – ScrollbarThemeComposite::thumbPosition uses the result of a divide by zero

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

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

[Levi Weintraub]

Created attachment 127608 [details]
Patch

[Eric Seidel (no email)]

How is this possible.  I thought divide by zero crashed on x86?

[Levi Weintraub]

(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.

[Levi Weintraub]

(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.

[Eric Seidel (no email)]

Can we write a test for this case?

[Eric Seidel (no email)]

I see, there already is a test.  But this change doens't fix that test?

[Levi Weintraub]

(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.

[Eric Seidel (no email)]

Comment on attachment 127608 [details]
Patch

OK.  How do we actually fix the FIXME?

[Levi Weintraub]

(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.

[James Robinson]

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()

[Levi Weintraub]

(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()?

[Antonio Gomes]

(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?

[Levi Weintraub]

(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.

[Levi Weintraub]

Comment on attachment 127608 [details]
Patch

Removing the r+. I'll use James' advice for now.

[Levi Weintraub]

Created attachment 127650 [details]
Patch

[Eric Seidel (no email)]

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.

[Levi Weintraub]

Created attachment 128253 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 128253 [details]
Patch

Seems reasonable.  I'm slightly surprised we can't detect this any way through a layout test.

[WebKit Review Bot]

Comment on attachment 128253 [details]
Patch

Clearing flags on attachment: 128253

Committed r108541: <http://trac.webkit.org/changeset/108541>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.