[webkit-reviews] review denied: [Bug 26088] large negative letter-spacing crashes the chromium port : [Attachment 32025] revised patch w/ brett's improvements

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Dirk Pranke
<dpranke at chromium.org>'s request for review:
Bug 26088: large negative letter-spacing crashes the chromium port
https://bugs.webkit.org/show_bug.cgi?id=26088

Attachment 32025: revised patch w/ brett's improvements
https://bugs.webkit.org/attachment.cgi?id=32025&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> 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.