Bug 196841

<rdar://problem/45957016>

Created attachment 367270 [details] Patch

Comment on attachment 367270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367270&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:548 > + if (textUnderlinePosition() == TextUnderlinePosition::Under || other.textUnderlinePosition() == TextUnderlinePosition::Under) > + return true; Can FromFont values also trigger visual overflow?

Comment on attachment 367270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367270&action=review >> Source/WebCore/rendering/style/RenderStyle.cpp:548 >> + return true; > > Can FromFont values also trigger visual overflow? FromFont can trigger visual overflow, but any Length value can also trigger visual overflow. If the tests don't catch this, then I need to write a new test that this patch would break. If this patch fixes the regression, it means that underline rects occur outside of the text bounds, which shouldn't be true for regularly-sized text with regular outlines. I don't think our performance tests try to change the type of underlines, so this probably isn't the right fix.

See also: https://bugs.webkit.org/show_bug.cgi?id=196158 We have at least one other bug about the repaint rects being wrong for outlines. Fixing the repaint rects so they are properly placed/sized would likely make them entirely contained within their text, which would mean the repaint rects wouldn't be affected by the underlines, which means it (hopefully) would fix this performance regression.

Comment on attachment 367270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367270&action=review >>> Source/WebCore/rendering/style/RenderStyle.cpp:548 >>> + return true; >> >> Can FromFont values also trigger visual overflow? > > FromFont can trigger visual overflow, but any Length value can also trigger visual overflow. If the tests don't catch this, then I need to write a new test that this patch would break. > > If this patch fixes the regression, it means that underline rects occur outside of the text bounds, which shouldn't be true for regularly-sized text with regular outlines. I don't think our performance tests try to change the type of underlines, so this probably isn't the right fix. "I don't think our performance tests try to change the type of underlines" What do you mean by type of underline? What would it look like from the web side? (e.g. css) I could check Speedometer. I do know that Speedometer underlines text although this may not be what you meant.

(In reply to Chris Dumez from comment #6) > Comment on attachment 367270 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367270&action=review > > >>> Source/WebCore/rendering/style/RenderStyle.cpp:548 > >>> + return true; > >> > >> Can FromFont values also trigger visual overflow? > > > > FromFont can trigger visual overflow, but any Length value can also trigger visual overflow. If the tests don't catch this, then I need to write a new test that this patch would break. > > > > If this patch fixes the regression, it means that underline rects occur outside of the text bounds, which shouldn't be true for regularly-sized text with regular outlines. I don't think our performance tests try to change the type of underlines, so this probably isn't the right fix. > > "I don't think our performance tests try to change the type of underlines" > > What do you mean by type of underline? What would it look like from the web > side? (e.g. css) I could check Speedometer. I do know that Speedometer > underlines text although this may not be what you meant. Using text-decoration-thickness, text-decoration-width, text-underline-offset, or text-underline-position.

Comment on attachment 367270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367270&action=review > Source/WebCore/rendering/style/RenderStyle.cpp:549 > + return visualOverflowForDecorations(*this, nullptr) != visualOverflowForDecorations(other, nullptr); I see what was happening. Before r237903, we were running this "visualOverflowForDecorations() != visualOverflowForDecorations()" code. visualOverflowForDecorations() returns a GlyphOverflow, which contains 4 values, each of which represents how far to explode the overflow rect from the layout overflow rect. When visualOverflowForDecorations() builds up its return value, the GlyphOverflow values start at 0, and extendIntToFloat() pipes everything through max(), so that means an underline that is entirely contained within the layout overflow rect would return {0, 0, 0, 0}, which is the same as what would be returned if there was no decoration at all. Therefore, when the style change is "add an underline that exists entirely within the layout rect" or "remove such an underline", visualOverflowForDecoration() returns the same value, and RenderStyle::changeAffectsVisualOverflow() says that this change doesn't affect visual overflow. This is correct and desirable. We should preserve this "only calculate rect diffs if they occur outside of the layout rect" behavior. I think the best patch would remove the check on lines 547-548 (because visualOverflowForDecorations() will handle that) and, in the return statement on line 549, each call to visualOverflowForDecoration() would be unioned with the layout rect before doing the inequality comparison.

Comment on attachment 367270 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367270&action=review >> Source/WebCore/rendering/style/RenderStyle.cpp:549 >> + return visualOverflowForDecorations(*this, nullptr) != visualOverflowForDecorations(other, nullptr); > > I see what was happening. Before r237903, we were running this "visualOverflowForDecorations() != visualOverflowForDecorations()" code. visualOverflowForDecorations() returns a GlyphOverflow, which contains 4 values, each of which represents how far to explode the overflow rect from the layout overflow rect. When visualOverflowForDecorations() builds up its return value, the GlyphOverflow values start at 0, and extendIntToFloat() pipes everything through max(), so that means an underline that is entirely contained within the layout overflow rect would return {0, 0, 0, 0}, which is the same as what would be returned if there was no decoration at all. Therefore, when the style change is "add an underline that exists entirely within the layout rect" or "remove such an underline", visualOverflowForDecoration() returns the same value, and RenderStyle::changeAffectsVisualOverflow() says that this change doesn't affect visual overflow. This is correct and desirable. > > We should preserve this "only calculate rect diffs if they occur outside of the layout rect" behavior. I think the best patch would remove the check on lines 547-548 (because visualOverflowForDecorations() will handle that) and, in the return statement on line 549, each call to visualOverflowForDecoration() would be unioned with the layout rect before doing the inequality comparison. Oh, we don't need to do this unioning if we keep calling visualOverflowForDecorations, which does the max() behavior described above. So, yeah, I think the best patch is the same as this but without lines 545-548.

Created attachment 367415 [details] Patch

(In reply to Myles C. Maxfield from comment #9) > Comment on attachment 367270 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=367270&action=review > > >> Source/WebCore/rendering/style/RenderStyle.cpp:549 > >> + return visualOverflowForDecorations(*this, nullptr) != visualOverflowForDecorations(other, nullptr); > > > > I see what was happening. Before r237903, we were running this "visualOverflowForDecorations() != visualOverflowForDecorations()" code. visualOverflowForDecorations() returns a GlyphOverflow, which contains 4 values, each of which represents how far to explode the overflow rect from the layout overflow rect. When visualOverflowForDecorations() builds up its return value, the GlyphOverflow values start at 0, and extendIntToFloat() pipes everything through max(), so that means an underline that is entirely contained within the layout overflow rect would return {0, 0, 0, 0}, which is the same as what would be returned if there was no decoration at all. Therefore, when the style change is "add an underline that exists entirely within the layout rect" or "remove such an underline", visualOverflowForDecoration() returns the same value, and RenderStyle::changeAffectsVisualOverflow() says that this change doesn't affect visual overflow. This is correct and desirable. > > > > We should preserve this "only calculate rect diffs if they occur outside of the layout rect" behavior. I think the best patch would remove the check on lines 547-548 (because visualOverflowForDecorations() will handle that) and, in the return statement on line 549, each call to visualOverflowForDecoration() would be unioned with the layout rect before doing the inequality comparison. > > Oh, we don't need to do this unioning if we keep calling > visualOverflowForDecorations, which does the max() behavior described above. > So, yeah, I think the best patch is the same as this but without lines > 545-548. Looks like the suggested change may be causing test failures: Regressions: Unexpected crashes (3) fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-altered.html [ Crash ] fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-removed.html [ Crash ] fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect.html [ Crash ]

Comment on attachment 367415 [details] Patch Attachment 367415 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11871465 New failing tests: fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect.html fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-altered.html fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-removed.html

Created attachment 367417 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6

Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000018 Exception Note: EXC_CORPSE_NOTIFY Termination Signal: Segmentation fault: 11 Termination Reason: Namespace SIGNAL, Code 0xb Terminating Process: exc handler [0] VM Regions Near 0x18: --> __TEXT 0000000100f96000-0000000100ff3000 [ 372K] r-x/rwx SM=COW 6 [/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DumpRenderTree] Application Specific Information: CRASHING TEST: fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect-altered.html Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x0000000105bfcbe4 WebCore::InlineBox::root() const + 4 (InlineBox.h:156) 1 com.apple.WebCore 0x0000000105e27f92 WebCore::computeUnderlineOffset(WebCore::TextUnderlinePosition, WebCore::TextUnderlineOffset, WebCore::FontMetrics const&, WebCore::InlineTextBox const*, float) + 242 (InlineTextBoxStyle.cpp:69) 2 com.apple.WebCore 0x0000000105e2831f WebCore::visualOverflowForDecorations(WebCore::RenderStyle const&, WebCore::InlineTextBox const*) + 463 (InlineTextBoxStyle.cpp:135) 3 com.apple.WebCore 0x0000000105dc677b WebCore::RenderStyle::changeAffectsVisualOverflow(WebCore::RenderStyle const&) const + 203 (FontCascade.h:74) 4 com.apple.WebCore 0x0000000105dc5844 WebCore::RenderStyle::changeRequiresLayout(WebCore::RenderStyle const&, WTF::OptionSet<WebCore::StyleDifferenceContextSensitiveProperty>&) const + 1076 (RenderStyle.cpp:738) 5 com.apple.WebCore 0x0000000105dc79e0 WebCore::RenderStyle::diff(WebCore::RenderStyle const&, WTF::OptionSet<WebCore::StyleDifferenceContextSensitiveProperty>&) const + 96 (RenderStyle.cpp:1094) 6 com.apple.WebCore 0x0000000105c7f58e WebCore::RenderElement::setStyle(WebCore::RenderStyle&&, WebCore::StyleDifference) + 62 (RenderElement.cpp:402) 7 com.apple.WebCore 0x0000000105e1e371 WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdate const&) + 689 (RenderTreeUpdater.cpp:294) 8 com.apple.WebCore 0x0000000105e1d6bd WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) + 1037 (RenderTreeUpdater.cpp:191) 9 com.apple.WebCore 0x0000000105e1d21b WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 587 10 com.apple.WebCore 0x00000001054d1c0d WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 989 (memory:2595) 11 com.apple.WebCore 0x00000001054d2619 WebCore::Document::updateStyleIfNeeded() + 329 (Document.cpp:2037) 12 com.apple.WebCore 0x000000010595ff01 WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive() + 1233 13 com.apple.WebKitLegacy 0x00000001085fd5af -[WebHTMLView(WebPrivate) viewWillDraw] + 79 (WebHTMLView.mm:1595)

(In reply to Chris Dumez from comment #14) > Exception Type: EXC_BAD_ACCESS (SIGSEGV) > Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000018 > Exception Note: EXC_CORPSE_NOTIFY > > Termination Signal: Segmentation fault: 11 > Termination Reason: Namespace SIGNAL, Code 0xb > Terminating Process: exc handler [0] > > VM Regions Near 0x18: > --> > __TEXT 0000000100f96000-0000000100ff3000 [ 372K] > r-x/rwx SM=COW 6 > [/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DumpRenderTree] > > Application Specific Information: > CRASHING TEST: > fast/css3-text/css3-text-decoration/repaint/underline-outside-of-layout-rect- > altered.html > > Thread 0 Crashed:: Dispatch queue: com.apple.main-thread > 0 com.apple.WebCore 0x0000000105bfcbe4 > WebCore::InlineBox::root() const + 4 (InlineBox.h:156) > 1 com.apple.WebCore 0x0000000105e27f92 > WebCore::computeUnderlineOffset(WebCore::TextUnderlinePosition, > WebCore::TextUnderlineOffset, WebCore::FontMetrics const&, > WebCore::InlineTextBox const*, float) + 242 (InlineTextBoxStyle.cpp:69) > 2 com.apple.WebCore 0x0000000105e2831f > WebCore::visualOverflowForDecorations(WebCore::RenderStyle const&, > WebCore::InlineTextBox const*) + 463 (InlineTextBoxStyle.cpp:135) > 3 com.apple.WebCore 0x0000000105dc677b > WebCore::RenderStyle::changeAffectsVisualOverflow(WebCore::RenderStyle > const&) const + 203 (FontCascade.h:74) > 4 com.apple.WebCore 0x0000000105dc5844 > WebCore::RenderStyle::changeRequiresLayout(WebCore::RenderStyle const&, > WTF::OptionSet<WebCore::StyleDifferenceContextSensitiveProperty>&) const + > 1076 (RenderStyle.cpp:738) > 5 com.apple.WebCore 0x0000000105dc79e0 > WebCore::RenderStyle::diff(WebCore::RenderStyle const&, > WTF::OptionSet<WebCore::StyleDifferenceContextSensitiveProperty>&) const + > 96 (RenderStyle.cpp:1094) > 6 com.apple.WebCore 0x0000000105c7f58e > WebCore::RenderElement::setStyle(WebCore::RenderStyle&&, > WebCore::StyleDifference) + 62 (RenderElement.cpp:402) > 7 com.apple.WebCore 0x0000000105e1e371 > WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, > WebCore::Style::ElementUpdate const&) + 689 (RenderTreeUpdater.cpp:294) > 8 com.apple.WebCore 0x0000000105e1d6bd > WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) + 1037 > (RenderTreeUpdater.cpp:191) > 9 com.apple.WebCore 0x0000000105e1d21b > WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style:: > Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 587 > 10 com.apple.WebCore 0x00000001054d1c0d > WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 989 > (memory:2595) > 11 com.apple.WebCore 0x00000001054d2619 > WebCore::Document::updateStyleIfNeeded() + 329 (Document.cpp:2037) > 12 com.apple.WebCore 0x000000010595ff01 > WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive() + 1233 > 13 com.apple.WebKitLegacy 0x00000001085fd5af > -[WebHTMLView(WebPrivate) viewWillDraw] + 79 (WebHTMLView.mm:1595) It is not OK to call visualOverflowForDecorations() with TextUnderlinePosition::Under and a null inlineTextBox. I'll go back to my original code.

Created attachment 367418 [details] Patch

Comment on attachment 367418 [details] Patch Clearing flags on attachment: 367418 Committed r244277: <https://trac.webkit.org/changeset/244277>

All reviewed patches have been landed. Closing bug.

Thanks for fixing this, Chris!