<rdar://60746109> #0 0x11f4297bd in WebCore::operator/(WebCore::LayoutUnit const&, WebCore::LayoutUnit const&) (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x2a477bd) #1 0x121505658 in WebCore::RenderBlockFlow::computeColumnCountAndWidth() (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x4b23658) #2 0x12150513b in WebCore::RenderBlockFlow::recomputeLogicalWidthAndColumnWidth() (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x4b2313b) #3 0x121506349 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x4b24349) #4 0x1214ccc74 in WebCore::RenderBlock::layout() (Safari_ASAN_258820_08056b77ac1d7bd944690afb1f182d3dda421853.app/Contents/Frameworks/WebCore.framework/Versions/A/WebCore:x86_64+0x4aeac74)
Root cause: In function computeColumnCountAndWidth, the computed colGap is -1 px, and colWidth is 1px. And next (colWidth + colGap) is used as denominator to get desiredColumnCount, resulting in division by zero. All the pixel settings seem normal, but function contentWidth() gives a negative value. contentWidth = paddingBoxWidth - paddingLeft - paddingRight where paddingBoxWidth=0, paddingLeft=0, paddingRight=64 And paddingBoxWidth = std::max(0_lu, width() - borderLeft() - borderRight() - verticalScrollbarWidth()); where width=192, borderLeft=64, borderRight=64, verticalScrollbarWidth=15
Division by zero so should not have security implications.
Created attachment 394395 [details] Patch
Created attachment 394408 [details] Patch
Limit the minimum to 0 in function contentWidth() and contentHeight() breaks expected result of some tests. (In reply to Jack from comment #3) > Created attachment 394395 [details] > Patch
Limit the minimum of column gap instead. However, this patch would still show error message below when the grid-gap is set to 50%. There is no error when grid-gap is 100%. ERROR: !(m_value >= 0) /Users/jacklee/browser2/OpenSource/Source/WebCore/platform/LayoutUnit.h(114) : unsigned int WebCore::LayoutUnit::toUnsigned() const <style> #TEXTAREA { grid-gap: 50%; -webkit-logical-width: 0px; } </style> <script> window.onload = () => { TEXTAREA.style.setProperty("column-width", "1px"); TEXTAREA.style.setProperty("padding", "0px 1px 0px 0px"); } </script> <body><textarea id=TEXTAREA><span></span></textarea> (In reply to Jack from comment #4) > Created attachment 394408 [details] > Patch
the first patch looks correct, you should check the nature of the different geometry and why we end up producing it (it might just be correct).
Alan, thanks! But a lot of tests fail in first patch. I guess the content width/height were negative in those tests. (In reply to zalan from comment #7) > the first patch looks correct, you should check the nature of the different > geometry and why we end up producing it (it might just be correct).
(In reply to Jack from comment #8) > Alan, thanks! But a lot of tests fail in first patch. I guess the content > width/height were negative in those tests. I see only one failure. https://ews-build.webkit.org/results/macOS-Mojave-Release-WK1-Tests-EWS/r394395-5787/results.html Am I missing something here?
Thanks for checking out the patch. There is one crash and one failed case in Mac-wk1, two failures in Mac-wk2 and one failure in ios-wk2 and mac-debug-wk1 when I obsolete the change. Most of them show different pixel sizes (almost for every element), so I think the test cases probably expect negative content width/height. :-( layer at (0,0) size 800x251 RenderBlock {HTML} at (0,0) size 800x251 RenderBody {BODY} at (8,8) size 784x235 layer at (0,0) size 800x249 RenderBlock {HTML} at (0,0) size 800x249 RenderBody {BODY} at (8,8) size 784x233 (In reply to zalan from comment #9) > (In reply to Jack from comment #8) > > Alan, thanks! But a lot of tests fail in first patch. I guess the content > > width/height were negative in those tests. > I see only one failure. > https://ews-build.webkit.org/results/macOS-Mojave-Release-WK1-Tests-EWS/ > r394395-5787/results.html > Am I missing something here?
(In reply to Jack from comment #10) > Thanks for checking out the patch. There is one crash and one failed case in > Mac-wk1, two failures in Mac-wk2 and one failure in ios-wk2 and > mac-debug-wk1 when I obsolete the change. I am so confused. Are we looking at the same test output? e.g. I don't see a crash at all. Could you include a link to these failures?
(In reply to zalan from comment #11) > (In reply to Jack from comment #10) > > Thanks for checking out the patch. There is one crash and one failed case in > > Mac-wk1, two failures in Mac-wk2 and one failure in ios-wk2 and > > mac-debug-wk1 when I obsolete the change. > I am so confused. Are we looking at the same test output? e.g. I don't see a > crash at all. Could you include a link to these failures? I guess you meant "open-after-abort.htm". that must be unrelated. please check locally.
I see. Thanks! Other links are attached below. There are five cases failing, but button.html fail on three MAC platforms. Maybe the width/height is negative only on MAC? And I am not sure if the other two are related to the code change. I will run the test to verify. css3/flexbox/button.html fast/hidpi/image-srcset-relative-svg-canvas-2x.html imported/w3c/web-platform-tests/content-security-policy/reporting/report-uri-from-child-frame.html https://ews-build.webkit.org/results/iOS-13-Simulator-WK2-Tests-EWS/r394395-13750/results.html https://ews-build.webkit.org/results/macOS-Mojave-Release-WK1-Tests-EWS/r394395-5787/results.html https://ews-build.webkit.org/results/macOS-Mojave-Release-WK2-Tests-EWS/r394395-5865/results.html https://ews-build.webkit.org/results/macOS-Mojave-Debug-WK1-Tests-EWS/r394395-5855/results.html (In reply to zalan from comment #12) > I guess you meant "open-after-abort.htm". that must be unrelated. please > check locally.
(In reply to Jack from comment #13) > I see. Thanks! Other links are attached below. There are five cases failing, > but button.html fail on three MAC platforms. Maybe the width/height is > negative only on MAC? And I am not sure if the other two are related to the > code change. I will run the test to verify. > > fast/hidpi/image-srcset-relative-svg-canvas-2x.html This is most likely unrelated. If you look at the diff, you can see that this is a circle drawing mismatch. > imported/w3c/web-platform-tests/content-security-policy/reporting/report-uri- > from-child-frame.html There's no diff to it's hard to tell but I'd be very surprised if it was caused by this change. > css3/flexbox/button.html I think this is the only failure caused by this change.
Thanks! Got it. I will submit the first patch and rerun the test. Hopefully the other two will disappear this time. But are they flaky cases? Since they don't fail in the second patch somehow. (In reply to zalan from comment #14) > (In reply to Jack from comment #13) > > I see. Thanks! Other links are attached below. There are five cases failing, > > but button.html fail on three MAC platforms. Maybe the width/height is > > negative only on MAC? And I am not sure if the other two are related to the > > code change. I will run the test to verify. > > > > fast/hidpi/image-srcset-relative-svg-canvas-2x.html > This is most likely unrelated. If you look at the diff, you can see that > this is a circle drawing mismatch. > > > > imported/w3c/web-platform-tests/content-security-policy/reporting/report-uri- > > from-child-frame.html > There's no diff to it's hard to tell but I'd be very surprised if it was > caused by this change. > > > css3/flexbox/button.html > I think this is the only failure caused by this change.
Created attachment 394745 [details] Patch
Thanks Alan for discussion. We are going to limit minimal value of content width and height to 0, since there is no concept of negative content size. Run with modified expected output for button.html (only for MAC platform). (In reply to Jack from comment #16) > Created attachment 394745 [details] > Patch
Committed r259210: <https://trac.webkit.org/changeset/259210> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394745 [details].
There is no security implication here.