Attachment 348655 Details for Bug 189154 – Patch

[patch] Patch

bug-189154-20180831112300.patch (text/plain), 9.46 KB, created by Brent Fulgham on 2018-08-31 11:23:01 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Brent Fulgham

Created: 2018-08-31 11:23:01 PDT

Size: 9.46 KB

patch

obsolete

>Subversion Revision: 235523
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 9267c1f9909e5a8a76a4ee1e4bfe074fdaaa8847..ee1738994791da481ee6c39d05f2b822e5029c8c 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,20 @@
>+2018-08-31  Brent Fulgham  <bfulgham@apple.com>
>+
>+        The width of a nullptr TextRun should be zero
>+        https://bugs.webkit.org/show_bug.cgi?id=189154
>+        <rdar://problem/43685926>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Most accessors in WTFString.cpp, such as isAllASCII(), hash(), etc., perform a nullptr check
>+        before using m_impl, but is8Bit() does not.
>+
>+        This patch adds a check in the is8Bit() implementation to be consistent with other methods,
>+        and to address a small number of crashes observed in testing.
>+
>+        * wtf/text/WTFString.h:
>+        (WTF::String::is8Bit const):
>+
> 2018-08-30  Tim Horton  <timothy_horton@apple.com>
> 
>         Bundle unified sources more tightly in projects with deep directory structures
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 399eb4962eeb7fab59e2d06533f70c6964f767fa..50c349311a632ba5c47272c268cd176bb98cec0e 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,31 @@
>+2018-08-31  Brent Fulgham  <bfulgham@apple.com>
>+
>+        The width of a nullptr TextRun should be zero
>+        https://bugs.webkit.org/show_bug.cgi?id=189154
>+        <rdar://problem/43685926>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        If a page has an empty TextRun and attempts to paint it we can crash with a nullptr.
>+
>+        This patch recognizes that an empty TextRun should always produce a zero width, rather than
>+        attempt to compute this value from font data.
>+
>+        Test: fast/text/null-string-textrun.html
>+
>+        * platform/graphics/ComplexTextController.cpp:
>+        (WebCore::TextLayout::isNeeded): An empty RenderText does not need to layout its text.
>+        * platform/graphics/FontCascade.cpp:
>+        (WebCore::FontCascade::widthOfTextRange const): An empty TextRun has zero width.
>+        (WebCore::FontCascade::width const): Ditto.
>+        (WebCore::FontCascade::codePath const): ASSERT that the TextRun is non-empty.
>+        * rendering/RenderText.cpp:
>+        (WebCore::RenderText::computeCanUseSimplifiedTextMeasuring): Don't consider font and CodePath
>+        if the RenderText contains a nullptr string.
>+        * rendering/svg/SVGTextMetricsBuilder.cpp:
>+        (WebCore::SVGTextMetricsBuilder::initializeMeasurementWithTextRenderer): Only consider font
>+        and CodePath if the RenderText contains a non-nullptr string.
>+
> 2018-08-30  Truitt Savell  <tsavell@apple.com>
> 
>         Unreviewed, rolling out r235516.
>diff --git a/Source/WTF/wtf/text/WTFString.h b/Source/WTF/wtf/text/WTFString.h
>index b08be530af463f149aaa837e6fc0f72011beb344..fb18428a5eb3873278ef507808fa90b8e6904945 100644
>--- a/Source/WTF/wtf/text/WTFString.h
>+++ b/Source/WTF/wtf/text/WTFString.h
>@@ -154,7 +154,7 @@ public:
>     // Return characters8() or characters16() depending on CharacterType.
>     template<typename CharacterType> const CharacterType* characters() const;
> 
>-    bool is8Bit() const { return m_impl->is8Bit(); }
>+    bool is8Bit() const { return !m_impl || m_impl->is8Bit(); }
> 
>     unsigned sizeInBytes() const { return m_impl ? m_impl->length() * (is8Bit() ? sizeof(LChar) : sizeof(UChar)) : 0; }
> 
>diff --git a/Source/WebCore/platform/graphics/ComplexTextController.cpp b/Source/WebCore/platform/graphics/ComplexTextController.cpp
>index 7d8ef0a3dafc426df51e683fc4df08954c380ffe..7a7ab6c933c1c56e7d696d67fde46b1dc2f8aafa 100644
>--- a/Source/WebCore/platform/graphics/ComplexTextController.cpp
>+++ b/Source/WebCore/platform/graphics/ComplexTextController.cpp
>@@ -70,6 +70,9 @@ class TextLayout {
> public:
>     static bool isNeeded(RenderText& text, const FontCascade& font)
>     {
>+        if (!text.length())
>+            return false;
>+
>         TextRun run = RenderBlock::constructTextRun(text, text.style());
>         return font.codePath(run) == FontCascade::Complex;
>     }
>diff --git a/Source/WebCore/platform/graphics/FontCascade.cpp b/Source/WebCore/platform/graphics/FontCascade.cpp
>index 7648f5bf2693bee26f0598ecebffaf1ac23d042c..2fbd0061bff32ff105dc5c4b5fe3781b507e88dd 100644
>--- a/Source/WebCore/platform/graphics/FontCascade.cpp
>+++ b/Source/WebCore/platform/graphics/FontCascade.cpp
>@@ -341,6 +341,9 @@ float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned
>     ASSERT(from <= to);
>     ASSERT(to <= run.length());
> 
>+    if (!run.length())
>+        return 0;
>+
>     float offsetBeforeRange = 0;
>     float offsetAfterRange = 0;
>     float totalWidth = 0;
>@@ -385,6 +388,9 @@ float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned
> 
> float FontCascade::width(const TextRun& run, HashSet<const Font*>* fallbackFonts, GlyphOverflow* glyphOverflow) const
> {
>+    if (!run.length())
>+        return 0;
>+
>     CodePath codePathToUse = codePath(run);
>     if (codePathToUse != Complex) {
>         // The complex path is more restrictive about returning fallback fonts than the simple path, so we need an explicit test to make their behaviors match.
>@@ -604,6 +610,8 @@ FontCascade::CodePath FontCascade::codePath(const TextRun& run, std::optional<un
>     if (s_codePath != Auto)
>         return s_codePath;
> 
>+    ASSERT(run.length());
>+
> #if !USE(FREETYPE)
>     // FIXME: Use the fast code path once it handles partial runs with kerning and ligatures. See http://webkit.org/b/100050
>     if ((enableKerning() || requiresShaping()) && (from.value_or(0) || to.value_or(run.length()) != run.length()))
>diff --git a/Source/WebCore/rendering/RenderText.cpp b/Source/WebCore/rendering/RenderText.cpp
>index ff05b72d26fe4967534011ceaadcfd3f4fd4caf2..2db90ed82fbf358f282ac1d266f1dc8f83e0c4b1 100644
>--- a/Source/WebCore/rendering/RenderText.cpp
>+++ b/Source/WebCore/rendering/RenderText.cpp
>@@ -1243,10 +1243,12 @@ bool RenderText::computeCanUseSimplifiedTextMeasuring() const
>         return false;
> 
>     // Additional check on the font codepath.
>-    TextRun run(m_text);
>-    run.setCharacterScanForCodePath(false);
>-    if (font.codePath(run) != FontCascade::Simple)
>-        return false;
>+    if (!m_text.isEmpty()) {
>+        TextRun run(m_text);
>+        run.setCharacterScanForCodePath(false);
>+        if (font.codePath(run) != FontCascade::Simple)
>+            return false;
>+    }
> 
>     auto whitespaceIsCollapsed = style().collapseWhiteSpace();
>     for (unsigned i = 0; i < text().length(); ++i) {
>diff --git a/Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp b/Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp
>index 324102584c1f113d0eed33214a67f5534ab0f59a..1f5309f287cc9a02e7c5c4267691f874790f1a13 100644
>--- a/Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp
>+++ b/Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp
>@@ -98,12 +98,14 @@ void SVGTextMetricsBuilder::initializeMeasurementWithTextRenderer(RenderSVGInlin
> 
>     const FontCascade& scaledFont = text.scaledFont();
>     m_run = SVGTextMetrics::constructTextRun(text);
>-    m_isComplexText = scaledFont.codePath(m_run) == FontCascade::Complex;
>+    if (text.length()) {
>+        m_isComplexText = scaledFont.codePath(m_run) == FontCascade::Complex;
> 
>-    if (m_isComplexText)
>-        m_simpleWidthIterator = nullptr;
>-    else
>-        m_simpleWidthIterator = std::make_unique<WidthIterator>(&scaledFont, m_run);
>+        if (m_isComplexText)
>+            m_simpleWidthIterator = nullptr;
>+        else
>+            m_simpleWidthIterator = std::make_unique<WidthIterator>(&scaledFont, m_run);
>+    }
> }
> 
> struct MeasureTextData {
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 34abd263c985ae7baf5b1289fe46beb1b18c7775..17f372badf4c9046df0b381c64aa482a8063bd14 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-08-31  Brent Fulgham  <bfulgham@apple.com>
>+
>+        The width of a nullptr TextRun should be zero
>+        https://bugs.webkit.org/show_bug.cgi?id=189154
>+        <rdar://problem/43685926>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * fast/text/null-string-textrun-expected.txt: Added.
>+        * fast/text/null-string-textrun.html: Added.
>+
> 2018-08-30  Truitt Savell  <tsavell@apple.com>
> 
>         Unreviewed, rolling out r235516.
>diff --git a/LayoutTests/fast/text/null-string-textrun-expected.txt b/LayoutTests/fast/text/null-string-textrun-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..99d8c8928ee37dbaadfa148b3dc7f111181b3bc4
>--- /dev/null
>+++ b/LayoutTests/fast/text/null-string-textrun-expected.txt
>@@ -0,0 +1,6 @@
>+This test confirms that a null text run doesn't trigger a crash. It passes if it loads without crashing.
>+
>+        
>+        
>+    
>+
>diff --git a/LayoutTests/fast/text/null-string-textrun.html b/LayoutTests/fast/text/null-string-textrun.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..b145900dbfe85b5b77aff7171c8cb7cac79f1c41
>--- /dev/null
>+++ b/LayoutTests/fast/text/null-string-textrun.html
>@@ -0,0 +1,19 @@
>+<!doctype html>
>+<head>
>+<script>
>+if (window.testRunner)
>+    testRunner.dumpAsText();
>+</script>
>+<head>
>+<body>
>+    <p>This test confirms that a null text run doesn't trigger a crash. It passes if it loads without crashing.</p>
>+    <pre id="pre_tag" dir="RTL" >
>+        <style onload="pre_tag.appendChild(meter_tag)"/></style>
>+        <select multiple="multiple">
>+            <optgroup/>
>+        </select>
>+    </pre>
>+    <label>
>+        <meter id="meter_tag">
>+    </label>
>+</body>
>\ No newline at end of file

Actions: View | Formatted Diff | Diff

Attachments on bug 189154: 348501 | 348509 | 348518 | 348655 | 348743 | 348825 | 348925