Bug 68287 - Decomposed text is displayed incorrectly when Verdana is specified
Summary: Decomposed text is displayed incorrectly when Verdana is specified
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-09-16 15:35 PDT by mitz
Modified: 2019-06-23 18:03 PDT (History)
7 users (show)

See Also:


Attachments
Test case (71 bytes, text/html)
2011-09-16 15:35 PDT, mitz
no flags Details
Allow Core Text to choose the fallback font for rendering a combining character sequence (91.30 KB, patch)
2011-09-16 16:18 PDT, mitz
hyatt: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2011-09-16 15:35:34 PDT
<rdar://problem/7860281>

Accents are displayed displaced for decomposed text on the attached test case.
Comment 1 mitz 2011-09-16 15:35:58 PDT
Created attachment 107733 [details]
Test case
Comment 2 mitz 2011-09-16 16:18:36 PDT
Created attachment 107741 [details]
Allow Core Text to choose the fallback font for rendering a combining character sequence
Comment 3 WebKit Review Bot 2011-09-16 16:21:29 PDT
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.
Comment 4 WebKit Review Bot 2011-09-16 17:14:57 PDT
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
Comment 5 mitz 2011-09-16 17:21:55 PDT
(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.
Comment 6 WebKit Review Bot 2011-09-17 03:58:44 PDT
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
Comment 7 WebKit Review Bot 2011-09-17 09:27:55 PDT
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
Comment 8 Alexey Proskuryakov 2011-09-17 12:52:12 PDT
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.
Comment 9 Dave Hyatt 2011-09-17 18:16:15 PDT
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.
Comment 10 Dave Hyatt 2011-09-17 18:17:28 PDT
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.
Comment 11 mitz 2011-09-18 09:52:38 PDT
(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.
Comment 12 mitz 2011-09-18 09:53:18 PDT
(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.
Comment 13 mitz 2011-09-18 09:53:42 PDT
Fixed in <http://trac.webkit.org/r95391>.
Comment 14 mitz 2011-09-18 09:54:35 PDT
(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.
Comment 15 mitz 2011-09-18 10:02:17 PDT
Leopard build fixed in <http://trac.webkit.org/r95392>.
Comment 16 mitz 2011-09-18 10:17:21 PDT
Snow Leopard build fix in <http://trac.webkit.org/r95393>.
Comment 17 mitz 2011-09-18 10:26:57 PDT
Attempted Chromium Mac build fix in <http://trac.webkit.org/r95394>.
Comment 18 mitz 2011-09-18 12:02:37 PDT
Additional Chromium Mac build fixes in <http://trac.webkit.org/r95395> and <http://trac.webkit.org/r95396>.
Comment 19 Ryosuke Niwa 2011-09-22 15:08:19 PDT
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?
Comment 20 James Robinson 2011-09-23 13:05:22 PDT
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.
Comment 21 James Robinson 2011-09-23 13:06:50 PDT
We're seeing this on 10.7 and 10.6 on a variety of URLs (no particular pattern to them).
Comment 22 mitz 2011-09-23 13:18:21 PDT
(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.
Comment 23 James Robinson 2011-09-23 14:48:07 PDT
(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.
Comment 24 mitz 2011-09-23 15:11:26 PDT
(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.
Comment 25 mitz 2011-09-23 15:17:30 PDT
Filed bug 68737.
Comment 26 mitz 2011-09-24 12:13:09 PDT
(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>.