Source/WebCore:
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Sep 2018 04:07:53 +0000 (04:07 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Sep 2018 04:07:53 +0000 (04:07 +0000)
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.

Source/WTF:
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):

LayoutTests:
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.

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

LayoutTests/ChangeLog
LayoutTests/fast/text/null-string-textrun-expected.txt [new file with mode: 0644]
LayoutTests/fast/text/null-string-textrun.html [new file with mode: 0644]
Source/WTF/ChangeLog
Source/WTF/wtf/text/WTFString.h
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/FontCascade.cpp
Source/WebCore/platform/graphics/TextRun.h
Source/WebCore/rendering/RenderListBox.cpp

index 60be969..5d50e7c 100644 (file)
@@ -1,3 +1,14 @@
+2018-09-05  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-05  Youenn Fablet  <youenn@apple.com>
 
         Expose RTCRtpSender.setParameters
diff --git a/LayoutTests/fast/text/null-string-textrun-expected.txt b/LayoutTests/fast/text/null-string-textrun-expected.txt
new file mode 100644 (file)
index 0000000..99d8c89
--- /dev/null
@@ -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 (file)
index 0000000..b145900
--- /dev/null
@@ -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
index 7696dac..b4983ab 100644 (file)
@@ -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-09-05  Mark Lam  <mark.lam@apple.com>
 
         Remove unused bad_optional_access implementation.
index b08be53..ddedefd 100644 (file)
@@ -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; }
 
index 90b6832..d1e9db6 100644 (file)
@@ -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-05  Zalan Bujtas  <zalan@apple.com>
 
         [LFC][BFC] ComputeFloat* methods should take a const FloatingContext&
index 7648f5b..0869253 100644 (file)
@@ -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.
index d39bcf7..e2bb4be 100644 (file)
@@ -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; }
index 36c1aed..ecd5516 100644 (file)
@@ -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())