Changeset 234318 in webkit


Ignore:
Timestamp:
Jul 27, 2018, 11:21:16 AM (6 years ago)
Author:
mmaxfield@apple.com
Message:

[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.

Location:
trunk/Source/WebCore
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r234317 r234318  
     12018-07-27  Myles C. Maxfield  <mmaxfield@apple.com>
     2
     3        [WIN] Crash when trying to access store pages
     4        https://bugs.webkit.org/show_bug.cgi?id=188032
     5        <rdar://problem/42467016>
     6
     7        Reviewed by Brent Fulgham.
     8
     9        The Windows implementation of GlyphBuffer has an additional member, m_offsets, which represents
     10        an additional offset to the position to paint each glyph. It also has two add() functions, one
     11        which appends to this vector, and one which doesn't. The one that doesn't append to the vector
     12        should never be called on Windows (because Windows requires this vector to be full).
     13
     14        There were two situations where it was getting called:
     15        1) Inside ComplexTextController
     16        2) Inside display list playback
     17
     18        Windows shouldn't be using ComplexTextController because the Windows implementation of this
     19        class isn't ready yet; instead it should be using UniscribeController. The display list playback
     20        code should be used on Windows.
     21
     22        Rather than fix the function to append an offset, we actually don't need the m_offsets vector
     23        in the first place. Instead, we can do it the same way that the Cocoa ports do it, which is to
     24        bake the offsets into the glyph advances. This is possible because the GlyphBuffer doesn't need
     25        to distinguish between layout advances and paint advances, so we can bake them together and
     26        just put paint advances in the GlyphBuffer. This should be a small (probably within-the-noise)
     27        performance and memory improvement.
     28
     29        * platform/graphics/ComplexTextController.cpp:
     30        (WebCore::ComplexTextController::ComplexTextController): Make sure that ComplexTextController
     31        isn't used on Windows.
     32        * platform/graphics/FontCascade.cpp:
     33        (WebCore::FontCascade::widthOfTextRange const): Switch from ComplexTextController to
     34        UniscribeController on Windows.
     35        (WebCore::FontCascade::drawGlyphBuffer const): After deleting the m_offsets vector, there's
     36        no reason to consult it when drawing.
     37        * platform/graphics/GlyphBuffer.h: Remove m_offsets
     38        (WebCore::GlyphBuffer::clear):
     39        (WebCore::GlyphBuffer::advanceAt const):
     40        (WebCore::GlyphBuffer::add):
     41        (WebCore::GlyphBuffer::expandLastAdvance):
     42        (WebCore::GlyphBuffer::shrink):
     43        (WebCore::GlyphBuffer::swap):
     44        (WebCore::GlyphBuffer::offsetAt const): Deleted.
     45        * platform/graphics/win/FontCGWin.cpp:
     46        (WebCore::FontCascade::drawGlyphs): After deleting the m_offsets vector, there's no reason
     47        to consult it when drawing.
     48        * platform/graphics/win/FontCascadeDirect2D.cpp:
     49        (WebCore::FontCascade::drawGlyphs): Ditto.
     50        * platform/graphics/win/UniscribeController.cpp:
     51        (WebCore::UniscribeController::shapeAndPlaceItem): Bake in the offsets into the glyph advances.
     52
    1532018-07-27  Basuke Suzuki  <Basuke.Suzuki@sony.com>
    254

  • trunk/Source/WebCore/platform/graphics/ComplexTextController.cpp

    r229400 r234318  
    145145    , m_forTextEmphasis(forTextEmphasis)
    146146{
     147#if PLATFORM(WIN)
     148    ASSERT_NOT_REACHED();
     149#endif
     150
    147151    computeExpansionOpportunity();
    148152

  • trunk/Source/WebCore/platform/graphics/FontCascade.cpp

    r233851 r234318  
    4242#include <wtf/text/StringBuilder.h>
    4343
     44#if PLATFORM(WIN)
     45#include "UniscribeController.h"
     46#endif
     47
    4448namespace WebCore {
    4549using namespace WTF;
    … …  
    344348    auto codePathToUse = codePath(run);
    345349    if (codePathToUse == Complex) {
     350#if PLATFORM(WIN)
     351        UniscribeController it(this, run);
     352        it.advance(from);
     353        offsetBeforeRange = it.runWidthSoFar();
     354        it.advance(to);
     355        offsetAfterRange = it.runWidthSoFar();
     356        it.advance(to);
     357        totalWidth = it.runWidthSoFar();
     358#else
    346359        ComplexTextController complexIterator(*this, run, false, fallbackFonts);
    347360        complexIterator.advance(from, nullptr, IncludePartialGlyphs, fallbackFonts);
    … …  
    351364        complexIterator.advance(run.length(), nullptr, IncludePartialGlyphs, fallbackFonts);
    352365        totalWidth = complexIterator.runWidthSoFar();
     366#endif
    353367    } else {
    354368        WidthIterator simpleIterator(this, run, fallbackFonts);
    … …  
    14191433    // Draw each contiguous run of glyphs that use the same font data.
    14201434    const Font* fontData = glyphBuffer.fontAt(0);
    1421     FloatSize offset = glyphBuffer.offsetAt(0);
     1435    // FIXME: Why do we subtract the initial advance's height but not its width???
    14221436    FloatPoint startPoint(point.x(), point.y() - glyphBuffer.initialAdvance().height());
    14231437    float nextX = startPoint.x() + glyphBuffer.advanceAt(0).width();
    … …  
    14271441    while (nextGlyph < glyphBuffer.size()) {
    14281442        const Font* nextFontData = glyphBuffer.fontAt(nextGlyph);
    1429         FloatSize nextOffset = glyphBuffer.offsetAt(nextGlyph);
    1430 
    1431         if (nextFontData != fontData || nextOffset != offset) {
     1443
     1444        if (nextFontData != fontData) {
    14321445            if (shouldDrawIfLoading(*fontData, customFontNotReadyAction))
    14331446                context.drawGlyphs(*fontData, glyphBuffer, lastFrom, nextGlyph - lastFrom, startPoint, m_fontDescription.fontSmoothing());
    … …  
    14351448            lastFrom = nextGlyph;
    14361449            fontData = nextFontData;
    1437             offset = nextOffset;
    14381450            startPoint.setX(nextX);
    14391451            startPoint.setY(nextY);

  • trunk/Source/WebCore/platform/graphics/GlyphBuffer.h

    r222610 r234318  
    2828 */
    2929
    30 #ifndef GlyphBuffer_h
    31 #define GlyphBuffer_h
     30#pragma once
    3231
    3332#include "FloatSize.h"
    … …  
    8685        if (m_offsetsInString)
    8786            m_offsetsInString->clear();
    88 #if PLATFORM(WIN)
    89         m_offsets.clear();
    90 #endif
    9187    }
    9288
    … …  
    111107        return m_advances[index];
    112108    }
    113 
    114     FloatSize offsetAt(unsigned index) const
    115     {
    116 #if PLATFORM(WIN)
    117         return m_offsets[index];
    118 #else
    119         UNUSED_PARAM(index);
    120         return FloatSize();
    121 #endif
    122     }
    123109   
    124110    static const unsigned noOffset = UINT_MAX;
    125     void add(Glyph glyph, const Font* font, float width, unsigned offsetInString = noOffset, const FloatSize* offset = 0)
     111    void add(Glyph glyph, const Font* font, float width, unsigned offsetInString = noOffset)
    126112    {
    127         m_font.append(font);
    128         m_glyphs.append(glyph);
     113        GlyphBufferAdvance advance;
     114        advance.setWidth(width);
     115        advance.setHeight(0);
    129116
    130 #if USE(CG)
    131         CGSize advance = { width, 0 };
    132         m_advances.append(advance);
    133 #else
    134         m_advances.append(FloatSize(width, 0));
    135 #endif
     117        add(glyph, font, advance, offsetInString);
     118    }
    136119
    137 #if PLATFORM(WIN)
    138         if (offset)
    139             m_offsets.append(*offset);
    140         else
    141             m_offsets.append(FloatSize());
    142 #else
    143         UNUSED_PARAM(offset);
    144 #endif
    145        
    146         if (offsetInString != noOffset && m_offsetsInString)
    147             m_offsetsInString->append(offsetInString);
    148     }
    149    
    150 #if !USE(WINGDI)
    151120    void add(Glyph glyph, const Font* font, GlyphBufferAdvance advance, unsigned offsetInString = noOffset)
    152121    {
    … …  
    159128            m_offsetsInString->append(offsetInString);
    160129    }
    161 #endif
    162130
    163131    void reverse(unsigned from, unsigned length)
    … …  
    172140        GlyphBufferAdvance& lastAdvance = m_advances.last();
    173141        lastAdvance.setWidth(lastAdvance.width() + width);
     142    }
     143
     144    void expandLastAdvance(GlyphBufferAdvance expansion)
     145    {
     146        ASSERT(!isEmpty());
     147        GlyphBufferAdvance& lastAdvance = m_advances.last();
     148        lastAdvance.setWidth(lastAdvance.width() + expansion.width());
     149        lastAdvance.setHeight(lastAdvance.height() + expansion.height());
    174150    }
    175151   
    … …  
    192168        if (m_offsetsInString)
    193169            m_offsetsInString->shrink(truncationPoint);
    194 #if PLATFORM(WIN)
    195         m_offsets.shrink(truncationPoint);
    196 #endif
    197170    }
    198171
    … …  
    211184        m_advances[index1] = m_advances[index2];
    212185        m_advances[index2] = s;
    213 
    214 #if PLATFORM(WIN)
    215         FloatSize offset = m_offsets[index1];
    216         m_offsets[index1] = m_offsets[index2];
    217         m_offsets[index2] = offset;
    218 #endif
    219186    }
    220187
    … …  
    224191    GlyphBufferAdvance m_initialAdvance;
    225192    std::unique_ptr<Vector<unsigned, 2048>> m_offsetsInString;
    226 #if PLATFORM(WIN)
    227     Vector<FloatSize, 2048> m_offsets;
    228 #endif
    229193};
    230194
    231195}
    232 #endif

  • trunk/Source/WebCore/platform/graphics/win/FontCGWin.cpp

    r233851 r234318  
    175175    CGContextSetTextMatrix(cgContext, matrix);
    176176
    177     // Uniscribe gives us offsets to help refine the positioning of combining glyphs.
    178     FloatSize translation = glyphBuffer.offsetAt(from);
    179 
    180177    CGContextSetFontSize(cgContext, platformData.size());
    181178    wkSetCGContextFontRenderingStyle(cgContext, font.platformData().isSystemFont(), false, font.platformData().useGDI());
    … …  
    193190        Color shadowFillColor(shadowColor.red(), shadowColor.green(), shadowColor.blue(), shadowColor.alpha() * fillColor.alpha() / 255);
    194191        graphicsContext.setFillColor(shadowFillColor);
    195         float shadowTextX = point.x() + translation.width() + shadowOffset.width();
     192        float shadowTextX = point.x() + shadowOffset.width();
    196193        // If shadows are ignoring transforms, then we haven't applied the Y coordinate flip yet, so down is negative.
    197         float shadowTextY = point.y() + translation.height() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
     194        float shadowTextY = point.y() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
    198195        CGContextSetTextPosition(cgContext, shadowTextX, shadowTextY);
    199196        CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
    200197        if (font.syntheticBoldOffset()) {
    201             CGContextSetTextPosition(cgContext, point.x() + translation.width() + shadowOffset.width() + font.syntheticBoldOffset(), point.y() + translation.height() + shadowOffset.height());
     198            CGContextSetTextPosition(cgContext, point.x() + shadowOffset.width() + font.syntheticBoldOffset(), point.y() + shadowOffset.height());
    202199            CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
    203200        }
    … …  
    205202    }
    206203
    207     CGContextSetTextPosition(cgContext, point.x() + translation.width(), point.y() + translation.height());
     204    CGContextSetTextPosition(cgContext, point.x(), point.y());
    208205    CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
    209206    if (font.syntheticBoldOffset()) {
    210         CGContextSetTextPosition(cgContext, point.x() + translation.width() + font.syntheticBoldOffset(), point.y() + translation.height());
     207        CGContextSetTextPosition(cgContext, point.x() + font.syntheticBoldOffset(), point.y());
    211208        CGContextShowGlyphsWithAdvances(cgContext, glyphBuffer.glyphs(from), static_cast<const CGSize*>(glyphBuffer.advances(from)), numGlyphs);
    212209    }

  • trunk/Source/WebCore/platform/graphics/win/FontCascadeDirect2D.cpp

    r233851 r234318  
    8484    }
    8585
    86     // Uniscribe gives us offsets to help refine the positioning of combining glyphs.
    87     FloatSize translation = glyphBuffer.offsetAt(from);
    88 
    8986    RELEASE_ASSERT(platformData.dwFont());
    9087    RELEASE_ASSERT(platformData.dwFontFace());
    … …  
    126123        Color fillColor = graphicsContext.fillColor();
    127124        Color shadowFillColor(shadowColor.red(), shadowColor.green(), shadowColor.blue(), shadowColor.alpha() * fillColor.alpha() / 255);
    128         float shadowTextX = point.x() + translation.width() + shadowOffset.width();
     125        float shadowTextX = point.x() + shadowOffset.width();
    129126        // If shadows are ignoring transforms, then we haven't applied the Y coordinate flip yet, so down is negative.
    130         float shadowTextY = point.y() + translation.height() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
     127        float shadowTextY = point.y() + shadowOffset.height() * (graphicsContext.shadowsIgnoreTransforms() ? -1 : 1);
    131128
    132129        auto shadowBrush = graphicsContext.brushWithColor(shadowFillColor);
    … …  
    136133    }
    137134
    138     context->DrawGlyphRun(D2D1::Point2F(point.x() + translation.width(), point.y() + translation.height()), &glyphRun, graphicsContext.solidFillBrush());
     135    context->DrawGlyphRun(D2D1::Point2F(point.x(), point.y()), &glyphRun, graphicsContext.solidFillBrush());
    139136    if (font.syntheticBoldOffset())
    140         context->DrawGlyphRun(D2D1::Point2F(point.x() + translation.width() + font.syntheticBoldOffset(), point.y() + translation.height()), &glyphRun, graphicsContext.solidFillBrush());
     137        context->DrawGlyphRun(D2D1::Point2F(point.x() + font.syntheticBoldOffset(), point.y()), &glyphRun, graphicsContext.solidFillBrush());
    141138
    142139    if (hasSimpleShadow)

  • trunk/Source/WebCore/platform/graphics/win/UniscribeController.cpp

    r232932 r234318  
    365365        // translation.
    366366        if (glyphBuffer) {
    367             FloatSize size(offsetX, -offsetY);
    368             glyphBuffer->add(glyph, fontData, advance, GlyphBuffer::noOffset, &size);
     367            GlyphBufferAdvance origin(offsetX, -offsetY);
     368            if (!glyphBuffer->advancesCount())
     369                glyphBuffer->setInitialAdvance(origin);
     370            else
     371                glyphBuffer->expandLastAdvance(origin);
     372            GlyphBufferAdvance advance(-origin.width() + advance, -origin.height());
     373            glyphBuffer->add(glyph, fontData, advance);
    369374        }
    370375
Note: See TracChangeset for help on using the changeset viewer.