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.
Created attachment 30792 [details] test case for large negative letter-spacing.
hmm ... okay, the test case reduction only seems to crash a debug build. Will continue looking into it further.
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.
Created attachment 31955 [details] patch w/ fixes submitting patch ...
Darin / Dmitri, you might want Brett to review this fix as well ...
Yes, I'd like Brett to say "LGTM" before I R+ this. Thanks!
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.
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.
Created attachment 32025 [details] revised patch w/ brett's improvements
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.
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 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
Landed as: http://trac.webkit.org/changeset/45482
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
Landed as: http://trac.webkit.org/changeset/45611