377976 - Check for null mDetailedGlyphs before referencing its members for missing CompressedGlyphs.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Karl Tomlinson (:karlt)]

In gfxFont.* there are cases where members of the nsAutoArrayPtr
mDetailedGlyphs are referenced without checking that mDetailedGlyphs has been
set up.

This was originally noticed in bug #377942, but, with that bug now fixed, I
have no test case that shows a problem.

Currently the only code path that looks like it should sometimes result in
missing CompressedGlyphs while mDetailedGlyphs remains null is
in SetGlyphsForCharacterGroup() in gfxAtsuiFonts.cpp when it calls
SetCharacterGlyph(index, g.SetMissing()).

But it seems reasonable that missing glyphs may not have any DetailedGlyph
information and gfxFont::Draw has already been written to handle these cases.

I'll attach a patch.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: patch

This patch removes the assertions on mDetailedGlyphs in cases where the
CompressedGlyphs may be missing (as opposed to complex) and handles the cases
when mDetailedGlyphs is null.  It also makes changes to clarify that the loops
over details only terminate on the break.

I haven't removed the assertion from gfxTextRun::ComputeClusterAdvance() as it
looks to be only when IsLigatureContinutation, which I guess should have an
entry in mDetailedGlyphs.

Comment 2

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

+        if (mDetailedGlyphs) {
+            return mDetailedGlyphs[aCharIndex];
+        } else {
+            return nsnull;
+        }

I'd just write "mDetailedGlyphs ? ... : nsnull"

These loops/conditionals could be simplified by writing
  DetailedGlyph *details = GetDetailedGlyphs(i);

We can have IsLigatureContinuation with no mDetailedGlyphs --- ligatures don't require mDetailedGlyphs to be created.

Comment 3

[Karl Tomlinson (:karlt)]

Currently gfxTextRun::ComputeClusterAdvance() references mDetailedGlyphs only if not IsSimpleGlyph.  (If not) It asserts IsComplexCluster.  Can we have IsLigatureContinuation with IsMissing?  If so, should the IsComplexCluster assert be changed to IsComplexOrMissing or just removed?

Comment 4

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

> Can we have IsLigatureContinuation with IsMissing?

Depends on the platform. I think ComputeClusterAdvance should lose the assertions and just return 0 if !mDetailedGlyphs or !mDetailedGlyphs[aClusterOffset].

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: updated patch

Made similar changes to ComputeClusterAdvance.
Using ?: and GetDetailedGlyphs [thank you].

Comment 6

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

ComputeClusterAdvance needs to return 0 if (!details).

The rest is OK.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: updated patch

ComputeClusterAdvance now always returns a value, thank you.

Comment 8

[Karl Tomlinson (:karlt)]

> Currently the only code path that looks like it should sometimes result in
> missing CompressedGlyphs while mDetailedGlyphs remains null is ...

Or if glyphs initialized (to missing) in the gfxTextRun constructor are (for some reason) never set with Set*Glyph().

Comment 9

[Nickolay_Ponomarev]

Checking in gfxFont.cpp;
/cvsroot/mozilla/gfx/thebes/src/gfxFont.cpp,v  <--  gfxFont.cpp
new revision: 1.33; previous revision: 1.32
done

Comment 10

[Karl Tomlinson (:karlt)]

The gfxFont.h changes in the same patch also need checkin.

Comment 11

[Nickolay_Ponomarev]

Bah. Somehow I missed that. Thanks for catching my mistake!

Checking in gfxFont.h;
/cvsroot/mozilla/gfx/thebes/public/gfxFont.h,v  <--  gfxFont.h
new revision: 1.52; previous revision: 1.51
done