RESOLVED FIXED 40966
[chromium] overlapping characters in complex text 
https://bugs.webkit.org/show_bug.cgi?id=40966
Summary [chromium] overlapping characters in complex text
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
DetailsFormatted DiffDiff
Patch (41.15 KB, patch)
2010-06-21 20:21 PDT, Evan Martin 		no flags
DetailsFormatted DiffDiff
Patch (117.91 KB, patch)
2010-06-22 13:30 PDT, Evan Martin 		abarth: review+
DetailsFormatted DiffDiff
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.