405268 - Crash [@ gfxPangoFontGroup::CreateGlyphRunsItemizing gfxPangoFonts.cpp:1153]

Status: VERIFIED FIXED

(Core :: Graphics, defect, P2)

Description

[Bob Clary [:bc] (inactive)]

Attachment: testcase

#0  0x00cd4bb2 in pango_shape () from /usr/lib/libpango-1.0.so.0
#1  0x010f8c05 in gfxPangoFontGroup::CreateGlyphRunsItemizing (this=0x97439e8, aTextRun=0x94a5898, aUTF8=0xbf8d0a28 "â\200­", aUTF8Length=4, aUTF8HeaderLen=3) at /work/mozilla/builds/1.9.0/mozilla/gfx/thebes/src/gfxPangoFonts.cpp:1153
#2  0x010f8f60 in gfxPangoFontGroup::InitTextRun (this=0x97439e8, aTextRun=0x94a5898, aUTF8Text=0xbf8d0a28 "â\200­", aUTF8Length=4, aUTF8HeaderLength=3, aTake8BitPath=1) at /work/mozilla/builds/1.9.0/mozilla/gfx/thebes/src/gfxPangoFonts.cpp:664
#3  0x010f9295 in gfxPangoFontGroup::MakeTextRun (this=0x97439e8, aString=0xbf8d0ecc "", aLength=1, aParams=0xbf8d0fac, aFlags=17826433) at /work/mozilla/builds/1.9.0/mozilla/gfx/thebes/src/gfxPangoFonts.cpp:593
#4  0x010f24c1 in TextRunWordCache::MakeTextRun (this=0x902bd30, aText=0xbf8d24e0 "", aLength=1, aFontGroup=0x97439e8, aParams=0xbf8d2390, aFlags=17826432) at /work/mozilla/builds/1.9.0/mozilla/gfx/thebes/src/gfxTextRunWordCache.cpp:539
#5  0x010f25e0 in gfxTextRunWordCache::MakeTextRun (aText=0xbf8d24e0 "", aLength=1, aFontGroup=0x97439e8, aParams=0xbf8d2390, aFlags=17826432) at /work/mozilla/builds/1.9.0/mozilla/gfx/thebes/src/gfxTextRunWordCache.cpp:698
#6  0x016ed123 in MakeTextRun (aText=0xbf8d24e0 "", aLength=1, aFontGroup=0x97439e8, aParams=0xbf8d2390, aFlags=17826432) at /work/mozilla/builds/1.9.0/mozilla/layout/generic/nsTextFrameThebes.cpp:411
#7  0x016edf49 in BuildTextRunsScanner::BuildTextRunForFrames (this=0xbf8d3554, aTextBuffer=0xbf8d24e1) at /work/mozilla/builds/1.9.0/mozilla/layout/generic/nsTextFrameThebes.cpp:1603
#8  0x016ee2af in BuildTextRunsScanner::FlushFrames (this=0xbf8d3554, aFlushLineBreaks=1) at /work/mozilla/builds/1.9.0/mozilla/layout/generic/nsTextFrameThebes.cpp:1046
#9  0x016ef442 in BuildTextRuns (aContext=0x90c5dc8, aForFrame=0x9782ef8, aLineContainer=0x9776a5c, aForFrameLine=0xbf8d4058) at /work/mozilla/builds/1.9.0/mozilla/layout/generic/nsTextFrameThebes.cpp:987
#10 0x016ef568 in nsTextFrame::EnsureTextRun (this=0x9782ef8, aReferenceContext=0x90c5dc8, aLineContainer=0x9776a5c, aLine=0xbf8d4058, aFlowEndInTextRun=0xbf8d3b28) at /work/mozilla/builds/1.9.0/mozilla/layout/generic/nsTextFrameThebes.cpp:1772
#11 0x016f0565 in nsTextFrame::Reflow (this=0x9782ef8, aPresContext=0x9761bd0, aMetrics=@0xbf8d3d68, aReflowState=@0xbf8d3cb8, aStatus=@0xbf8d3ea4) at /work/mozilla/builds/1.9.0/mozilla/layout/generic/nsTextFrameThebes.cpp:5247
...

Comment 1

[Alfred Peng]

Same crash happens on Solaris with Firefox trunk when visiting the testcase and the following Chinese website http://www.54doctor.net/user1/51/archives/2006/6791.html.

Comment 2

[Alfred Peng]

Attachment: patch v1

Comment 4

[Karl Tomlinson (:karlt)]

Comment on attachment 294742 [details] [diff] [review]
patch v1

Doing the check before pango_glyph_string_new() would avoid the need for the missing pango_glyph_string_free().

The pango_item_free() that was added at the end of the loop in bug 379433, appears to have been lost in bug 362682.
Are you able to add that back in please?

We're also missing the g_list_free(), pango_item_free(), pango_font_description_free() , and g_object_unref(context) on the early return.  The cleaning up in this function needs a redesign.  Perhaps gotos and a separate loop over items to free them.

Comment 5

[Alfred Peng]

Attachment: patch v2

update the patch based on karlt's suggestion.

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 295911 [details] [diff] [review]
patch v2

Thank you sorting out the memory management.  That looks good now.

However, there is more to fix up here than just the crash.
(Sorry I didn't look into this hard enough yesterday.)

The crash is one symptom of the fact that pango_shape only gets glyphs for the
characters before the first NUL character.  The following test case shows an "a"
and a missing glyph box for NUL but also (incorrectly) uses a missing glyph
box for the b.

data:text/html,<span style="text-rendering:optimizeLegibility">a&#0b;

If there are no characters before the first NUL (e.g. the a is removed), then
pango_shape crashes.

There was a similar issue with pango_break in SetupClusterBoundaries in bug
380153.

What we need is a loop around pango_shape and SetGlyphs that
iterates over each segment of non-NULL characters within the one PangoItem.

SetupClusterBoundaries need not be in this loop because it already handles
NULs and so could be before the loop, but it may be tidier to have it within
the loop and exchange the outside loop in SetupClusterBoundaries for a comment
or assertion that the UTF8 string must not contain NULs.

aTextRun->SetMissingGlyph should be called for the NUL characters

aTextRun->AddGlyphRun should be outside (before) this loop as the font
doesn't change.

g_utf8_strlen may be useful in finding the length of non-NULL-character
segments.

SetGlyphs won't need its NUL handling any more once this is done, but diving
in there to replace that with an assertion is an optional exercise.  A comment
that it is now unnecessary is sufficient.

cvs update (revision 1.121 is now current)

Comment 7

[Karl Tomlinson (:karlt)]

Behdad, pango_itemize has been happily handling NUL characters for us since before the changes in 362682.  Do you think we are safe to continue to assume that pango_itemize won't truncate text at the first NUL?

If not then we'll need to loop over pango_itemize instead, which may require adding the header in each iteration or should we just use the same text pointer in each iteration with different start_indices?

Comment 8

[Karl Tomlinson (:karlt)]

After conversation with Behdad, it sounds like NULs should be safe in pango_itemize.  If anything is going to change, it is that more functions will treat NULs as characters (not terminators).

Comment 9

[Alfred Peng]

Attachment: patch v3

karlt, thanks for the nice review.

Not quite sure whether the new patch addresses all your comments correctly. Please help review it again.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 296576 [details] [diff] [review]
patch v3

+        // It's now unncecessary to do NUL handling here.
+        NS_ASSERTION(glyphClusterStart >= 0, "Shouldn't have NUL");

The assertion can be before "while (utf8Index < aUTF8Length)" (on
nextGlyphClusterStart) as in other iterations nextGlyphClusterStart is tested.
I suggest this message "No glyphs! - NUL in string?"

         // need to append glyph runs here.
         PangoGlyphString *glyphString = pango_glyph_string_new();
         if (!glyphString)
-            return; // OOM
-
-        pango_shape(aUTF8 + offset, length, &item->analysis, glyphString);

This is no longer the right place for pango_glyph_string_new().  The best
place would be before the loop over items.  (pango_shape will resize as
necessary.)

+        guint len = 0;

Declare this when first set in the loop for clarity of initialization
and intended scope.  (Let the compiler can deal with optimal memory use for
stack variables.)

+        guint index = 0;
+        gunichar ch;

Use utf16Offset and index won't be needed.  ch also won't be needed - see below.

+            ch = g_utf8_get_char(p);
+            if (ch == 0) {
+                aTextRun->SetMissingGlyph(index, ch);
+                p = g_utf8_next_char(p);

It is safe to just compare *p against '\0', and use ++p, as a NUL is always one byte.

+            len = end - p;
+            pango_shape(p, len, &item->analysis, glyphString);
+            SetGlyphs(aTextRun, font, p, len, &utf16Offset, glyphString, spaceWidth, PR_FALSE);

Add a comment to explain why this is necessary, such as "pango_shape treats
NULs as string terminators".

The len argument to SetGlyphs should be the offset of the NUL (if present).
"len = g_utf8_strlen(p, end - p)" should do what you want.

SetupClusterBoundaries would fit nicely if moved to here, and then the outer
loop in SetupClusterBoundaries could be removed with just an assertion ch != 0
instead of a break on ch == 0.  (Maybe in the future SetupClusterBoundaries
and SetGlyphs should be merged into one function.)

+
+            while (p < end) {
+                ++index;
+                NS_ASSERTION(!IS_SURROGATE(ch), "Shouldn't have surrogates in UTF8");
+                if (ch >= 0x10000) {
+                    ++index;
+                }
+                p = g_utf8_next_char(p);
+
+                if (ch == 0) {
+                    aTextRun->SetMissingGlyph(index - 1, ch);
+                    break;
+                }
+                ch = g_utf8_get_char(p);
+            }
+        }

This shouldn't be needed with the above changes.  "p += len" should be enough.

Comment 11

[Alfred Peng]

karlt, the updated patch is here: http://pastebin.mozilla.org/302203. But it's still broken:-(

(gecko:5307): Pango-WARNING **: shaping failure, expect ugly output. shape-engine='BasicEngineFc', font='Arial 7.998046875', text=''

(dbx 2) up
Current function is gfxPangoFontGroup::CreateGlyphRunsItemizing
 1205               pango_shape(p, len, &item->analysis, glyphString);
(dbx 12) p len
len = 1U
(dbx 24) p p
p = 0x8043d3f "…"
(dbx 25) x 0x8043d3f /4x 
0x08043d3f:      0x80e2 0x00a6 0x74f6 0x043d
(dbx 26) p end - p
end-p = 3

It seems to be related to the use of g_utf8_strlen. It returns invalid data in some cases and then crashes in pango_shape. Any suggestions for this?

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to comment #11)

Sorry, my mistake.  g_utf8_strlen gives characters but we want bytes.
I guess we need to look for a NUL ourselves.

Comment 13

[Karl Tomlinson (:karlt)]

pango_glyph_string_new() can go before "GList *pos = items;"

Comment 14

[Alfred Peng]

Attachment: patch v4

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 296803 [details] [diff] [review]
patch v4

+        NS_ASSERTION(ch != 0, "Shouldn't have NUL in UTF8");

NUL is actually valid UTF-8:  Perhaps "Shouldn't have NUL in pango_break"

+    // need to append glyph runs here.

Ditch this comment, I think.  I don't know what it means and it doesn't seem
to make much sense here.

+            guint len = 0;
+            const gchar *q = p;
+            while (q < end) {
+                ++len;
+                NS_ASSERTION(!IS_SURROGATE(*q), "surrogates should not appear in UTF8");
+                if (*q >= 0x10000) {
+                    // Skip surrogate
+                    ++len;
+                }
+                ++q;
+                if (*q == 0) {
+                    break;
+                }
+            }

No need to look for surrogates.  p is UTF-8 and the surrogate code was for
keeping track of utf16Offset (which SetGlyphs will do).  NUL won't occur as
part of another UTF-8 character, so all that is necessary is to look for a
zero byte (strnlen if that were not platform-dependent).  Perhaps:

const gchar *text = p;
do {
   ++p;
} while(p < end && *p != 0);
gint len = p - text;

pango_shape(text, len, ...

Then "p += len" won't be needed.

+    pango_glyph_string_free(glyphString);
+
+out:

I would "if (glyphString) pango_glyph_string_free(glyphString)" after out, so
that only one pango_glyph_string_free call is required.

Comment 16

[Alfred Peng]

Attachment: patch v5

Comment 17

[Alfred Peng]

Attachment: malformed display

Met another issue with character display. The string "“烈士”，" is showed correctly in Firefox 2.0.0.x. But something seems to be wrong with Firefox 3.0 as the image shows. Is this an known issue? Or a new bug should be filed?

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 296913 [details] [diff] [review]
patch v5

             if (offset + length <= aUTF8HeaderLen) {
                 pango_item_free(item);
                 continue;

No need for this pango_item_free now.
r+ with that change.

Thank you for tidying up all of this.

Comment 19

[Karl Tomlinson (:karlt)]

(In reply to comment #17)
> Created an attachment (id=296915) [details]
> malformed display
> 
> Met another issue with character display. The string "“烈士”，" is
> showed correctly in Firefox 2.0.0.x. But something seems to be wrong with
> Firefox 3.0 as the image shows. Is this an known issue? Or a new bug should be
> filed?

Not an issue I know about, but I'm seeing left/right double quotation mark fine with Minefield here.  It may be a font issue - try some other fonts.  Copying to another application would test whether it is an encoding issue.  If nothing shows up, better to submit and dupe than not submit at all.

Comment 20

[Alfred Peng]

Attachment: patch v5.1

Comment 21

[Reed Loden [:reed]]

Ignore the flag change. I hit my converter script accidentally. :(

Comment 22

[Alfred Peng]

Checking in ./gfx/thebes/src/gfxPangoFonts.cpp;
/cvsroot/mozilla/gfx/thebes/src/gfxPangoFonts.cpp,v  <--  gfxPangoFonts.cpp
new revision: 1.124; previous revision: 1.123
done

Comment 23

[Alfred Peng]

Attachment: bustage

Could this be the fix for the bustage?

Comment 24

[Jesse Ruderman]

I added a "var" to the testcase and checked it in as a crashtest.

Comment 25

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9b3) Gecko/2008020513 Firefox/3.0b3 - no crash on testcase