Regression(r237903) Speedometer 2 is 1-2% regressed on iOS

• Component: Text
• Status: RESOLVED FIXED

[Chris Dumez]

PLT is 1-2% regressed on iOS after r237903.

[Chris Dumez]

<rdar://problem/45957016>

[Chris Dumez]

Created attachment 367270 [details]
Patch

[Simon Fraser (smfr)]

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?

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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.

[Chris Dumez]

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.

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

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.

[Chris Dumez]

Created attachment 367415 [details]
Patch

[Chris Dumez]

(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 ]

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

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)

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 367418 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367418 [details]
Patch

Clearing flags on attachment: 367418

Committed r244277: <https://trac.webkit.org/changeset/244277>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Myles C. Maxfield]

Thanks for fixing this, Chris!