Bug 26088 - large negative letter-spacing crashes the chromium port
Summary: large negative letter-spacing crashes the chromium port
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Dirk Pranke
URL: http://www1.travelalberta.com/en-ab/i...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-29 15:14 PDT by Dirk Pranke
Modified: 2009-07-07 16:45 PDT (History)
3 users (show)

See Also:


Attachments
test case for large negative letter-spacing. (322 bytes, text/html)
2009-05-29 15:16 PDT, Dirk Pranke
no flags Details
revised test reduction to reproduce the crash. (364 bytes, text/html)
2009-06-25 17:19 PDT, Dirk Pranke
no flags Details
patch w/ fixes (191.37 KB, patch)
2009-06-26 15:29 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
revised patch w/ brett's improvements (191.25 KB, patch)
2009-06-29 16:23 PDT, Dirk Pranke
fishd: review-
Details | Formatted Diff | Diff
revised patch w/ fishd's comments (31.47 KB, patch)
2009-07-01 15:49 PDT, Dirk Pranke
fishd: review+
Details | Formatted Diff | Diff
add pixel tests for LayoutTests/fast/text/text-letter-spacing.html, move generic test for text-large-negative-spaccing (122.88 KB, patch)
2009-07-07 15:28 PDT, Dirk Pranke
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2009-05-29 15:14:54 PDT
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.
Comment 1 Dirk Pranke 2009-05-29 15:16:39 PDT
Created attachment 30792 [details]
test case for large negative letter-spacing.
Comment 2 Dirk Pranke 2009-05-29 15:21:01 PDT
hmm ... okay, the test case reduction only seems to crash a debug build. Will continue looking into it further.
Comment 3 Dirk Pranke 2009-06-25 17:19:24 PDT
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.
Comment 4 Dirk Pranke 2009-06-26 15:29:05 PDT
Created attachment 31955 [details]
patch w/ fixes

submitting patch ...
Comment 5 Dirk Pranke 2009-06-26 15:30:21 PDT
Darin / Dmitri, you might want Brett to review this fix as well ...
Comment 6 Darin Fisher (:fishd, Google) 2009-06-29 12:30:29 PDT
Yes, I'd like Brett to say "LGTM" before I R+ this.  Thanks!
Comment 7 Brett Wilson (Google) 2009-06-29 12:52:44 PDT
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.
Comment 8 Brett Wilson (Google) 2009-06-29 12:53:52 PDT
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.
Comment 9 Dirk Pranke 2009-06-29 16:23:26 PDT
Created attachment 32025 [details]
revised patch w/ brett's improvements
Comment 10 Darin Fisher (:fishd, Google) 2009-07-01 00:00:58 PDT
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.
Comment 11 Dirk Pranke 2009-07-01 15:49:31 PDT
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.
Comment 12 Darin Fisher (:fishd, Google) 2009-07-02 09:18:55 PDT
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
Comment 13 Darin Fisher (:fishd, Google) 2009-07-02 11:08:01 PDT
Landed as:  http://trac.webkit.org/changeset/45482
Comment 14 Dirk Pranke 2009-07-07 15:28:24 PDT
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
Comment 15 Darin Fisher (:fishd, Google) 2009-07-07 16:45:04 PDT
Landed as:  http://trac.webkit.org/changeset/45611