Bug 68287 – Decomposed text is displayed incorrectly when Verdana is specified

• Component: Text
• Status: RESOLVED FIXED

<rdar://problem/7860281>

Accents are displayed displaced for decomposed text on the attached test case.

Created attachment 107733 [details]
Test case

Created attachment 107741 [details]
Allow Core Text to choose the fallback font for rendering a combining character sequence

[WebKit Review Bot]

Attachment 107741 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:53:  _font is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:54:  _character is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:55:  _count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:66:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:80:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 6 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 107741 [details]
Allow Core Text to choose the fallback font for rendering a combining character sequence

Attachment 107741 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9725215

(In reply to comment #4)
> (From update of attachment 107741 [details])
> Attachment 107741 [details] did not pass cr-mac-ews (chromium):
> Output: http://queues.webkit.org/results/9725215

This doesn’t seem to include the actual compile errors.

[WebKit Review Bot]

Comment on attachment 107741 [details]
Allow Core Text to choose the fallback font for rendering a combining character sequence

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

New failing tests:
fast/text/combining-character-sequence-fallback.html

[WebKit Review Bot]

Comment on attachment 107741 [details]
Allow Core Text to choose the fallback font for rendering a combining character sequence

Attachment 107741 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9735296

[Alexey Proskuryakov]

Comment on attachment 107741 [details]
Allow Core Text to choose the fallback font for rendering a combining character sequence

It's not clear what pass/fail criteria for the test are. The expected result in this patch looks like a failure due to poor rendering of enclosed a, unless you know how bad it was before.

[Dave Hyatt]

Comment on attachment 107741 [details]
Allow Core Text to choose the fallback font for rendering a combining character sequence

r=me

I think you need fontDataForCombiningCharacterSequence to have #if PLATFORM(MAC) in Font.h.

Fix the style errors also.

[Dave Hyatt]

I know you don't use the method on other platforms, but I think it's cleaner to have #if PLATFORM(MAC) in the header as a clear indicator that it's a Mac-specific method.

(In reply to comment #8)
> (From update of attachment 107741 [details])
> It's not clear what pass/fail criteria for the test are. The expected result in this patch looks like a failure due to poor rendering of enclosed a, unless you know how bad it was before.

I have added some explanatory text to the test and, where possible, reference renderings.

(In reply to comment #9)
> (From update of attachment 107741 [details])
> r=me
> 
> I think you need fontDataForCombiningCharacterSequence to have #if PLATFORM(MAC) in Font.h.

Not strictly needed, but done.

> 
> Fix the style errors also.

I think the style bot was confused by Objective-C++ in a .cpp file.

Fixed in <http://trac.webkit.org/r95391>.

(In reply to comment #4)
> (From update of attachment 107741 [details])
> Attachment 107741 [details] did not pass cr-mac-ews (chromium):
> Output: http://queues.webkit.org/results/9725215

I am hopeful that build.webkit.org will let me see the full build transcript now and I will be able to fix it ASAP.

Leopard build fixed in <http://trac.webkit.org/r95392>.

Snow Leopard build fix in <http://trac.webkit.org/r95393>.

Attempted Chromium Mac build fix in <http://trac.webkit.org/r95394>.

Additional Chromium Mac build fixes in <http://trac.webkit.org/r95395> and <http://trac.webkit.org/r95396>.

[Ryosuke Niwa]

It appears that a test added by this patch is failing on Snow Leopard:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r95747%20(33298)/platform/mac/fast/text/combining-character-sequence-fallback-pretty-diff.html

Do we just need a rebaseline?

[James Robinson]

Hullo,

We're getting some crash reports from users with stacks like this:

Thread 0 *CRASHED* ( EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE @ 0x00000000 )

0x013415ed	 [Google Chrome Framework	 - ComplexTextControllerCoreText.mm:93]	-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]
0x958b1191	 [CoreFoundation	 + 0x00012191]	CFArrayGetValueAtIndex
0x98a452a4	 [CoreText	 + 0x000432a4]	TFontCascade::CreateCascadeFallback(long)
0x98a4571f	 [CoreText	 + 0x0004371f]	TFontCascade::CreateFallback(__CTFont const*, __CFString const*, CFRange) const
0x98a32513	 [CoreText	 + 0x00030513]	TGlyphEncoder::AppendUnmappedCharRun(CTRun*, CFRange, TGlyphList<TDeletedGlyphIndex>&, TGlyphList<TDeletedGlyphIndex>&, TFontCascade const&)
0x98a323cb	 [CoreText	 + 0x000303cb]	TGlyphEncoder::RunUnicodeEncoderRecursively(CTRun*, adopted_t const&, CFRange, TGlyphList<TDeletedGlyphIndex>&, TGlyphList<TDeletedGlyphIndex>&, TFontCascade const&)
0x98a32677	 [CoreText	 + 0x00030677]	TGlyphEncoder::AppendUnmappedCharRun(CTRun*, CFRange, TGlyphList<TDeletedGlyphIndex>&, TGlyphList<TDeletedGlyphIndex>&, TFontCascade const&)
0x98a323cb	 [CoreText	 + 0x000303cb]	TGlyphEncoder::RunUnicodeEncoderRecursively(CTRun*, adopted_t const&, CFRange, TGlyphList<TDeletedGlyphIndex>&, TGlyphList<TDeletedGlyphIndex>&, TFontCascade const&)
0x98a1da97	 [CoreText	 + 0x0001ba97]	TGlyphEncoder::RunUnicodeEncoder(CTRun*, adopted_t const&, CFRange, TGlyphList<TDeletedGlyphIndex>&, TFontCascade const&)
0x98a096e4	 [CoreText	 + 0x000076e4]	TGlyphEncoder::EncodeChars(CFRange, TAttributes const&, TGlyphList<TDeletedGlyphIndex>&)
0x98a3e577	 [CoreText	 + 0x0003c577]	TTypesetterUniChar::Initialize()
0x98a33c0e	 [CoreText	 + 0x00031c0e]	CTLineCreateWithUniCharProvider
0x013420b0	 [Google Chrome Framework	 - ComplexTextControllerCoreText.mm:232]	WebCore::ComplexTextController::collectComplexTextRunsForCharactersCoreText
0x0133ec12	 [Google Chrome Framework	 - ComplexTextController.cpp:345]	WebCore::ComplexTextController::collectComplexTextRunsForCharacters
0x0133d67e	 [Google Chrome Framework	 - ComplexTextController.cpp:291]	WebCore::ComplexTextController::collectComplexTextRuns
0x0133d41a	 [Google Chrome Framework	 - ComplexTextController.cpp:97]	WebCore::ComplexTextController::ComplexTextController
0x0133d209	 [Google Chrome Framework	 - ComplexTextController.cpp:82]	WebCore::ComplexTextController::ComplexTextController
0x01344934	 [Google Chrome Framework	 - FontComplexTextMac.cpp:112]	WebCore::Font::floatWidthForComplexText
0x016e857d	 [Google Chrome Framework	 - RenderBlockLineLayout.cpp:1743]	WebCore::textWidth
0x016e58b3	 [Google Chrome Framework	 - RenderBlockLineLayout.cpp:2305]	WebCore::RenderBlock::LineBreaker::nextLineBreak
0x016e2945	 [Google Chrome Framework	 - RenderBlockLineLayout.cpp:1079]	WebCore::RenderBlock::layoutRunsAndFloatsInRange
0x016e1001	 [Google Chrome Framework	 - RenderBlockLineLayout.cpp:1048]	WebCore::RenderBlock::layoutRunsAndFloats
0x016e6d72	 [Google Chrome Framework	 - RenderBlockLineLayout.cpp:1334]	WebCore::RenderBlock::layoutInlineChildren
0x016c9344	 [Google Chrome Framework	 - RenderBlock.cpp:1265]	WebCore::RenderBlock::layoutBlock
0x016c8cf7	 [Google Chrome Framework	 - RenderBlock.cpp:1154]	WebCore::RenderBlock::layout
0x016cf0f6	 [Google Chrome Framework	 - RenderBlock.cpp:2020]	WebCore::RenderBlock::layoutBlockChild
0x016cacfb	 [Google Chrome Framework	 - RenderBlock.cpp:1958]	WebCore::RenderBlock::layoutBlockChildren
0x016c9361	 [Google Chrome Framework	 - RenderBlock.cpp:1267]	WebCore::RenderBlock::layoutBlock
0x016c8cf7	 [Google Chrome Framework	 - RenderBlock.cpp:1154]	WebCore::RenderBlock::layout
0x016cf0f6	 [Google Chrome Framework	 - RenderBlock.cpp:2020]	WebCore::RenderBlock::layoutBlockChild
...... (68 stack frames dropped.)


This showed up quite recently and seems related to this patch Are you seeing anything similar?  Do you think this is related?  I do not have repro steps yet.

[James Robinson]

We're seeing this on 10.7 and 10.6 on a variety of URLs (no particular pattern to them).

(In reply to comment #20)
> Hullo,
> 
> We're getting some crash reports from users with stacks like this:
> 
> Thread 0 *CRASHED* ( EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE @ 0x00000000 )
> 
> 0x013415ed     [Google Chrome Framework     - ComplexTextControllerCoreText.mm:93]    -[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]

Can you map that to an instruction and infer what is pointing to 0?

> This showed up quite recently and seems related to this patch Are you seeing anything similar?

No

> Do you think this is related?

A crash in code added in this patch is obviously related to the patch. Without knowing the conditions that trigger the crash it’s hard to tell whether they would have led to a different crash prior to this patch.

[James Robinson]

(In reply to comment #22)
> (In reply to comment #20)
> > Hullo,
> > 
> > We're getting some crash reports from users with stacks like this:
> > 
> > Thread 0 *CRASHED* ( EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE @ 0x00000000 )
> > 
> > 0x013415ed     [Google Chrome Framework     - ComplexTextControllerCoreText.mm:93]    -[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]
> 
> Can you map that to an instruction and infer what is pointing to 0?

Disassembly around the crashing instruction:

0x3e2e25c2 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+130>:	je     0x3e2e25ca <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+138>
0x3e2e25c4 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+132>:	movl   $0x0,(%eax)
0x3e2e25ca <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+138>:	add    $0x4,%eax
0x3e2e25cd <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+141>:	add    $0xfffffffc,%ecx
0x3e2e25d0 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+144>:	jne    0x3e2e25c0 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+128>
0x3e2e25d2 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+146>:	mov    %edi,(%esi)
0x3e2e25d4 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+148>:	mov    0x8(%ebp),%edi
0x3e2e25d7 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+151>:	mov    0x4(%edi),%ecx
0x3e2e25da <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+154>:	mov    0x28(%ecx),%eax
0x3e2e25dd <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+157>:	mov    %ebx,0x8(%esp)
0x3e2e25e1 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+161>:	mov    %ecx,0x4(%esp)
0x3e2e25e5 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+165>:	mov    %eax,(%esp)
0x3e2e25e8 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+168>:	call   0x3e2879c0 <_ZNK7WebCore16FontFallbackList10fontDataAtEPKNS_4FontEj>
***** CRASH HERE ***** 0x3e2e25ed <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+173>:	mov    (%eax),%ecx
0x3e2e25ef <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+175>:	mov    0x8(%edi),%edx
0x3e2e25f2 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+178>:	mov    %edx,0x4(%esp)
0x3e2e25f6 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+182>:	mov    %eax,(%esp)
0x3e2e25f9 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+185>:	call   *0x8(%ecx)
0x3e2e25fc <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+188>:	add    $0x28,%eax
0x3e2e25ff <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+191>:	mov    %eax,(%esp)
0x3e2e2602 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+194>:	call   0x3e2d1820 <_ZNK7WebCore16FontPlatformData6ctFontEv>
0x3e2e2607 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+199>:	mov    %eax,(%esp)
0x3e2e260a <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+202>:	call   0x3fd149e2 <dyld_stub_CTFontCopyFontDescriptor>
0x3e2e260f <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+207>:	mov    %eax,%esi
0x3e2e2611 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+209>:	mov    0x14(%edi),%edi
0x3e2e2614 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+212>:	test   %esi,%esi
0x3e2e2616 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+214>:	je     0x3e2e2620 <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+224>

info registers
eax            0x0	0
ecx            0x43d6f0	4445936
edx            0x8	8
ebx            0x2	2
esp            0xbfff8160	0xbfff8160
ebp            0xbfff8178	0xbfff8178
esi            0x182a40	1583680
edi            0x182a30	1583664
eip            0x3e2e25ed	0x3e2e25ed <-[ChromiumWebCoreObjCWebCascadeList objectAtIndex:]+173>

It looks like _font is not NULL but the return value of _font->fontDataAt(index) is NULL

> > Do you think this is related?
> 
> A crash in code added in this patch is obviously related to the patch. Without knowing the conditions that trigger the crash it’s hard to tell whether they would have led to a different crash prior to this patch.

Well I guess that was a silly question :). Here's a repro URL: http://www.pouria-alami.com/2011/08/blog-post_24.html.  This crashes on ToT but not on an older build, so I think this is a regression.

(In reply to comment #23)
> Here's a repro URL: http://www.pouria-alami.com/2011/08/blog-post_24.html.  This crashes on ToT but not on an older build, so I think this is a regression.

Thank you. I can now see what the problem is.

Filed bug 68737.

(In reply to comment #19)
> It appears that a test added by this patch is failing on Snow Leopard:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r95747%20(33298)/platform/mac/fast/text/combining-character-sequence-fallback-pretty-diff.html
> 
> Do we just need a rebaseline?

Added Snow Leopard-specific expected results in <http://trac.webkit.org/r95909>.