385423 - "ASSERTION: Couldn't find glyph for trailing marker" and crash [@ SetGlyphsForCharacterGroup]

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Jesse Ruderman]

Testcase 1 triggers:

###!!! ASSERTION: Couldn't find glyph for trailing marker: 'glyphRecords[0].originalOffset == aSegmentLength*2', gfxAtsuiFonts.cpp, line 802

###!!! ASSERTION: invalid array index: 'i < Length()', nsTArray.h, line 318

Crash [@ SetGlyphsForCharacterGroup] with EIP=0 on top.


Testcase 2 triggers the first assertion above, plus:

###!!! ASSERTION: Invisible character grouped with other characters?: 'firstOffset == lastOffset', gfxAtsuiFonts.cpp, line 693

Comment 1

[Jesse Ruderman]

Attachment: testcase 1

Comment 2

[Jesse Ruderman]

Attachment: testcase 2

Comment 3

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

The problem here is that ATSUI is treating \r as a newline which affects the bidi algorithm and messes up our interpretation of the returned glyphs. We need to strip \r (and \n and \t) before they reach ATSUI, somehow, as gfxFontGroup::IsInvisibleChar requires. Not sure yet of the best way to do this.

Comment 4

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

gfxTextRunWordCache already does this, which is why you only see this when we create textruns by special means, such as via text-transform.

The question is who should be responsible for making these characters invisible ... the gfxFontGroup subclasses or callers to gfxFontGroup::MakeTextRun. Hmm. Any thoughts, Stuart/Vlad?

Comment 5

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

I think probably the thing to do is to modify the existing gfxGlobalTextRunCache to be based on the word cache and then use it to create all textruns, which will take care of this issue for all platforms and all textrun clients.

Comment 6

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

That's something I intended to do anyway ... we don't really need the other textrun cache, so this change will be a net code decrease.

Comment 7

[Jesse Ruderman]

I think I see similar problems with the "line separator" character.  This doesn't surprise me, given bug 377231 and bug 377461.

Comment 8

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

Attachment: fix

Okay, here's the fix that reorganizes textrun caching and puts responsibility for sanitizing text onto the caller of gfxFontGroup::MakeTextRun --- which with this patch, is always gfxTextRunWordCache.

On the gfx side, I've got the following changes:
-- do not try to handle \r, \n or \t characters specially in platform textrun creation code. Those characters should not be passed in as input.
-- have gfxTextRunWordCache indicate whether the textrun is referenced by the cache by setting a flag in the textrun. This is more convenient for everyone than returning a boolean.
-- rename gfxTextRunGlobalCache to gfxTextRunCache and have it use gfxTextRunWordCache instead of the old gfxTextRunCache. This means gfxTextRun(Global)Cache always creates a new textrun now, instead of potentially returning an old one, but that simplifies things. Speedwise it could go either way because we get the benefits of the word cache now, but it probably doesn't matter because this isn't used by text frames anymore, only code that's still using the old nsIRenderingContext APIs.
-- I think the old way I had gfxTextRunGlobalCache only able to free textruns at the next event loop (because there's no explicit free/release API) was bad API. It's conceivable that we might want to write code that uses lots of textruns from the cache where we might suck up tons of memory before returning to the event loop. So I added a release protocol facilitated by an AutoReleaser object; this way at least we know which textruns are being used and which aren't so in the future if memory gets low we can flush the textrun cache anytime we want.
-- I made gfxTextRunWordCache use a private global cache object instead of being instantiated by different users. This means that the all textrun users now use the same cache.

On the layout side:
-- Use updated APIs
-- Use gfxTextRunCache from nsTextRunTransformations (which fixes this bug)
-- Use gfxTextRunCache from SVG (which would fix related bugs in SVG text)

Looking for vlad-review on the gfx parts and simon-review on the layout parts.

Comment 9

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

If/after this goes in, I'll do a followup to extend gfxFontGroup::IsInvalidChar to include Unicode LSEP and PSEP and anything else Jesse can come up with :-)

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 269925 [details] [diff] [review]
fix


Consider calling gfxTextRunCache::AutoReleaser something like gfxTextRunCache::AutoTextRun or AutoReleaseTextRun, to make it more obvious what type the object behaves like, but other than that, looks fine.

Comment 11

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

checked in with Vlad's comment

Comment 12

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

Attachment: handle ZWSP, PSEP and LSEP too

Simple followup: add ZWSP, PSEP and LSEP to the list of characters that should not be passed to gfxFontGroup::MakeTextRun and that will be automatically treated as zero-width invisible by gfxTextRunWordCache.

I've added ZWSP as a workaround for bug 386606.

Comment 13

[Simon Montagu :smontagu]

> Simple followup: add ZWSP, PSEP and LSEP to the list of characters that should
> not be passed to gfxFontGroup::MakeTextRun and that will be automatically
> treated as zero-width invisible by gfxTextRunWordCache.

I'm concerned that doing that might affect shaping and ligature behaviour.

Comment 14

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

It will disable ligatures and shaping across ZWSP/PSEP/LSEP. Are we supposed to do ligatures and shaping across a ZWSP/PSEP/LSEP? That sounds odd

Comment 15

[Simon Montagu :smontagu]

That's great. I was afraid that if we suppressed the ZWSP/PSEP/LSEP from the text run it would make us do ligatures and shaping where we shouldn't.

Comment 16

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

checked that in.

Comment 17

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011009 Firefox/3.0b3pre ID:2008011009 - no crash and no assertion on testcases -> Verified fixed

Comment 18

[Jesse Ruderman]

I don't see any assertions on branch with these testcases, so I'm making this bug report public.

Comment 19

[Jesse Ruderman]

Crashtests checked in.