Closed
Bug 375864
Opened 17 years ago
Closed 17 years ago
Crash [@ gfxTextRun::CompressedGlyph::IsClusterStart] when viewing URL as UTF-16
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha5
People
(Reporter: MatsPalmgren_bugz, Assigned: karlt)
References
()
Details
(Keywords: crash, regression)
Crash Data
Attachments
(3 files, 1 obsolete file)
10.65 KB,
text/plain
|
Details | |
63.23 KB,
text/html
|
Details | |
14.84 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
STEPS TO REPRODUCE 1. load URL 2. Choose UTF-16 from the View->Character Encoding menu Possibly related bug on Windows on the same URL: bug 375773 ACTUAL RESULT Crash, see attached stack. Assertions leading up to the crash: ###!!! ASSERTION: First character must be the start of a cluster and can't be a ligature continuation!: 'aCharIndex > 0 || (aGlyph.IsClusterStart() && !aGlyph.IsLigatureContinuation())', file ../../../dist/include/thebes/gfxFont.h, line 888 ###!!! ASSERTION: Ligature at the start of the run??: 'ligStart > 0', file gfxFont.cpp, line 605 PLATFORMS AND BUILDS TESTED Bug occurs in trunk debug build (updated today)
Reporter | ||
Comment 1•17 years ago
|
||
Comment 2•17 years ago
|
||
Worth attaching the page to the bug in case it changes...
Flags: blocking1.9?
Reporter | ||
Comment 3•17 years ago
|
||
Assignee | ||
Comment 4•17 years ago
|
||
reproducible with data:text/html,‮쎭‼‬
Assignee | ||
Comment 5•17 years ago
|
||
Problem is that gfxPangoFontGroup::SetGlyphs gets PangoGlyphString glyphs in a different order to what it expects, and then gets all confused at: } else if (glyphCount == numGlyphs || PRUint32(logClusters[glyphIndex]) > index) { // !!! // No glyphs for this cluster, and it's not a null byte. if (!aTextRun->GetCharacterGlyphs()[utf16Offset].IsClusterContinuation()) { // It must be part of a ligature. aTextRun->SetCharacterGlyph(utf16Offset, g.SetLigatureContinuation()); } Will see if I can make this more robust.
Assignee: nobody → mozbugz
Assignee | ||
Comment 6•17 years ago
|
||
The order of the glyphs returned from pango_shape depends on the shape engine used for the language. If the language is en or el, glyphs for RTL text appear to be in reverse-logical order. If the language is ko, glyphs for RTL text appear to be in logical order. This is contrary to the documentation of PangoGlyphVisAttr in pango 1.16.2: "Clusters are stored in visual order, within the cluster, glyphs are always ordered in logical order, since visual order is meaningless; that is, in Arabic text, accent glyphs follow the glyphs for the base character." The swap_range() functions used for RTL text by pango_ot_buffer_output() in pango-1.16.2/pango/pango-ot-buffer.c and fallback_shape() in pango-1.16.2/modules/basic/basic-fc.c, swap all glyphs irrespective of cluster position, so we cannot expect glyphs within clusters are to be in logical order. No glyph swapping is performed in the Hangul shape engine (pango-1.16.2/modules/hangul/hangul-fc.c) for any text direction so we cannot expect clusters to be in "visual" order. It seems that the most we can assume about the order of the glyphs returned from pango_shape is that glyphs within a cluster are grouped together, and perhaps that the orderings are consistent within each PangoGlyphString (because there is one text direction and one shape engine). PangoGlyphInfo.attr.is_cluster_start is set in the top level of pango_shape (pango-1.16.2/pango/shape.c) based on the log_clusters information, so this should be consistent across shape engines, marking the first glyph in the cluster when glyphs are read in PangoGlyphString order.
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•17 years ago
|
||
(In reply to comment #4) > reproducible with data:text/html,‮쎭‼‬ The above testcase won't crash when CreateGlyphRunsFast is removed in attachment 263598 [details] [diff] [review] of bug 357637. But expect problems with data:text/html,‮쎭쉎
Depends on: 357637
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9alpha6
Assignee | ||
Comment 8•17 years ago
|
||
With the checkin of attachment 263898 [details] [diff] [review] in bug 357637, * the testcase in comment 4 now doesn't crash. * the original URL with UTF-16 now doesn't crash (possibly because I don't have fonts for all characters installed). * the testcase in comment 7 now only crashes if the (Korean) fonts are installed.
Assignee | ||
Comment 9•17 years ago
|
||
Always process glyph clusters in logical order. This resolves the remaining crash case, and also fixes bug 380045. Will do some more testing.
Assignee | ||
Comment 10•17 years ago
|
||
Comment on attachment 264434 [details] [diff] [review] alpha patch alpha patch has tested fine.
Attachment #264434 -
Flags: review?(roc)
+ } while (logClusters[glyphIndex] == (gint)clusterUTF8Start && + glyphIndex < numGlyphs); Need to reverse the order of these tests. Use constructor-style casts: gint(...) instead of (gint)... + } while(nextGlyphClusterStart < 0 && aUTF8[utf8Index] != '\0'); missing space between while and ( Basically it looks great.
Assignee | ||
Comment 12•17 years ago
|
||
Requested changes made. Thanks for reviewing carefully enough to spot the error.
Attachment #264434 -
Attachment is obsolete: true
Attachment #264565 -
Flags: review?(roc)
Attachment #264434 -
Flags: review?(roc)
Attachment #264565 -
Flags: superreview+
Attachment #264565 -
Flags: review?(roc)
Attachment #264565 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Whiteboard: [checkin needed]
Comment 13•17 years ago
|
||
mozilla/gfx/thebes/src/gfxPangoFonts.cpp 1.78
Flags: in-testsuite?
Whiteboard: [checkin needed]
Target Milestone: mozilla1.9alpha6 → mozilla1.9alpha5
Updated•17 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 14•15 years ago
|
||
I've fixed the pango bug pointed out in Comment 6 btw. In pango 1.26.1.
Updated•13 years ago
|
Crash Signature: [@ gfxTextRun::CompressedGlyph::IsClusterStart]
Reporter | ||
Comment 15•11 years ago
|
||
A standalone testcase that can be added to our test suite would be appreciated. https://developer.mozilla.org/en-US/docs/Mozilla/QA/Reducing_testcases
Flags: in-testsuite?
Keywords: testcase → testcase-wanted
Updated•9 years ago
|
Keywords: testcase-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•