375864 - Crash [@ gfxTextRun::CompressedGlyph::IsClusterStart] when viewing URL as UTF-16

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mats Palmgren (inactive)]

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)

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

Worth attaching the page to the bug in case it changes...

Comment 3

[Mats Palmgren (inactive)]

Attachment: URL as of 2007-03-29 with <base href=...>

Comment 4

[Karl Tomlinson (:karlt)]

reproducible with data:text/html,‮쎭‼&#x202c

Comment 5

[Karl Tomlinson (:karlt)]

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.

Comment 6

[Karl Tomlinson (:karlt)]

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.

Comment 7

[Karl Tomlinson (:karlt)]

(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,‮쎭쉎

Comment 8

[Karl Tomlinson (:karlt)]

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.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: alpha patch

Always process glyph clusters in logical order.

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

Will do some more testing.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 264434 [details] [diff] [review]
alpha patch

alpha patch has tested fine.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+            } 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.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: patch v1

Requested changes made.
Thanks for reviewing carefully enough to spot the error.

Comment 13

[Nickolay_Ponomarev]

mozilla/gfx/thebes/src/gfxPangoFonts.cpp  1.78

Comment 14

[Behdad Esfahbod]

I've fixed the pango bug pointed out in Comment 6 btw.  In pango 1.26.1.

Comment 15

[Mats Palmgren (inactive)]

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