Crash when selecting a HarfBuzz text run with SVG fonts included

[Stephen Chenney]

When an SVG font is used in a text run, and "text-rendering: optimizelegibility;" is used (or there is kerning or ligature information on the font), and the text is selected, we crash.

This is a linux only bug, I believe, or at least HarfBuzz backed by Skia only.

There are two problems:
- An assert not reached in SimpleFontData::applyTransform that is too assertive. SVG fonts may come through this path and it is not an error for them to do so.
- The code in HarfBuzzShaper::shapeHarfBuzzRuns assumes it is always dealing with HarfBuxx font data, which is not the case for SVG fonts.

Patch shortly to prevent the crash. We still do not select properly in such cases, or on other platforms.

[Stephen Chenney]

Created attachment 188353 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188353 [details]
Patch

Attachment 188353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16538665

New failing tests:
svg/css/font-face-crash.html

[WebKit Review Bot]

Comment on attachment 188353 [details]
Patch

Attachment 188353 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16542657

New failing tests:
svg/css/font-face-crash.html

[Stephen Chenney]

Created attachment 188382 [details]
Patch

[Tony Chang]

Comment on attachment 188382 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188382&action=review

> LayoutTests/svg/css/font-face-crash.html:17
> +      if (window.testRunner)
> +        testRunner.waitUntilDone();

Can we add testRunner.dumpAsText() and share the results?

[Stephen Chenney]

(In reply to comment #5)
> (From update of attachment 188382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188382&action=review
> 
> > LayoutTests/svg/css/font-face-crash.html:17
> > +      if (window.testRunner)
> > +        testRunner.waitUntilDone();
> 
> Can we add testRunner.dumpAsText() and share the results?

It only fails if we paint (trying to paint the selection rect) and I don't think we can construct a reliable ref-test due to fonts and the selection rect (when we end up having the right selection rect).

[Stephen Chenney]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 188382 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=188382&action=review
> > 
> > > LayoutTests/svg/css/font-face-crash.html:17
> > > +      if (window.testRunner)
> > > +        testRunner.waitUntilDone();
> > 
> > Can we add testRunner.dumpAsText() and share the results?
> 
> It only fails if we paint (trying to paint the selection rect) and I don't think we can construct a reliable ref-test due to fonts and the selection rect (when we end up having the right selection rect).

And we need the timeout too.

[Tony Chang]

dumpAsText() tests still paint, we just don't compare the pixels when we're done.  I think your test will still crash with dumpAsText().

Also, you can use dumpAsText and waitUntilDone together.

[Tony Chang]

(In reply to comment #8)
> dumpAsText() tests still paint, we just don't compare the pixels when we're done.  I think your test will still crash with dumpAsText().

I verified locally on my Linux machine that we still crash on this test (release build) with dumpAsText() and without the code change.

[Stephen Chenney]

Created attachment 188435 [details]
Patch

It does paint after all. I must have had some other issue when it was earlier faiiing to crash.

[WebKit Review Bot]

Comment on attachment 188435 [details]
Patch

Clearing flags on attachment: 188435

Committed r142928: <http://trac.webkit.org/changeset/142928>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.