Created attachment 241465 [details] tests.txt platform/mac/editing/input/devanagari-ligature.html is flaky on bots, and I can reproduce 100% reliably locally. The failure shows correct Unicode content, but with a wrong width, meaning that a ligature failed to form at text drawing time I think. Steps to reproduce with attached tests.txt: run-webkit-tests --test-list=tests.txt --child-processes=1 --no-retry - RenderText {#text} at (0,4) size 12x18 - text run at (0,4) width 12: "\x{915}\x{94D}\x{937}" + RenderText {#text} at (0,4) size 21x18 + text run at (0,4) width 21: "\x{915}\x{94D}\x{937}" It runs ~1500 tests before this one, and I cannot reduce the list much further. So, it's not just that some previous test breaks this one, it looks more like some cache or table overflows and text machinery starts to misbehave.
Created attachment 241475 [details] tests.txt OK, reduced that to 33 tests. Probably not much help for analysis, but at least it can now be reproduced quickly.
Can reproduce on my cheese grater (with --child-processes=1)
SimpleFontData::canRenderCombiningCharacterSequence() is returning different answers
The font in question is "ITFDevanagari-Book" and the code point sequence in question is U+0915 U+094D
Disabling the cache in SimpleFontData::canRenderCombiningCharacterSequence() seems to fix this
This is because CFEqual() on CGFontRefs is a raw pointer compare instead of deep introspection. CTFontCopyGraphicsFont() uses an internal cache, so the pointer compare will fail or succeed based on whether or not the font happens to already be in the internal cache
Created attachment 241600 [details] Patch
Comment on attachment 241600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241600&action=review Nicely done! Leaving this for someone who knows about fonts to review though. > Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:509 > + RetainPtr<CFURLRef> runUrl = adoptCF(static_cast<CFURLRef>(CTFontDescriptorCopyAttribute(runFontDescriptor.get(), kCTFontReferenceURLAttribute))); WebKit style for this name is "runURL". I wonder if this attribute is always guaranteed to be non-null and unique.
Comment on attachment 241600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241600&action=review >> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:509 >> + RetainPtr<CFURLRef> runUrl = adoptCF(static_cast<CFURLRef>(CTFontDescriptorCopyAttribute(runFontDescriptor.get(), kCTFontReferenceURLAttribute))); > > WebKit style for this name is "runURL". > > I wonder if this attribute is always guaranteed to be non-null and unique. It is. WebFonts will look like "file://iNmEmOrYcGfOnT_0x7f8220cef460#postscript-name=DroidSansFallback" However, note the pointer inside the name. This means that the original bug will still exist for these fonts. I'll put a FIXME in the code.
Created attachment 241607 [details] Patch
(In reply to comment #9) > Comment on attachment 241600 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=241600&action=review > > >> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:509 > >> + RetainPtr<CFURLRef> runUrl = adoptCF(static_cast<CFURLRef>(CTFontDescriptorCopyAttribute(runFontDescriptor.get(), kCTFontReferenceURLAttribute))); > > > > WebKit style for this name is "runURL". > > > > I wonder if this attribute is always guaranteed to be non-null and unique. > > It is. WebFonts will look like > "file://iNmEmOrYcGfOnT_0x7f8220cef460#postscript-name=DroidSansFallback" > However, note the pointer inside the name. This means that the original bug > will still exist for these fonts. I'll put a FIXME in the code. Ultimately, there's no way to do a reliable deep compare for all CTFontRefs, and <rdar://problem/18985642> is about this
Created attachment 241613 [details] Patch
FWIW, I cannot reproduce on OS X Mavericks with the same list of 33 tests.
Please try the longer list. My laptop can only reproduce on the longer list, not the shorter one.
Cannot reproduce on Mavericks with the long list either.
<rdar://problem/19020875>
Created attachment 241835 [details] Patch
Ping?
The earliest I'll be able to investigate this is Monday.
This is ultimately caused by <rdar://problem/19316856>
We should probably only do this on 10.10 and below.
Comment on attachment 241835 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241835&action=review The pattern of using a CTFont’s underlying CGFont for comparison occurs in ComplexTextController::collectComplexTextRunsForCharacters too, so I think it should be fixed there as well. Additionally, if the problematic CTFontCopyGraphicsFont behavior is specific to some OS versions, then the workaround should only be used when building for those versions.. > Source/WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:129 > + // FIXME: https://bugs.webkit.org/show_bug.cgi?id=138683 This is a shallow pointer compare for web fonts > + // which means we might erroneously return false here What does this mean? If the objects are CFURLs as you assert above, how can CFEqual be comparing them differently than it compares other URLs?
Created attachment 243642 [details] Patch
Comment on attachment 243642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243642&action=review > Source/WebCore/platform/graphics/FontPlatformData.h:124 > + static RetainPtr<CFTypeRef> objectForEqualityCheck(CTFontRef); > + RetainPtr<CFTypeRef> objectForEqualityCheck() const; This seems a bit clumsy, but I guess it’s OK. > Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:296 > + RetainPtr<CTFontDescriptorRef> fontDescriptor = adoptCF(CTFontCopyFontDescriptor(ctFont)); Consider auto for the type here? > Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:309 > + return FontPlatformData::objectForEqualityCheck(ctFont()); No need for the FontPlatformData:: prefix here. > Source/WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:124 > + RetainPtr<CFTypeRef> runFontEqualityObject = FontPlatformData::objectForEqualityCheck(runFont); Do we need this local variable? > Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:496 > + RetainPtr<CFTypeRef> runFontEqualityObject = FontPlatformData::objectForEqualityCheck(runFont); Do we need this local variable?
Comment on attachment 243642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243642&action=review >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:296 >> + RetainPtr<CTFontDescriptorRef> fontDescriptor = adoptCF(CTFontCopyFontDescriptor(ctFont)); > > Consider auto for the type here? Done. >> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:309 >> + return FontPlatformData::objectForEqualityCheck(ctFont()); > > No need for the FontPlatformData:: prefix here. Done. >> Source/WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:124 >> + RetainPtr<CFTypeRef> runFontEqualityObject = FontPlatformData::objectForEqualityCheck(runFont); > > Do we need this local variable? Done. >> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:496 >> + RetainPtr<CFTypeRef> runFontEqualityObject = FontPlatformData::objectForEqualityCheck(runFont); > > Do we need this local variable? Done.
Committed r177689: <http://trac.webkit.org/changeset/177689>
*** Bug 138075 has been marked as a duplicate of this bug. ***
Comment on attachment 243642 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243642&action=review > Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm:259 > + if (CFEqual(runFontEqualityObject.get(), runFontEqualityObject.get())) Whoops, this is a tautology! It caused bug 141112.