Closed Bug 377976 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: Graphics, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: karlt, Assigned: karlt)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #262063 - Flags: review?(roc)
Status: NEW → ASSIGNED
+        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.
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?
> 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].
Attached patch updated patch (obsolete) — Splinter Review
Made similar changes to ComputeClusterAdvance.
Using ?: and GetDetailedGlyphs [thank you].
Attachment #262063 - Attachment is obsolete: true
Attachment #262080 - Flags: review?(roc)
Attachment #262063 - Flags: review?(roc)
ComputeClusterAdvance needs to return 0 if (!details).

The rest is OK.
Attached patch updated patchSplinter Review
ComputeClusterAdvance now always returns a value, thank you.
Attachment #262080 - Attachment is obsolete: true
Attachment #262083 - Flags: review?(roc)
Attachment #262080 - Flags: review?(roc)
> 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().
Severity: minor → normal
Whiteboard: [checkin needed]
Checking in gfxFont.cpp;
/cvsroot/mozilla/gfx/thebes/src/gfxFont.cpp,v  <--  gfxFont.cpp
new revision: 1.33; previous revision: 1.32
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha5
Flags: in-testsuite?
The gfxFont.h changes in the same patch also need checkin.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [checkin needed]
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
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
You need to log in before you can comment on or make changes to this bug.