Bug 138683 – platform/mac/editing/input/devanagari-ligature.html is flaky on Yosemite, ligature fails to form

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Myles C. Maxfield]

Can reproduce on my cheese grater (with --child-processes=1)

[Myles C. Maxfield]

SimpleFontData::canRenderCombiningCharacterSequence() is returning different answers

[Myles C. Maxfield]

The font in question is "ITFDevanagari-Book" and the code point sequence in question is U+0915 U+094D

[Myles C. Maxfield]

Disabling the cache in SimpleFontData::canRenderCombiningCharacterSequence() seems to fix this

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 241600 [details]
Patch

[Alexey Proskuryakov]

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.

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Created attachment 241607 [details]
Patch

[Myles C. Maxfield]

(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

[Myles C. Maxfield]

Created attachment 241613 [details]
Patch

[Alexey Proskuryakov]

FWIW, I cannot reproduce on OS X Mavericks with the same list of 33 tests.

[Myles C. Maxfield]

Please try the longer list. My laptop can only reproduce on the longer list, not the shorter one.

[Alexey Proskuryakov]

Cannot reproduce on Mavericks with the long list either.

[Radar WebKit Bug Importer]

<rdar://problem/19020875>

[Myles C. Maxfield]

Created attachment 241835 [details]
Patch

[Alexey Proskuryakov]

Ping?

[Myles C. Maxfield]

The earliest I'll be able to investigate this is Monday.

[Myles C. Maxfield]

This is ultimately caused by <rdar://problem/19316856>

[Myles C. Maxfield]

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?

[Myles C. Maxfield]

Created attachment 243642 [details]
Patch

[Darin Adler]

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?

[Myles C. Maxfield]

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.

[Myles C. Maxfield]

Committed r177689: <http://trac.webkit.org/changeset/177689>

[Alexey Proskuryakov]

*** 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.