404624 - ASSERTION: Invalid offset with ::first-letter and letter-spacing

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, I get this assertion in current debug trunk build:
###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file c:/m
ozilla-build/mozilla/gfx/thebes/src/gfxSkipChars.cpp, line 92
###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file c:/m
ozilla-build/mozilla/gfx/thebes/src/gfxSkipChars.cpp, line 92

Comment 1

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

This should get fixed, memory errors can develop

Comment 2

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

Attachment: fix

The problem here is that we're passing too-long a length to PropertyProvider when we construct it. The textrun may not map all the content in the flow chain, represented by GetInFlowContentLength(), so we get these errors when we check IsInBounds. My solution here is to allow PR_INT32_MAX to be used as the content length to indicate we don't really know or care what it is. Most PropertyProvider methods don't actually need it except for bounds checking.

Comment 3

[Simon Montagu :smontagu]

Why don't we need to check for PR_INT32_MAX whenever we call  PropertyProvider.GetOriginalLength(), e.g. nsTextFrame::PaintTextSelectionDecorations and elsewhere?

Comment 4

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

Yeah, I should do that. Sorry, I'll post a new patch.

Comment 5

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

Attachment: fix

Updated patch with that. The callers of GetOriginalLength construct their own PropertyProvider so it's pretty easy to see that assert won't fire.

Comment 6

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

checked in

Comment 7

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

I just backed this out because I think it may be causing a mochitest failure. (It had been backed out and relanded due to suspicious Tp.)

Comment 8

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

Tinderbox seems to show that the mochitest failure went away before I backed this out, so I relanded it.

Comment 9

[Martijn Wargers (dead)]

With my updated debug build, I don't see the assertion anymore, so verified fixed.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4477e9235df0
Add crashtest.  r=mats

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4477e9235df0