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
Created attachment 127608 [details] Patch
How is this possible. I thought divide by zero crashed on x86?
(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.
(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.
Can we write a test for this case?
I see, there already is a test. But this change doens't fix that test?
(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 on attachment 127608 [details] Patch OK. How do we actually fix the FIXME?
(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 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()
(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()?
(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?
(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 on attachment 127608 [details] Patch Removing the r+. I'll use James' advice for now.
Created attachment 127650 [details] Patch
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.
Created attachment 128253 [details] Patch
Comment on attachment 128253 [details] Patch Seems reasonable. I'm slightly surprised we can't detect this any way through a layout test.
Comment on attachment 128253 [details] Patch Clearing flags on attachment: 128253 Committed r108541: <http://trac.webkit.org/changeset/108541>
All reviewed patches have been landed. Closing bug.