Use-after-move in RenderCombineText::combineTextIfNeeded()
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Feb 2019 23:42:22 +0000 (23:42 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Feb 2019 23:42:22 +0000 (23:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195188

Reviewed by Zalan Bujtas.

Source/WebCore:

r241288 uncovered an existing problem with our text-combine code. r242204 alleviated the
symptom, but this patch fixes the source of the problem (and reverts r242204).

The code in RenderCombineText::combineTextIfNeeded() has a bit that’s like:

FontDescription bestFitDescription;
while (...) {
    FontCascade compressedFont(WTFMove(bestFitDescription), ...);
    ...
}

Clearly this is wrong.

Test: fast/text/text-combine-crash-2.html

* platform/graphics/cocoa/FontDescriptionCocoa.cpp:
(WebCore::FontDescription::platformResolveGenericFamily):
* rendering/RenderCombineText.cpp:
(WebCore::RenderCombineText::combineTextIfNeeded):

LayoutTests:

* fast/text/text-combine-crash-2-expected.html: Added.
* fast/text/text-combine-crash-2.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242237 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/text/text-combine-crash-2-expected.html [new file with mode: 0644]
LayoutTests/fast/text/text-combine-crash-2.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp
Source/WebCore/rendering/RenderCombineText.cpp

index c81c985..b57feed 100644 (file)
@@ -1,3 +1,13 @@
+2019-02-28  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Use-after-move in RenderCombineText::combineTextIfNeeded()
+        https://bugs.webkit.org/show_bug.cgi?id=195188
+
+        Reviewed by Zalan Bujtas.
+
+        * fast/text/text-combine-crash-2-expected.html: Added.
+        * fast/text/text-combine-crash-2.html: Added.
+
 2019-02-28  Devin Rousso  <drousso@apple.com>
 
         REGRESSION (r240644): Layout Test inspector/page/overrideSetting-ICECandidateFilteringEnabled.html is a flaky timeout
diff --git a/LayoutTests/fast/text/text-combine-crash-2-expected.html b/LayoutTests/fast/text/text-combine-crash-2-expected.html
new file mode 100644 (file)
index 0000000..5936d7e
--- /dev/null
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html lang="ja">
+<head>
+<meta charset="utf-8">
+</head>
+<body>
+This test passes if there is no crash.
+<p xmlns="http://www.w3.org/1999/xhtml" style="-webkit-font-smoothing: subpixel-antialiased;
+-webkit-hyphenate-limit-after: 3;
+-webkit-hyphenate-limit-before: 3;
+-webkit-hyphenate-limit-lines: 2;
+-webkit-hyphens: auto;
+-webkit-line-box-contain: block inline replaced;
+-webkit-locale: ja;
+display: block;
+font-family: serif;
+font-size: 22.399999618530273px;
+height: 636px;
+line-break: strict;
+margin-bottom: 0px;
+margin-left: 0px;
+margin-right: 0px;
+margin-top: 0px;
+orphans: 2;
+text-align: start;
+text-indent: 22.399999618530273px;
+text-rendering: auto;
+widows: 2;
+width: 33px;
+word-wrap: break-word;
+writing-mode: vertical-rl;">四桁文字<span class="tcy" style="-epub-text-combine:   horizontal;">ABCD</span></p>
+</body>
+</html>
diff --git a/LayoutTests/fast/text/text-combine-crash-2.html b/LayoutTests/fast/text/text-combine-crash-2.html
new file mode 100644 (file)
index 0000000..5936d7e
--- /dev/null
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html lang="ja">
+<head>
+<meta charset="utf-8">
+</head>
+<body>
+This test passes if there is no crash.
+<p xmlns="http://www.w3.org/1999/xhtml" style="-webkit-font-smoothing: subpixel-antialiased;
+-webkit-hyphenate-limit-after: 3;
+-webkit-hyphenate-limit-before: 3;
+-webkit-hyphenate-limit-lines: 2;
+-webkit-hyphens: auto;
+-webkit-line-box-contain: block inline replaced;
+-webkit-locale: ja;
+display: block;
+font-family: serif;
+font-size: 22.399999618530273px;
+height: 636px;
+line-break: strict;
+margin-bottom: 0px;
+margin-left: 0px;
+margin-right: 0px;
+margin-top: 0px;
+orphans: 2;
+text-align: start;
+text-indent: 22.399999618530273px;
+text-rendering: auto;
+widows: 2;
+width: 33px;
+word-wrap: break-word;
+writing-mode: vertical-rl;">四桁文字<span class="tcy" style="-epub-text-combine:   horizontal;">ABCD</span></p>
+</body>
+</html>
index 6c20267..2e78a2b 100644 (file)
@@ -1,3 +1,30 @@
+2019-02-28  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        Use-after-move in RenderCombineText::combineTextIfNeeded()
+        https://bugs.webkit.org/show_bug.cgi?id=195188
+
+        Reviewed by Zalan Bujtas.
+
+        r241288 uncovered an existing problem with our text-combine code. r242204 alleviated the
+        symptom, but this patch fixes the source of the problem (and reverts r242204).
+
+        The code in RenderCombineText::combineTextIfNeeded() has a bit that’s like:
+
+        FontDescription bestFitDescription;
+        while (...) {
+            FontCascade compressedFont(WTFMove(bestFitDescription), ...);
+            ...
+        }
+
+        Clearly this is wrong.
+
+        Test: fast/text/text-combine-crash-2.html
+
+        * platform/graphics/cocoa/FontDescriptionCocoa.cpp:
+        (WebCore::FontDescription::platformResolveGenericFamily):
+        * rendering/RenderCombineText.cpp:
+        (WebCore::RenderCombineText::combineTextIfNeeded):
+
 2019-02-28  Zalan Bujtas  <zalan@apple.com>
 
         [ContentChangeObserver] Introduce observer subclasses to scope content change observing.
index 572df39..8b9c434 100644 (file)
@@ -165,7 +165,8 @@ static void languageChanged(void*)
 
 AtomicString FontDescription::platformResolveGenericFamily(UScriptCode script, const AtomicString& locale, const AtomicString& familyName)
 {
-    if (locale.isNull() || script == USCRIPT_COMMON)
+    ASSERT((locale.isNull() && script == USCRIPT_COMMON) || !locale.isNull());
+    if (script == USCRIPT_COMMON)
         return nullAtom();
 
     static std::once_flag onceFlag;
index 15f1619..5b1a287 100644 (file)
@@ -174,7 +174,7 @@ void RenderCombineText::combineTextIfNeeded()
             bestFitDescription.setComputedSize(computedSize);
             shouldUpdateFont = m_combineFontStyle->setFontDescription(FontCascadeDescription { bestFitDescription });
         
-            FontCascade compressedFont(WTFMove(bestFitDescription), style().fontCascade().letterSpacing(), style().fontCascade().wordSpacing());
+            FontCascade compressedFont(FontCascadeDescription(bestFitDescription), style().fontCascade().letterSpacing(), style().fontCascade().wordSpacing());
             compressedFont.update(fontSelector);
             
             glyphOverflow.left = glyphOverflow.top = glyphOverflow.right = glyphOverflow.bottom = 0;