Bug 26088 – large negative letter-spacing crashes the chromium port

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Dirk Pranke]

from http://code.google.com/p/chromium/issues/detail?id=10977 , the above URL crashes chromium. 

per dglazkov@, closing the chromium bug as upstream and filing this bug instead.

[Dirk Pranke]

Created attachment 30792 [details]
test case for large negative letter-spacing.

[Dirk Pranke]

hmm ... okay, the test case reduction only seems to crash a debug build. Will continue looking into it further.

[Dirk Pranke]

Created attachment 31892 [details]
revised test reduction to reproduce the crash.

okay, here's a revised test case reduction that does actually reproduce the crash.

[Dirk Pranke]

Created attachment 31955 [details]
patch w/ fixes

submitting patch ...

[Dirk Pranke]

Darin / Dmitri, you might want Brett to review this fix as well ...

[Darin Fisher (:fishd, Google)]

Yes, I'd like Brett to say "LGTM" before I R+ this.  Thanks!

[Brett Wilson (Google)]

Comment on attachment 31955 [details]
patch w/ fixes

> +            // #10977 - very large positive or negative runs can fail to 

I'm more familiar with "bug" rather than "#"

>      m_drawContext->fillRect(IntRect(IntPoint(0, 0), m_layerSize), Color::white);
> -    // Layer rect represents the part of the original layer.
> +	// Layer rect represents the part of the original layer.

Looks like you got some space in there.


>  void TransparencyWin::setupTransform(const IntRect& region)
> @@ -289,6 +296,9 @@ void TransparencyWin::setupTransform(con
>  
>  void TransparencyWin::setupTransformForKeepTransform(const IntRect& region)
>  {
> +    if (!m_validLayer)
> +        return;
> +
>      if (m_layerMode != NoLayer) {
>          // Need to save things since we're modifying the transform.
>          m_drawContext->save();
> @@ -319,6 +329,9 @@ void TransparencyWin::setupTransformForU
>  
>  void TransparencyWin::setupTransformForScaleTransform()
>  {
> +    if (!m_validLayer)
> +        return;
> +
>      if (m_layerMode == NoLayer) {
>          // Need to save things since we're modifying the layer.
>          m_drawContext->save();
> @@ -345,16 +358,22 @@ void TransparencyWin::setTextCompositeCo
>  void TransparencyWin::initializeNewContext()
>  {
>      int pixelSize = m_layerSize.width() * m_layerSize.height();
> -    if (pixelSize > maxCachedBufferPixelSize) {
> +    if (pixelSize < 0) 
> +        return;

Do you think pixelSize <= 0 would also be good? Windows will actually fail to allocated a 0x0 bitmap, so this may prevent some problems.


LGTM otherwise.

[Brett Wilson (Google)]

Re: "#10977" this is the Chromium bug number. You should reference the WebKit bug number instead. This references the Chromium bug number in case anybody cares.

[Dirk Pranke]

Created attachment 32025 [details]
revised patch w/ brett's improvements

[Darin Fisher (:fishd, Google)]

Comment on attachment 32025 [details]
revised patch w/ brett's improvements

> Index: WebCore/ChangeLog
...
> +        Fix bug #26088 - TransparencyWin doesn't handle errors well at all;
> +        revise it to fail silently (drawing nothing).

nit: please include an URL to this bug report in the ChangeLog.  (that is what
is normally done since it makes it easy to visit the bug from the trac log.)

It looks like this patch is trying to handle out-of-memory errors for small
fixed-size allocations.  That is something we normally do not do since if we
cannot allocate a few bytes, then we are happy to crash.

Handling out-of-memory cases that result from web content, where web content
has control over the allocation size, is another story.


> Index: WebCore/platform/graphics/chromium/FontChromiumWin.cpp
> ===================================================================
> --- WebCore/platform/graphics/chromium/FontChromiumWin.cpp	(revision 45284)
> +++ WebCore/platform/graphics/chromium/FontChromiumWin.cpp	(working copy)
> @@ -149,14 +149,16 @@ void TransparencyAwareFontPainter::initi
>      m_transparency.init(m_graphicsContext, layerMode, TransparencyWin::KeepTransform, layerRect);
>  
>      // Set up the DC, using the one from the transparency helper.
> +    if (m_transparency.platformContext()) {

This looks like it is checking out-of-memory.  Why do we need to recover from
this condition?  Was this case being hit by those webkit tests?


> +            
> +            // webkit bug 26088 - very large positive or negative runs can fail
> +            // to render so we clamp the size here. In the specs, negative 
> +            // letter-spacing is implementation-defined, so this should be 
> +            // fine, and it matches Safari's implementation. The call actually
> +            // seems to crash if kMaxNegativeRun is set to somewhere around 
> +            // -32830, so we give ourselves a little breathing room.
> +            const int kMaxNegativeRun = -32768;
> +            const int kMaxPositiveRun =  32768;

webkit style is to not have the "k" prefix.  you should just use regular
variable naming style for constants.


> +            if ((curWidth + advances[i] < kMaxNegativeRun)
> +             || (curWidth + advances[i] > kMaxPositiveRun)) 

nit: indentation is 4 white spaces.  i know this means that the two
lines will not line-up as you desire, but that's what webkit style
calls for.  (normally, people would not wrap this line.)


> Index: WebCore/platform/graphics/chromium/TransparencyWin.h
...
> +    PlatformGraphicsContext* platformContext() const { return m_drawContext ? m_drawContext->platformContext() : 0; }

Is this really worth null checking?


> Index: LayoutTests/fast/text/text-large-negative-letter-spacing-with-opacity.html
...
> +<p>Test case for 
> +<a href="https://bugs.webkit.org/show_bug.cgi?id=26088" 
> +   >https://bugs.webkit.org/show_bug.cgi?id=26088</a>, 
> +which would crash the chromium port. If the browser does not crash, you should
> +see an partially-transparent "world" on the next line.</p>
> +<p style="opacity: 0.5"><span style="letter-spacing: -1000em" 
> +  >Hello</span>world</p>

layout tests that verify crash fixes should use dumpAsText since there is no
need for pixel comparisons.


> Index: LayoutTests/fast/text/text-letter-spacing.html
...
> +which are explicitly implementation-dependent. Different browsers will
> +render these differently, but Chromium and Safari are attempting to be
> +the same.</p>

It seems a bit presumptuous to state what Safari is trying to do.  It is
probably better to just say WebKit here instead of calling out particular
WebKit-based apps especially since the claim is that those apps behave
the same way.

[Dirk Pranke]

Created attachment 32153 [details]
revised patch w/ fishd's comments

Add comments about why TransparencyWin might fail to initialize, clean up test cases, conform better to WebKit coding style guidelines.

[Darin Fisher (:fishd, Google)]

Comment on attachment 32153 [details]
revised patch w/ fishd's comments

> Index: WebCore/platform/graphics/chromium/FontChromiumWin.cpp
...
> +    // bug #26088 - init() might fail if layerRect is invalid. Given this,
...
> +            // webkit bug 26088 - very large positive or negative runs can fail

nit: would be nice to use a consistent way to refer to bug numbers.  no need
to say webkit since that can be assumed given that this code is part of webkit.

R=me

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/45482

[Dirk Pranke]

Created attachment 32397 [details]
add pixel tests for LayoutTests/fast/text/text-letter-spacing.html, move generic test for text-large-negative-spaccing

adding the missing pixel tests, moving the generic crash output (dumpAsText) alongside the input file

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/45611