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)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: MatsPalmgren_bugz, Assigned: karlt)

References

()

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files, 1 obsolete file)

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)
Attached file stack
Worth attaching the page to the bug in case it changes...
Flags: blocking1.9?
reproducible with data:text/html,‮쎭‼&#x202c
Keywords: testcase
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
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
(In reply to comment #4)
> reproducible with data:text/html,‮쎭‼&#x202c

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
Blocks: 380045
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.
Attached patch alpha patch (obsolete) — Splinter Review
Always process glyph clusters in logical order.

This resolves the remaining crash case, and also fixes bug 380045.

Will do some more testing.
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.
Attached patch patch v1Splinter Review
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)
Whiteboard: [checkin needed]
mozilla/gfx/thebes/src/gfxPangoFonts.cpp  1.78
Flags: in-testsuite?
Whiteboard: [checkin needed]
Target Milestone: mozilla1.9alpha6 → mozilla1.9alpha5
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
I've fixed the pango bug pointed out in Comment 6 btw.  In pango 1.26.1.
Crash Signature: [@ gfxTextRun::CompressedGlyph::IsClusterStart]
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?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: