78910 2012-02-17 10:06:24 -0800 ScrollbarThemeComposite::thumbPosition uses the result of a divide by zero 2012-02-22 12:57:35 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 leviw leviw darin eae eric hyatt jamesr sam simon.fraser tonikitoo webkit.review.bot oldest_to_newest 559279 0 leviw 2012-02-17 10:06:24 -0800 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 559300 1 127608 leviw 2012-02-17 10:31:16 -0800 Created attachment 127608 Patch 559324 2 eric 2012-02-17 10:47:12 -0800 How is this possible. I thought divide by zero crashed on x86? 559332 3 leviw 2012-02-17 10:51:11 -0800 (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. 559335 4 leviw 2012-02-17 10:53:27 -0800 (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. 559339 5 eric 2012-02-17 10:56:14 -0800 Can we write a test for this case? 559340 6 eric 2012-02-17 10:56:31 -0800 I see, there already is a test. But this change doens't fix that test? 559344 7 leviw 2012-02-17 11:01:15 -0800 (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. 559358 8 127608 eric 2012-02-17 11:16:38 -0800 Comment on attachment 127608 Patch OK. How do we actually fix the FIXME? 559363 9 leviw 2012-02-17 11:23:10 -0800 (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. 559370 10 127608 jamesr 2012-02-17 11:27:48 -0800 Comment on attachment 127608 Patch Why 1 instead of 0? I think this would be cleaner as a separate check for usedTotalSize(scrollbar) == scrollbar->visibleSize() 559373 11 leviw 2012-02-17 11:35:29 -0800 (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()? 559414 12 tonikitoo 2012-02-17 12:03:37 -0800 (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? 559429 13 leviw 2012-02-17 12:37:40 -0800 (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. 559430 14 127608 leviw 2012-02-17 12:38:39 -0800 Comment on attachment 127608 Patch Removing the r+. I'll use James' advice for now. 559517 15 127650 leviw 2012-02-17 13:59:20 -0800 Created attachment 127650 Patch 559545 16 127650 eric 2012-02-17 14:29:11 -0800 Comment on attachment 127650 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. 562182 17 128253 leviw 2012-02-22 11:32:50 -0800 Created attachment 128253 Patch 562211 18 128253 eric 2012-02-22 11:59:26 -0800 Comment on attachment 128253 Patch Seems reasonable. I'm slightly surprised we can't detect this any way through a layout test. 562284 19 128253 webkit.review.bot 2012-02-22 12:57:29 -0800 Comment on attachment 128253 Patch Clearing flags on attachment: 128253 Committed r108541: <http://trac.webkit.org/changeset/108541> 562285 20 webkit.review.bot 2012-02-22 12:57:35 -0800 All reviewed patches have been landed. Closing bug. 127608 2012-02-17 10:31:16 -0800 2012-02-17 13:58:59 -0800 Patch bug-78910-20120217103115.patch text/plain 1831 leviw SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwODA5MCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBACisyMDEyLTAyLTE3ICBMZXZpIFdl aW50cmF1YiAgPGxldml3QGNocm9taXVtLm9yZz4KKworICAgICAgICBTY3JvbGxiYXJUaGVtZUNv bXBvc2l0ZTo6dGh1bWJQb3NpdGlvbiB1c2VzIHRoZSByZXN1bHQgb2YgYSBkaXZpZGUgYnkgemVy bworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Nzg5MTAK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUZXN0aW5n IGZvciBOYU4gYXMgdGhlIHJlc3VsdCBvZiBhIGZsb2F0aW5nIHBvaW50IGRpdmlkZSBmdW5jdGlv biB0aGF0IHNvbWV0aW1lcyBkaXZpZGVzIGJ5IHplcm8uCisgICAgICAgIFRoaXMgZG9lc24ndCBj cmFzaCwgYnV0IGFzc2lnbnMgTmFOIHRvIGFuIGludGVnZXIgYW5kIHVzZXMgdGhhdCBhcyBhIHNp emUgaW4gdGhlIHJlbmRlcmluZyBjb2RlLgorICAgICAgICBUaGlzIGNhdXNlcyBwcm9ibGVtcyB3 aXRoIG91ciBjb252ZXJzaW9uIHRvIHN1YnBpeGVsIGxheW91dCwgd2hpY2ggYXNzZXJ0cyB3aGVu IHdlIG92ZXJmbG93LgorCisgICAgICAgICogcGxhdGZvcm0vU2Nyb2xsYmFyVGhlbWVDb21wb3Np dGUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2Nyb2xsYmFyVGhlbWVDb21wb3NpdGU6OnRodW1i UG9zaXRpb24pOgorCiAyMDEyLTAyLTE3ICBTdGVwaGVuIENoZW5uZXkgIDxzY2hlbm5leUBjaHJv bWl1bS5vcmc+CiAKICAgICAgICAgQ3Jhc2ggYXQgV2ViQ29yZTo6U1ZHVXNlRWxlbWVudDo6ZXhw YW5kU3ltYm9sRWxlbWVudHNJblNoYWRvd1RyZWUKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL1Njcm9sbGJhclRoZW1lQ29tcG9zaXRlLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS9TY3JvbGxiYXJUaGVtZUNvbXBvc2l0ZS5jcHAJKHJldmlzaW9uIDEwNzk1 NCkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL1Njcm9sbGJhclRoZW1lQ29tcG9zaXRlLmNw cAkod29ya2luZyBjb3B5KQpAQCAtMjI2LDggKzIyNiw5IEBAIHN0YXRpYyBmbG9hdCB1c2VkVG90 YWxTaXplKFNjcm9sbGJhciogc2MKIGludCBTY3JvbGxiYXJUaGVtZUNvbXBvc2l0ZTo6dGh1bWJQ b3NpdGlvbihTY3JvbGxiYXIqIHNjcm9sbGJhcikKIHsKICAgICBpZiAoc2Nyb2xsYmFyLT5lbmFi bGVkKCkpIHsKKyAgICAgICAgLy8gRklYTUU6IHdoZW4gdXNlZFRvdGFsU2l6ZShzY3JvbGxiYXIp ID09IHNjcm9sbGJhci0+dmlzaWJsZVNpemUoKSwgd2UgZGl2aWRlIGJ5IHplcm8uCiAgICAgICAg IGZsb2F0IHBvcyA9IG1heCgwLjBmLCBzY3JvbGxiYXItPmN1cnJlbnRQb3MoKSkgKiAodHJhY2tM ZW5ndGgoc2Nyb2xsYmFyKSAtIHRodW1iTGVuZ3RoKHNjcm9sbGJhcikpIC8gKHVzZWRUb3RhbFNp emUoc2Nyb2xsYmFyKSAtIHNjcm9sbGJhci0+dmlzaWJsZVNpemUoKSk7Ci0gICAgICAgIHJldHVy biAocG9zIDwgMSAmJiBwb3MgPiAwKSA/IDEgOiBwb3M7CisgICAgICAgIHJldHVybiAocG9zIDwg MSAmJiBwb3MgPiAwKSB8fCBpc25hbihwb3MpID8gMSA6IHBvczsKICAgICB9CiAgICAgcmV0dXJu IDA7CiB9Cg== 127650 2012-02-17 13:59:20 -0800 2012-02-22 11:32:46 -0800 Patch bug-78910-20120217135919.patch text/plain 1635 leviw SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwODA5MCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTAyLTE3ICBMZXZpIFdl aW50cmF1YiAgPGxldml3QGNocm9taXVtLm9yZz4KKworICAgICAgICBTY3JvbGxiYXJUaGVtZUNv bXBvc2l0ZTo6dGh1bWJQb3NpdGlvbiB1c2VzIHRoZSByZXN1bHQgb2YgYSBkaXZpZGUgYnkgemVy bworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Nzg5MTAK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBZGRpbmcg YSBjaGVjayB0byBhdm9pZCBkb2luZyBhIGZsb2F0aW5nIHBvaW50IGRpdmlkZSBieSB6ZXJvIGFu ZCBhc3NpZ25pbmcgTmFOIHRvIGFuIGludGVnZXIuCisgICAgICAgIFRoaXMgY2F1c2VzIHByb2Js ZW1zIHdpdGggb3VyIGNvbnZlcnNpb24gdG8gc3VicGl4ZWwgbGF5b3V0LCB3aGljaCBhc3NlcnRz IHdoZW4gd2Ugb3ZlcmZsb3cuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9TY3JvbGxiYXJUaGVtZUNv bXBvc2l0ZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpTY3JvbGxiYXJUaGVtZUNvbXBvc2l0ZTo6 dGh1bWJQb3NpdGlvbik6CisKIDIwMTItMDItMTcgIFN0ZXBoZW4gQ2hlbm5leSAgPHNjaGVubmV5 QGNocm9taXVtLm9yZz4KIAogICAgICAgICBDcmFzaCBhdCBXZWJDb3JlOjpTVkdVc2VFbGVtZW50 OjpleHBhbmRTeW1ib2xFbGVtZW50c0luU2hhZG93VHJlZQpJbmRleDogU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vU2Nyb2xsYmFyVGhlbWVDb21wb3NpdGUuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL1Njcm9sbGJhclRoZW1lQ29tcG9zaXRlLmNwcAkocmV2aXNpb24g MTA3OTU0KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2Nyb2xsYmFyVGhlbWVDb21wb3Np dGUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yMjYsNiArMjI2LDggQEAgc3RhdGljIGZsb2F0IHVz ZWRUb3RhbFNpemUoU2Nyb2xsYmFyKiBzYwogaW50IFNjcm9sbGJhclRoZW1lQ29tcG9zaXRlOjp0 aHVtYlBvc2l0aW9uKFNjcm9sbGJhciogc2Nyb2xsYmFyKQogewogICAgIGlmIChzY3JvbGxiYXIt PmVuYWJsZWQoKSkgeworICAgICAgICBpZiAodXNlZFRvdGFsU2l6ZShzY3JvbGxiYXIpID09IHNj cm9sbGJhci0+dmlzaWJsZVNpemUoKSkKKyAgICAgICAgICAgIHJldHVybiAxOwogICAgICAgICBm bG9hdCBwb3MgPSBtYXgoMC4wZiwgc2Nyb2xsYmFyLT5jdXJyZW50UG9zKCkpICogKHRyYWNrTGVu Z3RoKHNjcm9sbGJhcikgLSB0aHVtYkxlbmd0aChzY3JvbGxiYXIpKSAvICh1c2VkVG90YWxTaXpl KHNjcm9sbGJhcikgLSBzY3JvbGxiYXItPnZpc2libGVTaXplKCkpOwogICAgICAgICByZXR1cm4g KHBvcyA8IDEgJiYgcG9zID4gMCkgPyAxIDogcG9zOwogICAgIH0K 128253 2012-02-22 11:32:50 -0800 2012-02-22 12:57:29 -0800 Patch bug-78910-20120222113249.patch text/plain 1899 leviw SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwODA5MCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDEyLTAyLTE3ICBMZXZpIFdl aW50cmF1YiAgPGxldml3QGNocm9taXVtLm9yZz4KKworICAgICAgICBTY3JvbGxiYXJUaGVtZUNv bXBvc2l0ZTo6dGh1bWJQb3NpdGlvbiB1c2VzIHRoZSByZXN1bHQgb2YgYSBkaXZpZGUgYnkgemVy bworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Nzg5MTAK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBZGRpbmcg YSBjaGVjayB0byBhdm9pZCBkb2luZyBhIGZsb2F0aW5nIHBvaW50IGRpdmlkZSBieSB6ZXJvIGFu ZCBhc3NpZ25pbmcgTmFOIHRvIGFuIGludGVnZXIuCisgICAgICAgIFRoaXMgY2F1c2VzIHByb2Js ZW1zIHdpdGggb3VyIGNvbnZlcnNpb24gdG8gc3VicGl4ZWwgbGF5b3V0LCB3aGljaCBhc3NlcnRz IHdoZW4gd2Ugb3ZlcmZsb3cuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9TY3JvbGxiYXJUaGVtZUNv bXBvc2l0ZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpTY3JvbGxiYXJUaGVtZUNvbXBvc2l0ZTo6 dGh1bWJQb3NpdGlvbik6CisKIDIwMTItMDItMTcgIFN0ZXBoZW4gQ2hlbm5leSAgPHNjaGVubmV5 QGNocm9taXVtLm9yZz4KIAogICAgICAgICBDcmFzaCBhdCBXZWJDb3JlOjpTVkdVc2VFbGVtZW50 OjpleHBhbmRTeW1ib2xFbGVtZW50c0luU2hhZG93VHJlZQpJbmRleDogU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vU2Nyb2xsYmFyVGhlbWVDb21wb3NpdGUuY3BwCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL1Njcm9sbGJhclRoZW1lQ29tcG9zaXRlLmNwcAkocmV2aXNpb24g MTA3OTU0KQorKysgU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2Nyb2xsYmFyVGhlbWVDb21wb3Np dGUuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yMjYsNyArMjI2LDExIEBAIHN0YXRpYyBmbG9hdCB1 c2VkVG90YWxTaXplKFNjcm9sbGJhciogc2MKIGludCBTY3JvbGxiYXJUaGVtZUNvbXBvc2l0ZTo6 dGh1bWJQb3NpdGlvbihTY3JvbGxiYXIqIHNjcm9sbGJhcikKIHsKICAgICBpZiAoc2Nyb2xsYmFy LT5lbmFibGVkKCkpIHsKLSAgICAgICAgZmxvYXQgcG9zID0gbWF4KDAuMGYsIHNjcm9sbGJhci0+ Y3VycmVudFBvcygpKSAqICh0cmFja0xlbmd0aChzY3JvbGxiYXIpIC0gdGh1bWJMZW5ndGgoc2Ny b2xsYmFyKSkgLyAodXNlZFRvdGFsU2l6ZShzY3JvbGxiYXIpIC0gc2Nyb2xsYmFyLT52aXNpYmxl U2l6ZSgpKTsKKyAgICAgICAgZmxvYXQgc2l6ZSA9IHVzZWRUb3RhbFNpemUoc2Nyb2xsYmFyKSAt IHNjcm9sbGJhci0+dmlzaWJsZVNpemUoKTsKKyAgICAgICAgLy8gQXZvaWQgZG9pbmcgYSBmbG9h dGluZyBwb2ludCBkaXZpZGUgYnkgemVybyBhbmQgcmV0dXJuIDEgd2hlbiB1c2VkVG90YWxTaXpl ID09IHZpc2libGVTaXplLgorICAgICAgICBpZiAoIXNpemUpCisgICAgICAgICAgICByZXR1cm4g MTsKKyAgICAgICAgZmxvYXQgcG9zID0gbWF4KDAuMGYsIHNjcm9sbGJhci0+Y3VycmVudFBvcygp KSAqICh0cmFja0xlbmd0aChzY3JvbGxiYXIpIC0gdGh1bWJMZW5ndGgoc2Nyb2xsYmFyKSkgLyBz aXplOwogICAgICAgICByZXR1cm4gKHBvcyA8IDEgJiYgcG9zID4gMCkgPyAxIDogcG9zOwogICAg IH0KICAgICByZXR1cm4gMDsK