Attachment 348925 Details for Bug 189154 – Patch

[patch] Patch

bug-189154-20180905084630.patch (text/plain), 8.88 KB, created by Brent Fulgham on 2018-09-05 08:46:31 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Brent Fulgham

Created: 2018-09-05 08:46:31 PDT

Size: 8.88 KB

patch

obsolete

>Subversion Revision: 235618
>diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
>index 37942bc85786d383e5fea2cacfa9490a2102e4bc..83b1774035de6bf2af9eb74f44c8d88c7cd4271a 100644
>--- a/Source/WTF/ChangeLog
>+++ b/Source/WTF/ChangeLog
>@@ -1,3 +1,20 @@
>+2018-09-05  Brent Fulgham  <bfulgham@apple.com>
>+
>+        The width of an empty or nullptr TextRun should be zero
>+        https://bugs.webkit.org/show_bug.cgi?id=189154
>+        <rdar://problem/43685926>
>+
>+        Reviewed by Zalan Bujtas.
>+
>+        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-31  Antti Koivisto  <antti@apple.com>
> 
>         Replace OptionSet |= and -= operators with add() and remove() functions
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f2234b96b91e90eb1eb08cb532648f6635139142..b67a2edb475eebc68a9ad2032a3735e7829bba11 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,28 @@
>+2018-09-05  Brent Fulgham  <bfulgham@apple.com>
>+
>+        The width of an empty or nullptr TextRun should be zero
>+        https://bugs.webkit.org/show_bug.cgi?id=189154
>+        <rdar://problem/43685926>
>+
>+        Reviewed by Zalan Bujtas.
>+
>+        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. It also prevents ListBox from attempting to
>+        paint a null string.
>+
>+        Test: fast/text/null-string-textrun.html
>+
>+        * platform/graphics/FontCascade.cpp:
>+        (WebCore::FontCascade::widthOfTextRange const): An empty TextRun has zero width.
>+        (WebCore::FontCascade::width const): Ditto.
>+        * platform/graphics/TextRun.h:
>+        (WebCore::TextRun::TextRun): ASSERT that the supplied String is non-null.
>+        (WebCore::TextRun::setText): Ditto.
>+        * rendering/RenderListBox.cpp:
>+        (WebCore::RenderListBox::paintItemForeground): Don't attempt to paint a null string.
>+
> 2018-09-04  Daniel Bates  <dabates@apple.com>
> 
>         Add helper function to create a potential CORS request
>diff --git a/Source/WTF/wtf/text/WTFString.h b/Source/WTF/wtf/text/WTFString.h
>index b08be530af463f149aaa837e6fc0f72011beb344..ddedefd63b94ea820bf03f8695bdd3d480ff1543 100644
>--- a/Source/WTF/wtf/text/WTFString.h
>+++ b/Source/WTF/wtf/text/WTFString.h
>@@ -1,6 +1,6 @@
> /*
>  * (C) 1999 Lars Knoll (knoll@kde.org)
>- * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2004-2018 Apple Inc. All rights reserved.
>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Library General Public
>@@ -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/FontCascade.cpp b/Source/WebCore/platform/graphics/FontCascade.cpp
>index 7648f5bf2693bee26f0598ecebffaf1ac23d042c..08692534a4747c0bc8229d5a4701c40f58060491 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.
>diff --git a/Source/WebCore/platform/graphics/TextRun.h b/Source/WebCore/platform/graphics/TextRun.h
>index d39bcf79171489d8d356d2c9f0ff9eaafe8d477d..e2bb4be4df8c9eb16167ec3c81a1dbcf30bf3aca 100644
>--- a/Source/WebCore/platform/graphics/TextRun.h
>+++ b/Source/WebCore/platform/graphics/TextRun.h
>@@ -57,6 +57,7 @@ public:
>         , m_characterScanForCodePath(characterScanForCodePath)
>         , m_disableSpacing(false)
>     {
>+        ASSERT(!m_text.isNull());
>     }
> 
>     explicit TextRun(StringView stringView, float xpos = 0, float expansion = 0, ExpansionBehavior expansionBehavior = DefaultExpansion, TextDirection direction = TextDirection::LTR, bool directionalOverride = false, bool characterScanForCodePath = true)
>@@ -89,7 +90,7 @@ public:
> 
>     void setText(const LChar* text, unsigned length) { setText({ text, length }); }
>     void setText(const UChar* text, unsigned length) { setText({ text, length }); }
>-    void setText(StringView text) { m_text = text.toStringWithoutCopying(); }
>+    void setText(StringView text) { ASSERT(!text.isNull()); m_text = text.toStringWithoutCopying(); }
> 
>     float horizontalGlyphStretch() const { return m_horizontalGlyphStretch; }
>     void setHorizontalGlyphStretch(float scale) { m_horizontalGlyphStretch = scale; }
>diff --git a/Source/WebCore/rendering/RenderListBox.cpp b/Source/WebCore/rendering/RenderListBox.cpp
>index 36c1aed0d04195a1feeff2a1647daf9c1a3c86ee..ecd55165ffc819c5b326b3e76a25048d1504ab1d 100644
>--- a/Source/WebCore/rendering/RenderListBox.cpp
>+++ b/Source/WebCore/rendering/RenderListBox.cpp
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2006, 2007, 2008, 2011, 2014-2015 Apple Inc. All rights reserved.
>+ * Copyright (C) 2006-2018 Apple Inc. All rights reserved.
>  *               2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
>  *
>  * Redistribution and use in source and binary forms, with or without
>@@ -422,6 +422,9 @@ void RenderListBox::paintItemForeground(PaintInfo& paintInfo, const LayoutPoint&
>         itemText = downcast<HTMLOptGroupElement>(*listItemElement).groupLabelText();
>     itemText = applyTextTransform(style(), itemText, ' ');
> 
>+    if (itemText.isNull())
>+        return;
>+
>     Color textColor = itemStyle.visitedDependentColorWithColorFilter(CSSPropertyColor);
>     if (isOptionElement && downcast<HTMLOptionElement>(*listItemElement).selected()) {
>         if (frame().selection().isFocusedAndActive() && document().focusedElement() == &selectElement())
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 78d1e1256ffc8a621832ee3ca64c33543143bf1c..4eeddbcce34d7955c11f54585a7fe437881f6894 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-09-04  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 Zalan Bujtas.
>+
>+        * fast/text/null-string-textrun-expected.txt: Added.
>+        * fast/text/null-string-textrun.html: Added.
>+
> 2018-09-04  Daniel Bates  <dabates@apple.com>
> 
>         Remove redundant inline text boxes for empty combined text
>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