86738 2012-05-17 08:32:09 -0700 SVGTextRunRenderingContext can return null font, calling code asserts not null 2012-05-21 09:06:03 -0700 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 schenney schenney eric fmalita gustavo pdr rhodovan.u-szeged rwlbuis webkit.review.bot xan.lopez zimmermann oldest_to_newest 626599 0 schenney 2012-05-17 08:32:09 -0700 The method SVGTextRunRenderingContext::glyphDataForCharacter has this code: const SimpleFontData* primaryFont = font.primaryFont(); ASSERT(primaryFont); pair<GlyphData, GlyphPage*> pair = font.glyphDataAndPageForCharacter(character, mirror); GlyphData glyphData = pair.first; if (!glyphData.fontData) return glyphData; GlyphData missingGlyphData = primaryFont->missingGlyphData(); if (glyphData.glyph == missingGlyphData.glyph && glyphData.fontData == missingGlyphData.fontData ) return glyphData; Note that if glyphData.fontData is null the glyphData is returned as is. The calling code is in WidthIterator::advance while (textIterator.consume(character, clusterLength)) { unsigned advanceLength = clusterLength; const GlyphData& glyphData = glyphDataForCharacter(character, rtl, textIterator.currentCharacter(), advanceLength); Glyph glyph = glyphData.glyph; const SimpleFontData* fontData = glyphData.fontData; ASSERT(fontData); // Now that we have a glyph and font data, get its width. float width; if (character == '\t' && m_run.allowTabs()) width = m_font->tabWidth(*fontData, m_run.tabSize(), m_run.xPos() + m_runWidthSoFar + widthSinceLastRounding); else { width = fontData->widthForGlyph(glyph); We have a problem here because fontData will be null in some cases, and this gets by the assert in release builds, and then crashes when the null font is used. I have no idea at all how to test this, but it may be the cause, or a related symptom, of a top Chromium crasher: http://code.google.com/p/chromium/issues/detail?id=128418 I'll get a patch up shortly that I would like to put in.despite not having a test. It seems this is obviously wrong just by inspection, and we should fix it. The question is, do we assert in the SVG code, or return something non-null? 626621 1 zimmermann 2012-05-17 09:01:46 -0700 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. 627825 2 142740 schenney 2012-05-18 11:10:36 -0700 Created attachment 142740 Patch 627836 3 142740 gustavo 2012-05-18 11:17:27 -0700 Comment on attachment 142740 Patch Attachment 142740 did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12721868 627890 4 142740 schenney 2012-05-18 12:00:24 -0700 Comment on attachment 142740 Patch gtk failure doesn't relate to anything in this patch. No idea what's wrong there. 628367 5 142740 zimmermann 2012-05-18 23:15:23 -0700 Comment on attachment 142740 Patch Good catch. I don't see a link between the Gtk build problems and your patch, so r=me. 628368 6 zimmermann 2012-05-18 23:16:48 -0700 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 :-) 628587 7 schenney 2012-05-20 10:18:54 -0700 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. 629131 8 schenney 2012-05-21 08:47:17 -0700 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. 629153 9 schenney 2012-05-21 09:06:03 -0700 Committed r117788: <http://trac.webkit.org/changeset/117788> 142740 2012-05-18 11:10:36 -0700 2012-05-18 23:15:23 -0700 Patch bug-86738-20120518141035.patch text/plain 5193 schenney SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExNzYwNSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI1IEBACisyMDEyLTA1LTE4ICBTdGVwaGVu IENoZW5uZXkgIDxzY2hlbm5leUBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgU1ZHVGV4dFJ1blJl bmRlcmluZ0NvbnRleHQgY2FuIHJldHVybiBudWxsIGZvbnQsIGNhbGxpbmcgY29kZSBhc3NlcnRz IG5vdCBudWxsCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD04NjczOAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IFNWR1RleHRSdW5SZW5kZXJpbmdDb250ZXh0OjpnbHlwaERhdGFGb3JDaGFyYWN0ZXIgd2FzIHJl dHVybmluZyBhIGdseXBoIHdpdGgKKyAgICAgICAgbnVsbCBmb250IGRhdGEgZm9yIG51bWVyb3Vz IGNvZGUgcGF0aHMuIEl0IHNlZW1zIHRoYXQgaXQgd2FzIGRvaW5nIHNvCisgICAgICAgIHdoZW5l dmVyIGl0IGRldGVjdGVkIG51bGwgZm9udERhdGEsIHJhdGhlciB0aGFuIHRyeSB0byBjb250aW51 ZS4KKyAgICAgICAgQ2FsbGluZyBjb2RlIHdvdWxkIHRoZW4gaW1tZWRpYXRlbHkgYXNzZXJ0IG9u IHRoaXMgbnVsbCBmb250RGF0YS4KKworICAgICAgICBUaGlzIHBhdGNoIHJlZmFjdG9ycyBTVkdU ZXh0UnVuUmVuZGVyaW5nQ29udGV4dDo6Z2x5cGhEYXRhRm9yQ2hhcmFjdGVyCisgICAgICAgIHNv IHRoYXQgaXQgbmV2ZXIgcmV0dXJucyBudWxsIGZvbnQgZGF0YSwgYWRkaW5nIGFuIGFzc2VydGlv biB0byB0aGF0CisgICAgICAgIGVmZmVjdC4gSW4gcGFydGljdWxhciwgd2hlbiB0aGUgZm9udCBk YXRhIGlzIG51bGwgdGhlIGNvZGUgd2lsbCByZWFjaAorICAgICAgICB0aGUgZmFsbGJhY2sgZ2x5 cGggY2FsY3VsYXRpb25zLgorCisgICAgICAgIFJlZmFjdG9yaW5nIGNvdmVyZWQgYnkgZXhpc3Rp bmcgdGVzdHMuCisKKyAgICAgICAgKiByZW5kZXJpbmcvc3ZnL1NWR1RleHRSdW5SZW5kZXJpbmdD b250ZXh0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNWR1RleHRSdW5SZW5kZXJpbmdDb250ZXh0 OjpnbHlwaERhdGFGb3JDaGFyYWN0ZXIpOgorCiAyMDEyLTA1LTE4ICBNYXJ0aW4gUm9iaW5zb24g IDxtcm9iaW5zb25AaWdhbGlhLmNvbT4KIAogICAgICAgICBHZXQgSW5kZXhlZERCIGNsb3NlciB0 byBidWlsZGluZyBmb3IgR1RLKy4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9zdmcv U1ZHVGV4dFJ1blJlbmRlcmluZ0NvbnRleHQuY3BwCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJD b3JlL3JlbmRlcmluZy9zdmcvU1ZHVGV4dFJ1blJlbmRlcmluZ0NvbnRleHQuY3BwCShyZXZpc2lv biAxMTc1OTMpCisrKyBTb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc3ZnL1NWR1RleHRSdW5SZW5k ZXJpbmdDb250ZXh0LmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTc5LDE1ICsxNzksMTYgQEAgR2x5 cGhEYXRhIFNWR1RleHRSdW5SZW5kZXJpbmdDb250ZXh0OjpnbAogCiAgICAgcGFpcjxHbHlwaERh dGEsIEdseXBoUGFnZSo+IHBhaXIgPSBmb250LmdseXBoRGF0YUFuZFBhZ2VGb3JDaGFyYWN0ZXIo Y2hhcmFjdGVyLCBtaXJyb3IpOwogICAgIEdseXBoRGF0YSBnbHlwaERhdGEgPSBwYWlyLmZpcnN0 OwotICAgIGlmICghZ2x5cGhEYXRhLmZvbnREYXRhKQotICAgICAgICByZXR1cm4gZ2x5cGhEYXRh OwogCisgICAgLy8gQ2hlY2sgaWYgd2UgaGF2ZSB0aGUgbWlzc2luZyBnbHlwaCBkYXRhLCBpbiB3 aGljaCBjYXNlIHdlIGNhbiBqdXN0IHJldHVybi4KICAgICBHbHlwaERhdGEgbWlzc2luZ0dseXBo RGF0YSA9IHByaW1hcnlGb250LT5taXNzaW5nR2x5cGhEYXRhKCk7Ci0gICAgaWYgKGdseXBoRGF0 YS5nbHlwaCA9PSBtaXNzaW5nR2x5cGhEYXRhLmdseXBoICYmIGdseXBoRGF0YS5mb250RGF0YSA9 PSBtaXNzaW5nR2x5cGhEYXRhLmZvbnREYXRhKQorICAgIGlmIChnbHlwaERhdGEuZ2x5cGggPT0g bWlzc2luZ0dseXBoRGF0YS5nbHlwaCAmJiBnbHlwaERhdGEuZm9udERhdGEgPT0gbWlzc2luZ0ds eXBoRGF0YS5mb250RGF0YSkgeworICAgICAgICBBU1NFUlQoZ2x5cGhEYXRhLmZvbnREYXRhKTsK ICAgICAgICAgcmV0dXJuIGdseXBoRGF0YTsKKyAgICB9CiAKICAgICAvLyBDaGFyYWN0ZXJzIGVu Y2xvc2VkIGJ5IGFuIDxhbHRHbHlwaD4gZWxlbWVudCwgbWF5IG5vdCBiZSByZWdpc3RlcmVkIGlu IHRoZSBHbHlwaFBhZ2UuCi0gICAgaWYgKCFnbHlwaERhdGEuZm9udERhdGEtPmlzU1ZHRm9udCgp KSB7CisgICAgaWYgKGdseXBoRGF0YS5mb250RGF0YSAmJiAhZ2x5cGhEYXRhLmZvbnREYXRhLT5p c1NWR0ZvbnQoKSkgewogICAgICAgICBpZiAoVGV4dFJ1bjo6UmVuZGVyaW5nQ29udGV4dCogcmVu ZGVyaW5nQ29udGV4dCA9IHJ1bi5yZW5kZXJpbmdDb250ZXh0KCkpIHsKICAgICAgICAgICAgIFJl bmRlck9iamVjdCogcmVuZGVyT2JqZWN0ID0gc3RhdGljX2Nhc3Q8U1ZHVGV4dFJ1blJlbmRlcmlu Z0NvbnRleHQqPihyZW5kZXJpbmdDb250ZXh0KS0+cmVuZGVyZXIoKTsKICAgICAgICAgICAgIFJl bmRlck9iamVjdCogcGFyZW50UmVuZGVyT2JqZWN0ID0gcmVuZGVyT2JqZWN0LT5pc1RleHQoKSA/ IHJlbmRlck9iamVjdC0+cGFyZW50KCkgOiByZW5kZXJPYmplY3Q7CkBAIC0xOTksMjQgKzIwMCwy NSBAQCBHbHlwaERhdGEgU1ZHVGV4dFJ1blJlbmRlcmluZ0NvbnRleHQ6OmdsCiAgICAgICAgIH0K ICAgICB9CiAKLSAgICBpZiAoIWdseXBoRGF0YS5mb250RGF0YSB8fCAhZ2x5cGhEYXRhLmZvbnRE YXRhLT5pc1NWR0ZvbnQoKSkKLSAgICAgICAgcmV0dXJuIGdseXBoRGF0YTsKLQogICAgIGNvbnN0 IFNpbXBsZUZvbnREYXRhKiBmb250RGF0YSA9IGdseXBoRGF0YS5mb250RGF0YTsKLQotICAgIFNW R0ZvbnRFbGVtZW50KiBmb250RWxlbWVudCA9IDA7Ci0gICAgU1ZHRm9udEZhY2VFbGVtZW50KiBm b250RmFjZUVsZW1lbnQgPSAwOwotCi0gICAgY29uc3QgU1ZHRm9udERhdGEqIHN2Z0ZvbnREYXRh ID0gc3ZnRm9udEFuZEZvbnRGYWNlRWxlbWVudEZvckZvbnREYXRhKGZvbnREYXRhLCBmb250RmFj ZUVsZW1lbnQsIGZvbnRFbGVtZW50KTsKLSAgICBpZiAoIWZvbnRFbGVtZW50IHx8ICFmb250RmFj ZUVsZW1lbnQpCi0gICAgICAgIHJldHVybiBnbHlwaERhdGE7Ci0KLSAgICAvLyBJZiB3ZSBnb3Qg aGVyZSwgd2UncmUgZGVhbGluZyB3aXRoIGEgZ2x5cGggZGVmaW5lZCBpbiBhIFNWRyBGb250Lgot ICAgIC8vIFRoZSByZXR1cm5lZCBnbHlwaCBieSBnbHlwaERhdGFBbmRQYWdlRm9yQ2hhcmFjdGVy KCkgaXMgYSBnbHlwaCBzdG9yZWQgaW4gdGhlIFNWRyBGb250IGdseXBoIHRhYmxlLgotICAgIC8v IFRoaXMgZG9lc24ndCBuZWNlc3NhcmlseSBtZWFuIHRoZSBnbHlwaCBpcyBzdWl0YWJsZSBmb3Ig cmVuZGVyaW5nL21lYXN1cmluZyBpbiB0aGlzIGNvbnRleHQsIGl0cwotICAgIC8vIGFyYWJpYy1m b3JtL29yaWVudGF0aW9uLy4uLiBtYXkgbm90IG1hdGNoLCB3ZSBoYXZlIHRvIGFwcGx5IFNWRyBH bHlwaCBzZWxlY3Rpb24gdG8gZGlzY292ZXIgdGhhdC4KLSAgICBpZiAoc3ZnRm9udERhdGEtPmFw cGx5U1ZHR2x5cGhTZWxlY3Rpb24oaXRlcmF0b3IsIGdseXBoRGF0YSwgbWlycm9yLCBjdXJyZW50 Q2hhcmFjdGVyLCBhZHZhbmNlTGVuZ3RoKSkKLSAgICAgICAgcmV0dXJuIGdseXBoRGF0YTsKKyAg ICBpZiAoZm9udERhdGEpIHsKKyAgICAgICAgaWYgKCFmb250RGF0YS0+aXNTVkdGb250KCkpCisg ICAgICAgICAgICByZXR1cm4gZ2x5cGhEYXRhOworCisgICAgICAgIFNWR0ZvbnRFbGVtZW50KiBm b250RWxlbWVudCA9IDA7CisgICAgICAgIFNWR0ZvbnRGYWNlRWxlbWVudCogZm9udEZhY2VFbGVt ZW50ID0gMDsKKworICAgICAgICBjb25zdCBTVkdGb250RGF0YSogc3ZnRm9udERhdGEgPSBzdmdG b250QW5kRm9udEZhY2VFbGVtZW50Rm9yRm9udERhdGEoZm9udERhdGEsIGZvbnRGYWNlRWxlbWVu dCwgZm9udEVsZW1lbnQpOworICAgICAgICBpZiAoIWZvbnRFbGVtZW50IHx8ICFmb250RmFjZUVs ZW1lbnQpCisgICAgICAgICAgICByZXR1cm4gZ2x5cGhEYXRhOworCisgICAgICAgIC8vIElmIHdl IGdvdCBoZXJlLCB3ZSdyZSBkZWFsaW5nIHdpdGggYSBnbHlwaCBkZWZpbmVkIGluIGEgU1ZHIEZv bnQuCisgICAgICAgIC8vIFRoZSByZXR1cm5lZCBnbHlwaCBieSBnbHlwaERhdGFBbmRQYWdlRm9y Q2hhcmFjdGVyKCkgaXMgYSBnbHlwaCBzdG9yZWQgaW4gdGhlIFNWRyBGb250IGdseXBoIHRhYmxl LgorICAgICAgICAvLyBUaGlzIGRvZXNuJ3QgbmVjZXNzYXJpbHkgbWVhbiB0aGUgZ2x5cGggaXMg c3VpdGFibGUgZm9yIHJlbmRlcmluZy9tZWFzdXJpbmcgaW4gdGhpcyBjb250ZXh0LCBpdHMKKyAg ICAgICAgLy8gYXJhYmljLWZvcm0vb3JpZW50YXRpb24vLi4uIG1heSBub3QgbWF0Y2gsIHdlIGhh dmUgdG8gYXBwbHkgU1ZHIEdseXBoIHNlbGVjdGlvbiB0byBkaXNjb3ZlciB0aGF0LgorICAgICAg ICBpZiAoc3ZnRm9udERhdGEtPmFwcGx5U1ZHR2x5cGhTZWxlY3Rpb24oaXRlcmF0b3IsIGdseXBo RGF0YSwgbWlycm9yLCBjdXJyZW50Q2hhcmFjdGVyLCBhZHZhbmNlTGVuZ3RoKSkKKyAgICAgICAg ICAgIHJldHVybiBnbHlwaERhdGE7CisgICAgfQogCiAgICAgR2x5cGhQYWdlKiBwYWdlID0gcGFp ci5zZWNvbmQ7CiAgICAgQVNTRVJUKHBhZ2UpOwpAQCAtMjQwLDYgKzI0Miw3IEBAIEdseXBoRGF0 YSBTVkdUZXh0UnVuUmVuZGVyaW5nQ29udGV4dDo6Z2wKICAgICBwYWdlLT5zZXRHbHlwaERhdGFG b3JDaGFyYWN0ZXIoY2hhcmFjdGVyLCBnbHlwaERhdGEuZ2x5cGgsIGZvbnREYXRhKTsKICAgICBm b250TGlzdC0+c2V0R2x5cGhQYWdlWmVybyhvcmlnaW5hbEdseXBoUGFnZVplcm8pOwogICAgIGZv bnRMaXN0LT5zZXRHbHlwaFBhZ2VzKG9yaWdpbmFsR2x5cGhQYWdlcyk7CisgICAgQVNTRVJUKGZh bGxiYWNrR2x5cGhEYXRhLmZvbnREYXRhKTsKICAgICByZXR1cm4gZmFsbGJhY2tHbHlwaERhdGE7 CiB9CiAK