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]

Jonathan are you the right one to look at this?

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? :)

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.

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

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?)

(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.)

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.

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.)

back to the drawing board then :-(

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

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.

(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.

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.

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

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

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

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