Bug 40966

Summary:

[chromium] overlapping characters in complex text

Product:

WebKit

Reporter:

Evan Martin <evan>

Component:

New Bugs

Assignee:

Evan Martin <evan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

agl

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	abarth: review+
a simple test	none

Evan Martin

Reported 2010-06-21 20:18:40 PDT

[chromium] overlapping characters in complex text

Attachments
Patch (40.84 KB, patch) 2010-06-21 20:19 PDT, Evan Martin	no flags	Details Formatted Diff Diff
Patch (41.15 KB, patch) 2010-06-21 20:21 PDT, Evan Martin	no flags	Details Formatted Diff Diff
Patch (117.91 KB, patch) 2010-06-22 13:30 PDT, Evan Martin	abarth: review+	Details Formatted Diff Diff
a simple test (611 bytes, text/html) 2010-06-23 15:25 PDT, Jungshik Shin	no flags	Details
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Evan Martin

Comment 1 2010-06-21 20:19:29 PDT

Created attachment 59328 [details] Patch

Evan Martin

Comment 2 2010-06-21 20:21:29 PDT

Created attachment 59329 [details] Patch

Adam Langley

Comment 3 2010-06-22 09:29:37 PDT

Code cleanup is good, but I'm not clear where the actual fix is. Is it the clearing of the arrays? If so, isn't that a Harfbuzz bug? Maybe add a comment when clearing them so that someone doesn't optimise it away in the future.

Evan Martin

Comment 4 2010-06-22 13:25:53 PDT

I spent a long time reverting and reapplying bits of this patch and I am actually kind of confused why this refactoring is making the test fail or pass. However, the new code is clearly more correct than the prior code (instead of a looping reallocation we now just allocate the proper memory in the first go) I think the patch is ok anyway. I imagine it's probably something using uninitialized memory that's causing the problem.

Evan Martin

Comment 5 2010-06-22 13:30:28 PDT

Created attachment 59412 [details] Patch

Jungshik Shin

Comment 6 2010-06-23 15:25:22 PDT

Created attachment 59567 [details] a simple test I confirmed that the patch here fixes one of two issues reported against Chromium ( http://crbug.com/44646.html http://crbug.com/43951.html ). I'm attaching a test html file illustrating the issue fixed by this patch.

Adam Barth

Comment 7 2010-06-24 15:55:03 PDT

Comment on attachment 59412 [details] Patch Not my strong suit, but looks reasonable. WebCore/platform/graphics/chromium/FontLinux.cpp:421 + m_glyphs16 = new uint16_t[size]; This doesn't need to be zeroed?

Evan Martin

Comment 8 2010-06-24 15:57:49 PDT

That's an output, so it seems ok.

Evan Martin

Comment 9 2010-06-24 16:12:33 PDT

Committed r61795: <http://trac.webkit.org/changeset/61795>

Note You need to log in before you can comment on or make changes to this bug.