146953 – Negative scroll snap repeat values cause web process to hang indefinitely

RESOLVED FIXED 146953

Negative scroll snap repeat values cause web process to hang indefinitely

https://bugs.webkit.org/show_bug.cgi?id=146953

Summary Negative scroll snap repeat values cause web process to hang indefinitely

Wenson Hsieh

Reported 2015-07-14 17:15:09 PDT

Setting -webkit-scroll-snap-points-x or -y to repeat(-n) causes the web process to hang when computing snap offsets. Simon and I have agreed that the appropriate way to fix this is to not consider negative repeat values as valid in our CSS parser, and also min-threshold the parsed repeat value to 1px to prevent cases like 0.000001px from causing us trouble as well.

Attachments
Patch (10.50 KB, patch) 2015-07-14 18:37 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (10.15 KB, patch) 2015-07-14 19:01 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (9.56 KB, patch) 2015-07-14 21:20 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Patch (9.46 KB, patch) 2015-07-15 07:02 PDT, Wenson Hsieh	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Wenson Hsieh

Comment 1 2015-07-14 17:17:30 PDT

<rdar://problem/21823681>

Wenson Hsieh

Comment 2 2015-07-14 18:37:48 PDT

Created attachment 256816 [details] Patch

Simon Fraser (smfr)

Comment 3 2015-07-14 18:43:10 PDT

Comment on attachment 256816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256816&action=review > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:90 > + LayoutUnit repeatOffset = points ? valueForLength(points->repeatOffset, viewSize) : LayoutUnit(1.0f); LayoutUnit(1.0f) should be LayoutUnit::fromPixel(1) > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:92 > + if (repeatOffset.toFloat() < 1.0f) > + repeatOffset = LayoutUnit(1.0f); repeatOffset = std::min<LayoutUnit>(repeatOffset, 1); > LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:39 > + <style> > + #gallery { > + height: 400px; > + width: 400px; > + overflow-x: hidden; > + overflow-y: auto; > + -webkit-overflow-scrolling: touch; > + -webkit-scroll-snap-type: mandatory; > + -webkit-scroll-snap-points-y: repeat(-400px); > + margin: 0; > + padding: 0; > + } > + > + .colorBox { > + width: 400px; > + height: 400px; > + } > + > + #item0 { background-color: red; } > + #item1 { background-color: green; } > + #item2 { background-color: blue; } > + #item3 { background-color: aqua; } > + #item4 { background-color: yellow; } > + #item5 { background-color: fuchsia; } > + </style> > + <script src="../../resources/js-test-pre.js"></script> > + </head> > + <body> > + <div id="gallery"> > + <div id="item0" class="colorBox"></div> > + <div id="item1" class="colorBox"></div> > + <div id="item2" class="colorBox"></div> > + <div id="item3" class="colorBox"></div> > + <div id="item4" class="colorBox"></div> > + <div id="item5" class="colorBox"></div> > + </div> Not sure that any of this is needed. Can't you just have body { -webkit-scroll-snap-type: mandatory; -webkit-scroll-snap-points-y: repeat(-400px); } > LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:10 > + -webkit-overflow-scrolling: touch; Remove this. > LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:34 > + testRunner.waitUntilDone(); Not sure why the test needs to be async.

Wenson Hsieh

Comment 4 2015-07-14 18:59:59 PDT

Comment on attachment 256816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256816&action=review >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:90 >> + LayoutUnit repeatOffset = points ? valueForLength(points->repeatOffset, viewSize) : LayoutUnit(1.0f); > > LayoutUnit(1.0f) should be LayoutUnit::fromPixel(1) Fixed! >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:92 >> + repeatOffset = LayoutUnit(1.0f); > > repeatOffset = std::min<LayoutUnit>(repeatOffset, 1); Fixed! (I assumed std::min was meant to be std::max?) >> LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:39 >> + </div> > > Not sure that any of this is needed. Can't you just have body { -webkit-scroll-snap-type: mandatory; -webkit-scroll-snap-points-y: repeat(-400px); } Good point -- fixed! >> LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:10 >> + -webkit-overflow-scrolling: touch; > > Remove this. Removed.

Wenson Hsieh

Comment 5 2015-07-14 19:01:34 PDT

Created attachment 256818 [details] Patch

Simon Fraser (smfr)

Comment 6 2015-07-14 19:09:07 PDT

Comment on attachment 256818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256818&action=review > LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:20 > + #item0 { background-color: red; } > + #item1 { background-color: green; } > + #item2 { background-color: blue; } > + #item3 { background-color: aqua; } > + #item4 { background-color: yellow; } > + #item5 { background-color: fuchsia; } Not needed. > LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:30 > + <div id="item0" class="colorBox"></div> > + <div id="item1" class="colorBox"></div> > + <div id="item2" class="colorBox"></div> > + <div id="item3" class="colorBox"></div> > + <div id="item4" class="colorBox"></div> > + <div id="item5" class="colorBox"></div> You still don't need these. > LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:56 > + finishJSTest(); > + testRunner.notifyDone(); finishJSTest calls notifyDone for you.

Wenson Hsieh

Comment 7 2015-07-14 21:17:39 PDT

Comment on attachment 256818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256818&action=review >> LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:20 >> + #item5 { background-color: fuchsia; } > > Not needed. I left these in because the length of the scrollable content must be greater than the length of the viewport in order to trigger the code path that computes scroll snap offsets (otherwise, the while loop that causes the infinite loop won't be run and the test will pass even without this patch). However, I can make this a bit simpler -- instead of 6 elements, I can just have one really long one. >> LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:30 >> + <div id="item5" class="colorBox"></div> > > You still don't need these. Good point -- took out the colors. >> LayoutTests/css3/scroll-snap/scroll-snap-subpixel-repeat.html:56 >> + testRunner.notifyDone(); > > finishJSTest calls notifyDone for you. Got it. I'll remember that for next time.

Wenson Hsieh

Comment 8 2015-07-14 21:20:46 PDT

Created attachment 256823 [details] Patch

Simon Fraser (smfr)

Comment 9 2015-07-14 21:38:09 PDT

(In reply to comment #7) > Comment on attachment 256818 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256818&action=review > > >> LayoutTests/css3/scroll-snap/scroll-snap-negative-repeat.html:20 > >> + #item5 { background-color: fuchsia; } > > > > Not needed. Just put a height on the body in CSS.

Wenson Hsieh

Comment 10 2015-07-15 07:02:54 PDT

Created attachment 256837 [details] Patch

WebKit Commit Bot

Comment 11 2015-07-15 09:03:52 PDT

Comment on attachment 256837 [details] Patch Clearing flags on attachment: 256837 Committed r186840: <http://trac.webkit.org/changeset/186840>

WebKit Commit Bot

Comment 12 2015-07-15 09:03:57 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P1

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component WebCore Misc.

Assignee

Nobody

Reported

2015-07-14 17:15 PDT

Modified

2015-07-15 09:03 PDT History

CC List

5 users Show

URL

Keywords InRadar

Depends on

Blocks