REGRESSION (298372@main): Occasional CheckedPtr crash when executing insertUnorderedList command

whsieh · whsieh · commit 8c41028b00f8 · 2025-09-25T01:15:21.000-07:00
https://bugs.webkit.org/show_bug.cgi?id=299492 rdar://161086162 Reviewed by Abrar Rahman Protyasha and Ryosuke Niwa. After the changes in 298372@main, we now crash when decrementing `CheckedPtr<RenderStyle>` underneath `EditingStyle::init`: ``` if (node && node->computedStyle()) { CheckedPtr renderStyle = node->computedStyle(); removeTextFillAndStrokeColorsIfNeeded(renderStyle.get()); if (renderStyle->fontDescription().keywordSize()) { if (auto cssValue = computedStyleAtPosition.getFontSizeCSSValuePreferringKeyword()) mutableStyle->setProperty(CSSPropertyFontSize, cssValue->cssText(CSS::defaultSerializationContext())); } } ``` This is because `getFontSizeCSSValuePreferringKeyword()` updates layout, which could potentially destroy and recreate the node's renderer along with its `RenderStyle`. To fix this, we simply limit the scope of the `CheckedPtr` so that it doesn't outlive the layout update. Test: editing/execCommand/insert-unordered-list-after-DOMNodeRemoved.html * LayoutTests/editing/execCommand/insert-unordered-list-after-DOMNodeRemoved-expected.txt: Added. * LayoutTests/editing/execCommand/insert-unordered-list-after-DOMNodeRemoved.html: Added. Add a test to exercise the fix; this test passes if it does not crash. * Source/WebCore/editing/EditingStyle.cpp: (WebCore::EditingStyle::init): Canonical link: https://commits.webkit.org/300502@main
diff --git a/LayoutTests/editing/execCommand/insert-unordered-list-after-DOMNodeRemoved-expected.txt b/LayoutTests/editing/execCommand/insert-unordered-list-after-DOMNodeRemoved-expected.txt
@@ -0,0 +1 @@
+PASS
diff --git a/LayoutTests/editing/execCommand/insert-unordered-list-after-DOMNodeRemoved.html b/LayoutTests/editing/execCommand/insert-unordered-list-after-DOMNodeRemoved.html
@@ -0,0 +1,56 @@
+<!DOCTYPE html><!-- webkit-test-runner [ dumpJSConsoleLogInStdErr=true ] -->
+<html>
+<head>
+<style>
+html, body, table, span, abbr, p, video, output {
+    font-variant-ligatures: common-ligatures no-contextual;
+    justify-self: unsafe self-end;
+    -webkit-column-span: all;
+    transform: scale3d(9, 1, 0);
+    overflow: -webkit-paged-y clip;
+    font-style: normal;
+}
+</style>
+<script>
+window.testRunner?.waitUntilDone();
+window.testRunner?.dumpAsText();
+resizeValue = 1
+insertListCount = 0
+
+setTimeout(() => {
+    document.writeln("PASS");
+    window.testRunner?.notifyDone();
+}, 1000);
+
+function main() {
+    let outputElement = document.querySelector("output");
+    let videoElement = document.querySelector("video");
+    document.getElementById("target").addEventListener("DOMNodeRemoved", () => {
+        try { outputElement.remove(); } catch (e) { }
+        resizeTo(resizeValue, resizeValue++);
+        requestAnimationFrame(() => {
+            find("F");
+            requestAnimationFrame(() => {
+                document.execCommand("insertUnorderedList");
+            });
+        });
+    });
+    document.styleSheets[0].media.appendMedium("all and (orientation:portrait)");
+    videoElement.remove();
+}
+</script>
+</head>
+<body onload="main()" contenteditable autofocus>
+<table rules="none" spellcheck="false" accesskey="A" border="989"></table>
+<span id="text">F</span>
+<abbr>
+    A
+    <p>
+        <span id="target">
+            <video></video>
+            <output></output>
+        </span>
+    </p>
+</abbr>
+</body>
+</html>
diff --git a/Source/WebCore/editing/EditingStyle.cpp b/Source/WebCore/editing/EditingStyle.cpp
@@ -590,9 +590,14 @@ void EditingStyle::init(Node* initialNode, PropertiesToInclude propertiesToInclu
     }
 
     if (node && node->computedStyle()) {
-        CheckedPtr renderStyle = node->computedStyle();
-        removeTextFillAndStrokeColorsIfNeeded(renderStyle.get());
-        if (renderStyle->fontDescription().keywordSize()) {
+        bool shouldSetFontSize = false;
+        {
+            CheckedPtr renderStyle = node->computedStyle();
+            removeTextFillAndStrokeColorsIfNeeded(renderStyle.get());
+            shouldSetFontSize = renderStyle->fontDescription().keywordSize();
+        }
+
+        if (shouldSetFontSize) {
             if (auto cssValue = computedStyleAtPosition.getFontSizeCSSValuePreferringKeyword())
                 mutableStyle->setProperty(CSSPropertyFontSize, cssValue->cssText(CSS::defaultSerializationContext()));
         }
