849606 - "ASSERTION: Invalid offset" and crash with textPath and "svg.text.css-frames.enabled" preffed on

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

With:
  user_pref("svg.text.css-frames.enabled", true);

###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /Users/jruderman/trees/mozilla-central/gfx/thebes/gfxSkipChars.cpp, line 61

###!!! ASSERTION: aPos out of range: 'aPos < GetLength()', file ../../dist/include/gfxFont.h, line 2471

Crash [@ gfxShapedText::CompressedGlyph::IsClusterStart]

Comment 1

[Jesse Ruderman]

Attachment: stacks

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Jonathan are you the right one to look at this?

Comment 3

[Daniel Holbert [:dholbert]]

heycam would be the obvious person to have look at this, except he's on vacation at the moment. 

The next-obvious would be longsonr, since he's been working on improving this "svg.text.css-frames.enabled"-dependent code while heycam's been away.

longsonr, any chance you've got cycles to take this? :)

Comment 4

[Robert Longson [:longsonr]]

I don't yet know how to fix it. I was hoping that fixing other bugs would help but I'm just don't quite know enough yet. I'll keep trying.

Comment 5

[Robert Longson [:longsonr]]

This can't be sec-critial as it's preffed off though, must be only sec-high.

Comment 6

[Daniel Holbert [:dholbert]]

longsonr says this affects Aurora **if** you set the pref, and we chatted briefly on IRC about strategies for preventing this bug there.

My impression is that we don't actually need to worry about Aurora (which will eventually become beta and then release), because it's preffed off there -- only users who've manually toggled the pref in about:config would be at risk.  We could e.g. yank out this code, or forcibly prevent people from setting the pref & turn off all the tests that depend on being able to set the pref, if we need to, but I don't think we normally go to those measures for pref-dependent security bugs -- right?  (dveditz or dbolter, could you sanity-check my assessment there?)

Comment 7

[Daniel Holbert [:dholbert]]

(Also, reassigning to longsonr to reflect reality, since I believe he's working a fix for this on trunk, and jfkthame was only assigned as a guess.)

Comment 8

[Robert Longson [:longsonr]]

Attachment: patch

So broadly speaking display="none" elements should work the same as if they don't exist.

The first problem is that when CharIterator::CharIterator is constructed mTextElementCharIndex is initialised but mGlyphStartTextElementCharIndex is not. 

Once I fixed that the crashtest then crashes in TextRenderedRunIterator::Next instead as it adds the 2 undisplayed characters to the index on every loop rather than just adding them once.

With those fixed we run into the reftest actually being wrong as it expects <text x="1 2 3 4 5 6">ab<tspan display="none">cd</tspan>ef</text> to use 5 and 6 as the locations of ef rather than 3 and 4. The other lines in the testcase are correct. Opera's rendering matches ours with this patch.

Comment 9

[Cameron McCormack (:heycam)]

Thanks for looking at this while I'm away, Robert.  A quick comment while I'm online:

My intention was for display:none characters in the DOM to still consume the positions specified in x="" etc.  IIRC browsers were not consistent in this, and I think it makes sense to limit the effect of style property changes on which position attribute values correspond to which DOM characters.  (We still have white-space changing this mapping of position attribute values to DOM characters, but that should be the only one.)

Comment 10

[Robert Longson [:longsonr]]

back to the drawing board then :-(

Comment 11

[Robert Longson [:longsonr]]

Attachment: updated patch

This one passes the svg/text reftests without changing them. Including the ones that check that display: none text still consumes positioning attributes.

Comment 12

[Robert Longson [:longsonr]]

Attachment: right patch this time

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 725854 [details] [diff] [review]
right patch this time

I don't really know how these iterators are supposed to work & what the various edge-cases involved are, but it looks like jwatt reviewed this code (for bug 655877), so I suspect he can review this more efficiently & effectively than I can.  Kicking review over to him.

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> My impression is that we don't actually need to worry about Aurora (which
> will eventually become beta and then release), because it's preffed off
> there -- only users who've manually toggled the pref in about:config would
> be at risk.

That's right. If the feature is preffed-off we can mark that release "disabled" and not worry about it (which I've done for Firefox 21). I marked Firefox 22 as "affected" assuming you're trying to land bug 839955 in that time-frame. There's not much time left, however, so if you know it's not landing go ahead and mark that one "disabled" too.

(In reply to Robert Longson from comment #5)
> This can't be sec-critical as it's preffed off though

That rationale is intended for optional features that will always require a configuration change to trigger. This feature is intended to be standard behavior once it fully lands, and you should treat this bug as the severity it would be when it's landed as intended.

Do you ever write into these text runs? In the code you're patching it looks like you're only reading. If the worst case is incorporating random chunks of memory into displayed text we'd usually call that sec-hight (data leakage) at most. If we go wrong trying to read a more complex object it could sec-critical.

Comment 15

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3550c269a565

Comment 16

[Robert Longson [:longsonr]]

Correct, we're not writing into the text runs here.

I doubt we we'll land bug 839955 for Firefox 22 as I think it's likely we'd want to fix more of the svgtext dependent bugs first and I suspect heycam and I can't complete that before Firefox 22 development ends.

Comment 17

[Robert Longson [:longsonr]]

This patch also fixes bug 842630. I'll close that as fixed once this lands on central.

Comment 18

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/3550c269a565

Comment 19

[Robert Longson [:longsonr]]

I tried again and it seems that this doesn't fix bug 842630 after all.

Comment 20

[Alex Keybl [:akeybl]]

Any plans to prioritize this for this cycle? FF22 (the first affected version, according to status flags) is being released in 5 weeks.

Comment 21

[Robert Longson [:longsonr]]

This is preffed off so I didn't think it necessary.