Address Safer cpp failures in TextPainter #47356

iidmsa · 2025-06-29T19:23:01Z

`df16d2d`

Address Safer cpp failures in TextPainter
https://bugs.webkit.org/show_bug.cgi?id=295182
rdar://problem/154618319

Reviewed by Chris Dumez.

Fix clang static analyzer warnings in TextPainter.

* Source/WebCore/SaferCPPExpectations/ForwardDeclCheckerExpectations:
* Source/WebCore/SaferCPPExpectations/NoUncheckedPtrMemberCheckerExpectations:
* Source/WebCore/SaferCPPExpectations/UncheckedCallArgsCheckerExpectations:
* Source/WebCore/SaferCPPExpectations/UncheckedLocalVarsCheckerExpectations:
* Source/WebCore/rendering/TextPainter.cpp:
(WebCore::TextPainter::paintTextWithShadows):
(WebCore::TextPainter::paintTextAndEmphasisMarksIfNeeded):
* Source/WebCore/rendering/TextPainter.h:

Canonical link: https://commits.webkit.org/296830@main

1ae8733

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

cdumez · 2025-06-29T21:36:58Z

Source/WebCore/rendering/TextPainter.cpp

@@ -97,7 +96,7 @@ ShadowApplier::~ShadowApplier()
        m_context.clearDropShadow();
 }

-TextPainter::TextPainter(GraphicsContext& context, const FontCascade& font, const RenderStyle& renderStyle, const TextPaintStyle& textPaintStyle, const FixedVector<Style::TextShadow>& shadow, const FilterOperations* shadowColorFilter, const AtomString& emphasisMark, float emphasisMarkOffset, const RenderCombineText* combinedText)
+TextPainter::TextPainter(GraphicsContext& context, const CheckedRef<const FontCascade> font, const CheckedRef<const RenderStyle> renderStyle, const TextPaintStyle& textPaintStyle, const FixedVector<Style::TextShadow>& shadow, const FilterOperations* shadowColorFilter, const AtomString& emphasisMark, float emphasisMarkOffset, const CheckedPtr<const RenderCombineText> combinedText)


Nothing in safer cpp should cause you to update parameter types. This change is incorrect, we do not pass smart pointers as parameters, unless we transfer ownership and then we use r-value references.

Gotcha. Fixed.

cdumez · 2025-06-29T21:37:29Z

Source/WebCore/rendering/TextPainter.cpp

@@ -203,13 +202,13 @@ void TextPainter::paintTextAndEmphasisMarksIfNeeded(const TextRun& textRun, cons
    FloatPoint boxOrigin = boxRect.location();
    updateGraphicsContext(m_context, paintStyle, UseEmphasisMarkColor);
    static NeverDestroyed<TextRun> objectReplacementCharacterTextRun(StringView { span(objectReplacementCharacter) });
-    const TextRun& emphasisMarkTextRun = m_combinedText ? objectReplacementCharacterTextRun.get() : textRun;
-    FloatPoint emphasisMarkTextOrigin = m_combinedText ? FloatPoint(boxOrigin.x() + boxRect.width() / 2, boxOrigin.y() + m_font.metricsOfPrimaryFont().intAscent()) : textOrigin;
+    const CheckedRef emphasisMarkTextRun = m_combinedText ? objectReplacementCharacterTextRun.get() : textRun;


We can drop const now.

cdumez · 2025-06-29T21:37:47Z

Source/WebCore/rendering/TextPainter.h

@@ -54,7 +53,7 @@ static inline AffineTransform rotation(const FloatRect& boxRect, RotationDirecti

 class TextPainter {
 public:
-    TextPainter(GraphicsContext&, const FontCascade&, const RenderStyle&, const TextPaintStyle&, const FixedVector<Style::TextShadow>&, const FilterOperations*, const AtomString& emphasisMark, float emphasisMarkOffset, const RenderCombineText*);
+    TextPainter(GraphicsContext&, const CheckedRef<const FontCascade>, const CheckedRef<const RenderStyle>, const TextPaintStyle&, const FixedVector<Style::TextShadow>&, const FilterOperations*, const AtomString& emphasisMark, float emphasisMarkOffset, const CheckedPtr<const RenderCombineText>);


Does not follow our style.

webkit-early-warning-system · 2025-06-30T00:13:46Z

EWS run on previous version of this PR (hash 2384f1c)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	❌ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-ews-buildbot · 2025-06-30T01:10:56Z

Safer C++ Build #42018 (`2384f1c`)

❌ Found 1 failing file with 2 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming.
(cc @rniwa)

webkit-early-warning-system · 2025-06-30T01:44:43Z

EWS run on previous version of this PR (hash a4e4d5d)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	⏳ 🧪 win-tests
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🛠 wpe-cairo~~
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	✅ 🛠 gtk
	✅ 🛠 vision	~~🧪 mac-AS-debug-wk2~~	🧪 gtk-wk2
	✅ 🛠 vision-sim	~~🧪 mac-wk2-stress~~	🧪 api-gtk
	~~🧪 vision-wk2~~	~~🧪 mac-intel-wk2~~	✅ 🛠 playstation
	✅ 🛠 tv	~~🛠 mac-safer-cpp~~
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-early-warning-system · 2025-06-30T01:50:08Z

EWS run on previous version of this PR (hash de6af53)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	~~🛠 wpe~~	~~🛠 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	~~🧪 wpe-wk2~~	~~🧪 win-tests~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🧪 api-mac~~	~~🧪 api-wpe~~
	~~🧪 ios-wk2-wpt~~	~~🧪 mac-wk1~~	~~🛠 wpe-cairo~~
	~~🧪 api-ios~~	~~🧪 mac-wk2~~	✅ 🛠 gtk
	✅ 🛠 vision	~~🧪 mac-AS-debug-wk2~~	~~🧪 gtk-wk2~~
	✅ 🛠 vision-sim	~~🧪 mac-wk2-stress~~	~~🧪 api-gtk~~
	🧪 vision-wk2	~~🧪 mac-intel-wk2~~	~~🛠 playstation~~
	~~🛠 tv~~	~~🛠 mac-safer-cpp~~
	~~🛠 tv-sim~~
	✅ 🛠 watch
	~~🛠 watch-sim~~

webkit-early-warning-system · 2025-06-30T01:53:18Z

EWS run on previous version of this PR (hash 3c2e218)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	❌ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	❌ 🧪 api-gtk
	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

Source/WebCore/rendering/TextPainter.cpp

annevk · 2025-06-30T07:44:49Z

Source/WebCore/rendering/TextPainter.cpp

@@ -244,7 +244,7 @@ String TextPainter::cachedGlyphDisplayListsForTextNodeAsText(Text& textNode, Opt

    StringBuilder builder;

-    for (auto textBox : InlineIterator::textBoxesFor(*textNode.renderer())) {
+    for (auto textBox : InlineIterator::textBoxesFor(*CheckedPtr { textNode.renderer() })) {


You should use *textNode.checkedRenderer() here.

annevk · 2025-06-30T07:46:16Z

Source/WebCore/rendering/TextPainter.h

 */

 #pragma once

 #include "AffineTransform.h"
 #include "FloatRect.h"
 #include "GlyphDisplayListCache.h"
+#include "RenderCombineText.h"


This might regress compile times. I suspect that instead you need to include CheckedPtr if that's what you get an error for.

Right, I bet it wouldn't be needed if we moved ~TextPainter() out of line. Currently, it is implicit and thus inline in the header.

webkit-early-warning-system · 2025-06-30T17:59:54Z

EWS run on current version of this PR (hash 1ae8733)

Misc	iOS, visionOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🧪 wpe-wk2	⏳ 🧪 win-tests
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 api-wpe
	✅ 🧪 ios-wk2-wpt	✅ 🧪 mac-wk1	✅ 🛠 wpe-cairo
	✅ 🧪 api-ios	✅ 🧪 mac-wk2	✅ 🛠 gtk
	✅ 🛠 vision	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 gtk-wk2
	✅ 🛠 vision-sim	✅ 🧪 mac-wk2-stress	✅ 🧪 api-gtk
✅ 🛠 🧪 merge	✅ 🧪 vision-wk2	✅ 🧪 mac-intel-wk2	✅ 🛠 playstation
	✅ 🛠 tv	✅ 🛠 mac-safer-cpp
	✅ 🛠 tv-sim
	✅ 🛠 watch
	✅ 🛠 watch-sim

webkit-ews-buildbot · 2025-06-30T21:38:12Z

Safe-Merge-Queue: Build #61886.

https://bugs.webkit.org/show_bug.cgi?id=295182 rdar://problem/154618319 Reviewed by Chris Dumez. Fix clang static analyzer warnings in TextPainter. * Source/WebCore/SaferCPPExpectations/ForwardDeclCheckerExpectations: * Source/WebCore/SaferCPPExpectations/NoUncheckedPtrMemberCheckerExpectations: * Source/WebCore/SaferCPPExpectations/UncheckedCallArgsCheckerExpectations: * Source/WebCore/SaferCPPExpectations/UncheckedLocalVarsCheckerExpectations: * Source/WebCore/rendering/TextPainter.cpp: (WebCore::TextPainter::paintTextWithShadows): (WebCore::TextPainter::paintTextAndEmphasisMarksIfNeeded): * Source/WebCore/rendering/TextPainter.h: Canonical link: https://commits.webkit.org/296830@main

webkit-commit-queue · 2025-06-30T21:59:48Z

Committed 296830@main (df16d2d): https://commits.webkit.org/296830@main

Reviewed commits have been landed. Closing PR #47356 and removing active labels.

iidmsa self-assigned this Jun 29, 2025

iidmsa added the Layout and Rendering For bugs with layout and rendering of Web pages. label Jun 29, 2025

iidmsa requested a review from cdumez June 29, 2025 19:29

cdumez requested changes Jun 29, 2025

View reviewed changes

iidmsa force-pushed the eng/Address-Safer-cpp-failures-in-TextPainter branch from 31e5dea to fc35b59 Compare June 29, 2025 21:48

iidmsa requested a review from cdumez June 29, 2025 21:49

iidmsa force-pushed the eng/Address-Safer-cpp-failures-in-TextPainter branch from fc35b59 to 2384f1c Compare June 30, 2025 00:13

iidmsa force-pushed the eng/Address-Safer-cpp-failures-in-TextPainter branch from 2384f1c to a4e4d5d Compare June 30, 2025 01:44

iidmsa force-pushed the eng/Address-Safer-cpp-failures-in-TextPainter branch from a4e4d5d to de6af53 Compare June 30, 2025 01:50

iidmsa force-pushed the eng/Address-Safer-cpp-failures-in-TextPainter branch from de6af53 to 3c2e218 Compare June 30, 2025 01:53

annevk reviewed Jun 30, 2025

View reviewed changes

iidmsa force-pushed the eng/Address-Safer-cpp-failures-in-TextPainter branch from 3c2e218 to 1ae8733 Compare June 30, 2025 17:59

cdumez approved these changes Jun 30, 2025

View reviewed changes

iidmsa added the safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks label Jun 30, 2025

webkit-ews-buildbot added merge-queue Applied to send a pull request to merge-queue and removed safe-merge-queue Applied to automatically send a pull-request to merge-queue after passing EWS checks labels Jun 30, 2025

webkit-commit-queue force-pushed the eng/Address-Safer-cpp-failures-in-TextPainter branch from 1ae8733 to df16d2d Compare June 30, 2025 21:59

webkit-commit-queue requested a review from rniwa as a code owner June 30, 2025 21:59

webkit-commit-queue merged commit df16d2d into WebKit:main Jun 30, 2025

webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jun 30, 2025

Address Safer cpp failures in TextPainter #47356

Address Safer cpp failures in TextPainter #47356

Conversation

iidmsa commented Jun 29, 2025 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-ews-buildbot commented Jun 30, 2025

Safer C++ Build #42018 (2384f1c)

Uh oh!

webkit-early-warning-system commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-ews-buildbot commented Jun 30, 2025

Uh oh!

webkit-commit-queue commented Jun 30, 2025

Uh oh!

Uh oh!

iidmsa commented Jun 29, 2025 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Jun 30, 2025 •

edited

Loading

Safer C++ Build #42018 (`2384f1c`)

webkit-early-warning-system commented Jun 30, 2025 •

edited

Loading

webkit-early-warning-system commented Jun 30, 2025 •

edited

Loading

webkit-early-warning-system commented Jun 30, 2025 •

edited

Loading

webkit-early-warning-system commented Jun 30, 2025 •

edited

Loading