[WIN] Crash when trying to access store pages
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Jul 2018 18:21:16 +0000 (18:21 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Jul 2018 18:21:16 +0000 (18:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188032
<rdar://problem/42467016>

Reviewed by Brent Fulgham.

The Windows implementation of GlyphBuffer has an additional member, m_offsets, which represents
an additional offset to the position to paint each glyph. It also has two add() functions, one
which appends to this vector, and one which doesn't. The one that doesn't append to the vector
should never be called on Windows (because Windows requires this vector to be full).

There were two situations where it was getting called:
1) Inside ComplexTextController
2) Inside display list playback

Windows shouldn't be using ComplexTextController because the Windows implementation of this
class isn't ready yet; instead it should be using UniscribeController. The display list playback
code should be used on Windows.

Rather than fix the function to append an offset, we actually don't need the m_offsets vector
in the first place. Instead, we can do it the same way that the Cocoa ports do it, which is to
bake the offsets into the glyph advances. This is possible because the GlyphBuffer doesn't need
to distinguish between layout advances and paint advances, so we can bake them together and
just put paint advances in the GlyphBuffer. This should be a small (probably within-the-noise)
performance and memory improvement.

* platform/graphics/ComplexTextController.cpp:
(WebCore::ComplexTextController::ComplexTextController): Make sure that ComplexTextController
isn't used on Windows.
* platform/graphics/FontCascade.cpp:
(WebCore::FontCascade::widthOfTextRange const): Switch from ComplexTextController to
UniscribeController on Windows.
(WebCore::FontCascade::drawGlyphBuffer const): After deleting the m_offsets vector, there's
no reason to consult it when drawing.
* platform/graphics/GlyphBuffer.h: Remove m_offsets
(WebCore::GlyphBuffer::clear):
(WebCore::GlyphBuffer::advanceAt const):
(WebCore::GlyphBuffer::add):
(WebCore::GlyphBuffer::expandLastAdvance):
(WebCore::GlyphBuffer::shrink):
(WebCore::GlyphBuffer::swap):
(WebCore::GlyphBuffer::offsetAt const): Deleted.
* platform/graphics/win/FontCGWin.cpp:
(WebCore::FontCascade::drawGlyphs): After deleting the m_offsets vector, there's no reason
to consult it when drawing.
* platform/graphics/win/FontCascadeDirect2D.cpp:
(WebCore::FontCascade::drawGlyphs): Ditto.
* platform/graphics/win/UniscribeController.cpp:
(WebCore::UniscribeController::shapeAndPlaceItem): Bake in the offsets into the glyph advances.

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/ComplexTextController.cpp
Source/WebCore/platform/graphics/FontCascade.cpp
Source/WebCore/platform/graphics/GlyphBuffer.h
Source/WebCore/platform/graphics/win/FontCGWin.cpp
Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp
Source/WebCore/platform/graphics/win/UniscribeController.cpp

index 0874f5a..81736d8 100644 (file)
@@ -1,3 +1,55 @@
+2018-07-27  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        [WIN] Crash when trying to access store pages
+        https://bugs.webkit.org/show_bug.cgi?id=188032
+        <rdar://problem/42467016>
+
+        Reviewed by Brent Fulgham.
+
+        The Windows implementation of GlyphBuffer has an additional member, m_offsets, which represents
+        an additional offset to the position to paint each glyph. It also has two add() functions, one
+        which appends to this vector, and one which doesn't. The one that doesn't append to the vector
+        should never be called on Windows (because Windows requires this vector to be full).
+
+        There were two situations where it was getting called:
+        1) Inside ComplexTextController
+        2) Inside display list playback
+
+        Windows shouldn't be using ComplexTextController because the Windows implementation of this 
+        class isn't ready yet; instead it should be using UniscribeController. The display list playback
+        code should be used on Windows.
+
+        Rather than fix the function to append an offset, we actually don't need the m_offsets vector
+        in the first place. Instead, we can do it the same way that the Cocoa ports do it, which is to
+        bake the offsets into the glyph advances. This is possible because the GlyphBuffer doesn't need
+        to distinguish between layout advances and paint advances, so we can bake them together and
+        just put paint advances in the GlyphBuffer. This should be a small (probably within-the-noise)
+        performance and memory improvement.
+
+        * platform/graphics/ComplexTextController.cpp:
+        (WebCore::ComplexTextController::ComplexTextController): Make sure that ComplexTextController
+        isn't used on Windows.
+        * platform/graphics/FontCascade.cpp:
+        (WebCore::FontCascade::widthOfTextRange const): Switch from ComplexTextController to
+        UniscribeController on Windows.
+        (WebCore::FontCascade::drawGlyphBuffer const): After deleting the m_offsets vector, there's
+        no reason to consult it when drawing.
+        * platform/graphics/GlyphBuffer.h: Remove m_offsets
+        (WebCore::GlyphBuffer::clear):
+        (WebCore::GlyphBuffer::advanceAt const):
+        (WebCore::GlyphBuffer::add):
+        (WebCore::GlyphBuffer::expandLastAdvance):
+        (WebCore::GlyphBuffer::shrink):
+        (WebCore::GlyphBuffer::swap):
+        (WebCore::GlyphBuffer::offsetAt const): Deleted.
+        * platform/graphics/win/FontCGWin.cpp:
+        (WebCore::FontCascade::drawGlyphs): After deleting the m_offsets vector, there's no reason
+        to consult it when drawing.
+        * platform/graphics/win/FontCascadeDirect2D.cpp:
+        (WebCore::FontCascade::drawGlyphs): Ditto.
+        * platform/graphics/win/UniscribeController.cpp:
+        (WebCore::UniscribeController::shapeAndPlaceItem): Bake in the offsets into the glyph advances.
+
 2018-07-27  Basuke Suzuki  <Basuke.Suzuki@sony.com>
 
         [Curl] Crash on synchronous request via ResourceHandle.
index f9f18a8..2e2bbbe 100644 (file)
@@ -144,6 +144,10 @@ ComplexTextController::ComplexTextController(const FontCascade& font, const Text
     , m_mayUseNaturalWritingDirection(mayUseNaturalWritingDirection)
     , m_forTextEmphasis(forTextEmphasis)
 {
+#if PLATFORM(WIN)
+    ASSERT_NOT_REACHED();
+#endif
+
     computeExpansionOpportunity();
 
     collectComplexTextRuns();
index 5462209..9327894 100644 (file)
 #include <wtf/text/AtomicStringHash.h>
 #include <wtf/text/StringBuilder.h>
 
+#if PLATFORM(WIN)
+#include "UniscribeController.h"
+#endif
+
 namespace WebCore {
 using namespace WTF;
 using namespace Unicode;
@@ -343,6 +347,15 @@ float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned
 
     auto codePathToUse = codePath(run);
     if (codePathToUse == Complex) {
+#if PLATFORM(WIN)
+        UniscribeController it(this, run);
+        it.advance(from);
+        offsetBeforeRange = it.runWidthSoFar();
+        it.advance(to);
+        offsetAfterRange = it.runWidthSoFar();
+        it.advance(to);
+        totalWidth = it.runWidthSoFar();
+#else
         ComplexTextController complexIterator(*this, run, false, fallbackFonts);
         complexIterator.advance(from, nullptr, IncludePartialGlyphs, fallbackFonts);
         offsetBeforeRange = complexIterator.runWidthSoFar();
@@ -350,6 +363,7 @@ float FontCascade::widthOfTextRange(const TextRun& run, unsigned from, unsigned
         offsetAfterRange = complexIterator.runWidthSoFar();
         complexIterator.advance(run.length(), nullptr, IncludePartialGlyphs, fallbackFonts);
         totalWidth = complexIterator.runWidthSoFar();
+#endif
     } else {
         WidthIterator simpleIterator(this, run, fallbackFonts);
         simpleIterator.advance(from, nullptr);
@@ -1418,7 +1432,7 @@ void FontCascade::drawGlyphBuffer(GraphicsContext& context, const GlyphBuffer& g
 {
     // Draw each contiguous run of glyphs that use the same font data.
     const Font* fontData = glyphBuffer.fontAt(0);
-    FloatSize offset = glyphBuffer.offsetAt(0);
+    // FIXME: Why do we subtract the initial advance's height but not its width???
     FloatPoint startPoint(point.x(), point.y() - glyphBuffer.initialAdvance().height());
     float nextX = startPoint.x() + glyphBuffer.advanceAt(0).width();
     float nextY = startPoint.y() + glyphBuffer.advanceAt(0).height();
@@ -1426,15 +1440,13 @@ void FontCascade::drawGlyphBuffer(GraphicsContext& context, const GlyphBuffer& g
     unsigned nextGlyph = 1;
     while (nextGlyph < glyphBuffer.size()) {
         const Font* nextFontData = glyphBuffer.fontAt(nextGlyph);
-        FloatSize nextOffset = glyphBuffer.offsetAt(nextGlyph);
 
-        if (nextFontData != fontData || nextOffset != offset) {
+        if (nextFontData != fontData) {
             if (shouldDrawIfLoading(*fontData, customFontNotReadyAction))
                 context.drawGlyphs(*fontData, glyphBuffer, lastFrom, nextGlyph - lastFrom, startPoint, m_fontDescription.fontSmoothing());
 
             lastFrom = nextGlyph;
             fontData = nextFontData;
-            offset = nextOffset;
             startPoint.setX(nextX);
             startPoint.setY(nextY);
         }
index 1752dcb..bf0e94c 100644 (file)
@@ -27,8 +27,7 @@
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#ifndef GlyphBuffer_h
-#define GlyphBuffer_h
+#pragma once
 
 #include "FloatSize.h"
 #include "Glyph.h"
@@ -85,9 +84,6 @@ public:
         m_advances.clear();
         if (m_offsetsInString)
             m_offsetsInString->clear();
-#if PLATFORM(WIN)
-        m_offsets.clear();
-#endif
     }
 
     GlyphBufferGlyph* glyphs(unsigned from) { return m_glyphs.data() + from; }
@@ -110,44 +106,17 @@ public:
     {
         return m_advances[index];
     }
-
-    FloatSize offsetAt(unsigned index) const
-    {
-#if PLATFORM(WIN)
-        return m_offsets[index];
-#else
-        UNUSED_PARAM(index);
-        return FloatSize();
-#endif
-    }
     
     static const unsigned noOffset = UINT_MAX;
-    void add(Glyph glyph, const Font* font, float width, unsigned offsetInString = noOffset, const FloatSize* offset = 0)
+    void add(Glyph glyph, const Font* font, float width, unsigned offsetInString = noOffset)
     {
-        m_font.append(font);
-        m_glyphs.append(glyph);
+        GlyphBufferAdvance advance;
+        advance.setWidth(width);
+        advance.setHeight(0);
 
-#if USE(CG)
-        CGSize advance = { width, 0 };
-        m_advances.append(advance);
-#else
-        m_advances.append(FloatSize(width, 0));
-#endif
-
-#if PLATFORM(WIN)
-        if (offset)
-            m_offsets.append(*offset);
-        else
-            m_offsets.append(FloatSize());
-#else
-        UNUSED_PARAM(offset);
-#endif
-        
-        if (offsetInString != noOffset && m_offsetsInString)
-            m_offsetsInString->append(offsetInString);
+        add(glyph, font, advance, offsetInString);
     }
-    
-#if !USE(WINGDI)
+
     void add(Glyph glyph, const Font* font, GlyphBufferAdvance advance, unsigned offsetInString = noOffset)
     {
         m_font.append(font);
@@ -158,7 +127,6 @@ public:
         if (offsetInString != noOffset && m_offsetsInString)
             m_offsetsInString->append(offsetInString);
     }
-#endif
 
     void reverse(unsigned from, unsigned length)
     {
@@ -172,6 +140,14 @@ public:
         GlyphBufferAdvance& lastAdvance = m_advances.last();
         lastAdvance.setWidth(lastAdvance.width() + width);
     }
+
+    void expandLastAdvance(GlyphBufferAdvance expansion)
+    {
+        ASSERT(!isEmpty());
+        GlyphBufferAdvance& lastAdvance = m_advances.last();
+        lastAdvance.setWidth(lastAdvance.width() + expansion.width());
+        lastAdvance.setHeight(lastAdvance.height() + expansion.height());
+    }
     
     void saveOffsetsInString()
     {
@@ -191,9 +167,6 @@ public:
         m_advances.shrink(truncationPoint);
         if (m_offsetsInString)
             m_offsetsInString->shrink(truncationPoint);
-#if PLATFORM(WIN)
-        m_offsets.shrink(truncationPoint);
-#endif
     }
 
 private:
@@ -210,12 +183,6 @@ private:
         GlyphBufferAdvance s = m_advances[index1];
         m_advances[index1] = m_advances[index2];
         m_advances[index2] = s;
-
-#if PLATFORM(WIN)
-        FloatSize offset = m_offsets[index1];
-        m_offsets[index1] = m_offsets[index2];
-        m_offsets[index2] = offset;
-#endif
     }
 
     Vector<const Font*, 2048> m_font;
@@ -223,10 +190,6 @@ private:
     Vector<GlyphBufferAdvance, 2048> m_advances;
     GlyphBufferAdvance m_initialAdvance;
     std::unique_ptr<Vector<unsigned, 2048>> m_offsetsInString;
-#if PLATFORM(WIN)
-    Vector<FloatSize, 2048> m_offsets;
-#endif
 };
 
 }
-#endif
index d70e6e4..7aaf068 100644 (file)
@@ -174,9 +174,6 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
     CGAffineTransform savedMatrix = CGContextGetTextMatrix(cgContext);
     CGContextSetTextMatrix(cgContext, matrix);
 
-    // Uniscribe gives us offsets to help refine the positioning of combining glyphs.
-    FloatSize translation = glyphBuffer.offsetAt(from);
-
     CGContextSetFontSize(cgContext, platformData.size());
     wkSetCGContextFontRenderingStyle(cgContext, font.platformData().isSystemFont(), false, font.platformData().useGDI());
 
@@ -192,22 +189,22 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
         Color fillColor = graphicsContext.fillColor();
         Color shadowFillColor(shadowColor.red(), shadowColor.green(), shadowColor.blue(), shadowColor.alpha() * fillColor.alpha() / 255);
         graphicsContext.setFillColor(shadowFillColor);
-        float shadowTextX = point.x() + translation.width() + shadowOffset.width();
+        float shadowTextX = point.x() + shadowOffset.width();
         // If shadows are ignoring transforms, then we haven't applied the Y coordinate flip yet, so down is negative.
-        float shadowTextY = point.y() + translation.height() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
+        float shadowTextY = point.y() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
         CGContextSetTextPosition(cgContext, shadowTextX, shadowTextY);
         CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
         if (font.syntheticBoldOffset()) {
-            CGContextSetTextPosition(cgContext, point.x() + translation.width() + shadowOffset.width() + font.syntheticBoldOffset(), point.y() + translation.height() + shadowOffset.height());
+            CGContextSetTextPosition(cgContext, point.x() + shadowOffset.width() + font.syntheticBoldOffset(), point.y() + shadowOffset.height());
             CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
         }
         graphicsContext.setFillColor(fillColor);
     }
 
-    CGContextSetTextPosition(cgContext, point.x() + translation.width(), point.y() + translation.height());
+    CGContextSetTextPosition(cgContext, point.x(), point.y());
     CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
     if (font.syntheticBoldOffset()) {
-        CGContextSetTextPosition(cgContext, point.x() + translation.width() + font.syntheticBoldOffset(), point.y() + translation.height());
+        CGContextSetTextPosition(cgContext, point.x() + font.syntheticBoldOffset(), point.y());
         CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
     }
 
index d18addf..b5d2c41 100644 (file)
@@ -83,9 +83,6 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
         context->SetTransform(matrix * skewMatrix);
     }
 
-    // Uniscribe gives us offsets to help refine the positioning of combining glyphs.
-    FloatSize translation = glyphBuffer.offsetAt(from);
-
     RELEASE_ASSERT(platformData.dwFont());
     RELEASE_ASSERT(platformData.dwFontFace());
 
@@ -125,9 +122,9 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
         graphicsContext.clearShadow();
         Color fillColor = graphicsContext.fillColor();
         Color shadowFillColor(shadowColor.red(), shadowColor.green(), shadowColor.blue(), shadowColor.alpha() * fillColor.alpha() / 255);
-        float shadowTextX = point.x() + translation.width() + shadowOffset.width();
+        float shadowTextX = point.x() + shadowOffset.width();
         // If shadows are ignoring transforms, then we haven't applied the Y coordinate flip yet, so down is negative.
-        float shadowTextY = point.y() + translation.height() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
+        float shadowTextY = point.y() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
 
         auto shadowBrush = graphicsContext.brushWithColor(shadowFillColor);
         context->DrawGlyphRun(D2D1::Point2F(shadowTextX, shadowTextY), &glyphRun, shadowBrush);
@@ -135,9 +132,9 @@ void FontCascade::drawGlyphs(GraphicsContext& graphicsContext, const Font& font,
             context->DrawGlyphRun(D2D1::Point2F(shadowTextX + font.syntheticBoldOffset(), shadowTextY), &glyphRun, shadowBrush);
     }
 
-    context->DrawGlyphRun(D2D1::Point2F(point.x() + translation.width(), point.y() + translation.height()), &glyphRun, graphicsContext.solidFillBrush());
+    context->DrawGlyphRun(D2D1::Point2F(point.x(), point.y()), &glyphRun, graphicsContext.solidFillBrush());
     if (font.syntheticBoldOffset())
-        context->DrawGlyphRun(D2D1::Point2F(point.x() + translation.width() + font.syntheticBoldOffset(), point.y() + translation.height()), &glyphRun, graphicsContext.solidFillBrush());
+        context->DrawGlyphRun(D2D1::Point2F(point.x() + font.syntheticBoldOffset(), point.y()), &glyphRun, graphicsContext.solidFillBrush());
 
     if (hasSimpleShadow)
         graphicsContext.setShadow(shadowOffset, shadowBlur, shadowColor);
index 35d7935..a014773 100644 (file)
@@ -364,8 +364,13 @@ bool UniscribeController::shapeAndPlaceItem(const UChar* cp, unsigned i, const F
         // as well, so that when the time comes to draw those glyphs, we can apply the appropriate
         // translation.
         if (glyphBuffer) {
-            FloatSize size(offsetX, -offsetY);
-            glyphBuffer->add(glyph, fontData, advance, GlyphBuffer::noOffset, &size);
+            GlyphBufferAdvance origin(offsetX, -offsetY);
+            if (!glyphBuffer->advancesCount())
+                glyphBuffer->setInitialAdvance(origin);
+            else
+                glyphBuffer->expandLastAdvance(origin);
+            GlyphBufferAdvance advance(-origin.width() + advance, -origin.height());
+            glyphBuffer->add(glyph, fontData, advance);
         }
 
         FloatRect glyphBounds = fontData->boundsForGlyph(glyph);