Summary: | SVGTextRunRenderingContext can return null font, calling code asserts not null | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephen Chenney <schenney> | ||||
Component: | SVG | Assignee: | Stephen Chenney <schenney> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | eric, fmalita, gustavo, pdr, rhodovan.u-szeged, rwlbuis, webkit.review.bot, xan.lopez, zimmermann | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Stephen Chenney
2012-05-17 08:32:09 PDT
We should at least understand in what cases 'glyphDataAndPageForCharacter' from FontFastPath.cpp returns a 0 fontData. The null-checks in SVGTextRunRenderingContext::glyphDataForCharacter are misleading, as the FontFastPath glyphDataAndPageForCharacter method should never return a 0 FontData -- it should use a system fallback font instead. Maybe this can fail as well? I'm not sure, and we should check this first. Note that HTML would also be affected by this -- it uses the same glyphDataAndPageForCharacter functionality from FontFastPath.cpp, and for non-SVGFonts the ASSERT(fontData) is in since years. Created attachment 142740 [details]
Patch
Comment on attachment 142740 [details] Patch Attachment 142740 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12721868 Comment on attachment 142740 [details]
Patch
gtk failure doesn't relate to anything in this patch. No idea what's wrong there.
Comment on attachment 142740 [details]
Patch
Good catch. I don't see a link between the Gtk build problems and your patch, so r=me.
Btw, please do try to construct a testcase - even if this now goes in as quick-fix for Chromium problems, we need to make sure to never regress this again. I'd prefer to land the patch together with the testcase - if that's impractical I'm fine with this as-is, but we shouldn't forget about the test :-) I'm going to set it up to run with the acid3 test that was failing, without the real fix for that test. If it no longer crashes with this patch, I'll consider that as verifying that this change works (although I expect the actual test output may be wrong because the wrong font may get used). I honestly have no idea at all how to test it otherwise. Well, a unit test could do the job, but that would really require some work, and hence needs its own bug. I tested that this fixes the acid3 test (Bug 86715), which I take as evidence that this patch solves a problem and does not introduce a new one. As I said, unit testing is the real way to get at this, but we'll have to wait for such support for SVG. I'll commit now. Committed r117788: <http://trac.webkit.org/changeset/117788> |