###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file layout/generic/nsTextFrame.h, line 307 ###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file nsTextFragment.h, line 184 ###!!! ASSERTION: integer overflow: 'mMaxTextLength <= mMaxTextLength + aFrame->GetContentLength()', file layout/generic/nsTextFrameThebes.cpp, line 1187 ###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file gfx/thebes/src/gfxSkipChars.cpp, line 92 Security-sensitive because these assertions scare me. Related to bug 429458?

I can repro all the assertions from comment 0 using my up-to-date Linux mozilla-central build. (I actually get 3 consecutive copies of the "negative length" assertion.) Platfom --> All/All

If I add a character inside the second binding's <content> (as exemplified in this attached testcase), I get: - 2 copies of the "negative length" assertion - 1 copy of the "integer overflow" assertion - a crash in nsTextFrameUtils::TransformText (I'll post a backtrace in a minute) Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090102 Minefield/3.2a1pre

(oops, here's the testcase)

Here's the GDB log of the crash & the backtrace. Strangely, the backtrace only includes 2 stack levels, and then it hits "0x00000000 in ?? ()". Does that mean we're accidentally overwriting chunks of the stack? That sounds bad...

You can replace "2!" in the earlier testcases with "<alphanumeric><punctuation_or_space>" (in either order), and the testcase still asserts and crashes, as demonstrated here.

(In reply to comment #2) > If I add a character inside the second binding's <content> [I get a crash] I get the same crash if I put the character directly inside span 's'. A space character triggers the crash there, as well (though it doesn't trigger a crash when put into the <content> of binding 'y' instead of into span 's'). One more note: If I put any character (I've tested alphanumeric, space, & punctuation) just inside div 'd' (before span 's') then I get no crash & no assertions. I think this is true of all testcases posted so far.

(In reply to comment #4) > Strangely, the backtrace only includes 2 stack levels, and then it hits > "0x00000000 in ?? ()". Does that mean we're accidentally overwriting chunks of > the stack? That sounds bad... Yup, this is a classic integer overflow + buffer-overrun situation. What happens is: A) In BuildTextRunForFrames, we have contentLength = -1 B) That gets passed into nsTextFrameUtils::TransformText as "PRUint32 aLength". The conversion to unsigned yields a value of 4294967295. C) TransformText writes aLength characters to the buffer 'aOutput', which only has capacity 4096. We write (way) beyond the end of the buffer, & we fail. Note: aOutput's memory is allocated up a few stack levels in BuildTextRunsScanner::FlushFrames, quoted here: 1156 nsAutoTArray<PRUint8,BIG_TEXT_NODE_SIZE> buffer; [snip] 1159 textRun = BuildTextRunForFrames(buffer.Elements()); http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#1156

(In reply to comment #4) > Strangely, the backtrace only includes 2 stack levels Here's a more useful backtrace, taken just after we enter TransformText but before we've blown away the stack. Note the 'aLength=4294967295' at level 0.

This crashes Firefox v3.0, too, btw. (I just tried using testcase #5) Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.5) Gecko/2008121622 Ubuntu/8.10 (intrepid) Firefox/3.0.5

Is there a compiler warning which would have fingered this if we'd made the effort to fix it? The warning-blame site seems to be down now, not entirely surprising given it was promoted as an experimental service or somesuch, so I can't track down if one is present (and don't have the time now to do a build to see for myself).

I looked for a regression range, using testcase 4. It turns out there were two regressions -- first we started hanging, and then we started crashing. (I don't know when the various assertions appeared, because I'm using non-debug nightly builds to find the regression range.) No Problem --> Hang: ==================== Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007081504 Minefield/3.0a8pre Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007081604 Minefield/3.0a8pre Bonsai link: http://tinyurl.com/8v57p4 -- possibly bug 385270? Hang --> Crash: =============== Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111104 Minefield/3.0b2pre Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111204 Minefield/3.0b2pre Bonsai link: http://tinyurl.com/8rgst4 -- possibly bug 402427?

(In reply to comment #12) > No Problem --> Hang: > Bonsai link: http://tinyurl.com/8v57p4 -- possibly bug 385270? Confirmed that this initial regression was from bug 385270. I took a CVS checkout from just after that bug's landing (15 Aug 2007 11:40 PDT), and I get 2000 asserts[1] and then an abort[2]. When I back out that bug's patch[3], I get good behavior -- no asserts & no crash/hang/abort. [1] The asserts are all repetitions of this pair: ASSERTION: redo line on totally empty line: 'aState.IsImpactedByFloat()', file /mozilla/layout/generic/nsBlockFrame.cpp, line 3428 ASSERTION: unconstrained height on totally empty line: 'NS_UNCONSTRAINEDSIZE != aState.mAvailSpaceRect.height', file /mozilla/layout/generic/nsBlockFrame.cpp, line 3430 [2] The abort is accompanied by this message: Block(div)(0)@0x9c6d504: yikes! spinning on a line over 1000 times! and it's here in the code: http://tinyurl.com/6wem6x [3] The patch posted on the bug didn't back out cleanly, so I backed out the patch as-it-was-landed, via the commands here: http://tinyurl.com/9l6jwp

(In reply to comment #12) > Hang --> Crash: > =============== > Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111104 > Minefield/3.0b2pre > Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007111204 > Minefield/3.0b2pre > Bonsai link: http://tinyurl.com/8rgst4 -- possibly bug 402427? Turns out the hang-to-crash change was caused by bug 397961's checkin. (Verified by backing its patch out of a trunk-checkout from just after the behavior change.)

Not wanted on 1.8.1 given the regression range and bugs involved.

In testcase 5, the problem seems to be the frame reconstruction that happens when we apply the binding on click. The frame tree looks correct before the click. But afterwards we have Block(div)(0)@0x2178fed4 {0,0,59520,1152} [state=00101010] sc=0x2178dea0(i=1,b=0)< line 0x11ecdc4: count=3 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0xc141] {58346,0,1174,1152} ca={0,0,59520,1152} < Inline(span)(-1)@0x2178ff50 next=0x11eceb8 next-continuation=0x11eceb8 {59040,96,480,960} [state=00a00400] [content=0x8caf4c0] [sc=0x2178f7e4]< Text(0)@0x11ecc10[0,1,F] next=0x11eccfc next-continuation=0x11ece74 {0,0,480,960} [state=d0220000] [content=0x8caf510] sc=0x2178ff88 pst=:-moz-non-element< "x" > Inline(span)(0)@0x11eccfc {0,0,0,0} [state=00000402] [content=0x8cb6650] [sc=0x11ecc74]< Text(0)@0x11ecd84[0,1,T] {0,0,0,0} [state=00000402] [content=0x8cb66a0] sc=0x11ecd34 pst=:-moz-non-element< "s" > > > Inline(span)(-1)@0x11eceb8 next=0x11ecef0 prev-continuation=0x2178ff50 next-continuation=0x11ecef0 {58720,96,320,960} [state=00200000] [content=0x8caf4c0] [sc=0x2178f7e4]< Text(0)@0x11ece74[1,1,T] prev-continuation=0x11ecc10 {0,0,320,960} [state=d0020000] [content=0x8caf510] sc=0x2178ff88 pst=:-moz-non-element< "!" > > Inline(span)(-1)@0x11ecef0 prev-continuation=0x11eceb8 {58346,96,374,960} [state=00600400] [content=0x8caf4c0] [sc=0x2178f7e4]<> > > > which looks bad. In particular, "x"s next-continuation is "!" but somehow the "s" got jammed in the middle. Then I suspect bidi resolution gets all confused and sets up the wrong offsets which causes reflow to explode. The "s" shouldn't even be in the frame tree anymore, if I read this XBL right.

Slightly simplified version of testcase 5; we don't need the inner XBL binding, all we need to trigger the bug is frame reconstruction of the span "s".

OK, so GetInsertionPoint is returning the wrong frame. It's returning the first continuation of the span (0x2178ff50 above), not the last continuation.

No, GetInsertionPoint is OK, the problem is that ContentInserted is not skipping all the way to the last continuation of parentFrame when it decides to append (because we have no specified prevSibling or nextSibling). ContentAppended has code for this, we just need to use that in ContentInserted too.

This testcase shows the bug visually, without crashing or asserting and without involving RTL. The "s" should be after the "b", but it's rendered before the "b".

Comment on attachment 357266 [details] [diff] [review] fix Looks good.

Pushed http://hg.mozilla.org/mozilla-central/rev/7af1fe6b7ab4 I pushed the reftest. The crashtests should be added when this bug gets decloaked.

roc: Can you get this landed on 1.9.1 soon so it can bake and we can get a 1.9.0 patch worked up and ready?

Patch pushed to 1.9.1 on roc's behalf: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3b8c370b2be9

roc: Can you work up a 1.9.0 patch for this blocking bug?

This bug's patch (attachment 357266 [details] [diff] [review]) applies cleanly to 1.9.0.x branch, aside from some contextual differences in reftest.list and crashtests.list. I'm attaching a 1.9.0.x-specific version here, with the *.list conflicts resolved. (I'll request approval1.9.0.x? after I've verified that this fixes the bug on 1.9.0.x.)

Comment on attachment 360094 [details] [diff] [review] fix, as applied to 1.9.0.x branch I just confirmed that this patch fixes the bug on 1.9.0.x. (I tried 'visual test' as well as a number of the crashing testcases, and they're broken before-patching but fixed after.) Requesting approval for landing it there.

Thanks Daniel!

Comment on attachment 360094 [details] [diff] [review] fix, as applied to 1.9.0.x branch Approved for 1.9.0.7, a=dveditz for release-drivers.

Committed to CVS trunk. I left out the crashtests, per comment 23. Checking in layout/base/nsCSSFrameConstructor.cpp; /cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v <-- nsCSSFrameConstructor.cpp new revision: 1.1477; previous revision: 1.1476 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/471594-1-ref.html,v done Checking in layout/reftests/bugs/471594-1-ref.html; /cvsroot/mozilla/layout/reftests/bugs/471594-1-ref.html,v <-- 471594-1-ref.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/layout/reftests/bugs/471594-1.xhtml,v done Checking in layout/reftests/bugs/471594-1.xhtml; /cvsroot/mozilla/layout/reftests/bugs/471594-1.xhtml,v <-- 471594-1.xhtml initial revision: 1.1 done Checking in layout/reftests/bugs/reftest.list; /cvsroot/mozilla/layout/reftests/bugs/reftest.list,v <-- reftest.list new revision: 1.469; previous revision: 1.468 done

Tomcat, another debug based testcase for verification for 1.9.0.7. This seems to be the day for them.

(In reply to comment #32) > Tomcat, another debug based testcase for verification for 1.9.0.7. This seems > to be the day for them. done :) verified 1.9.0.7 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021407 Firefox/3.0.7pre (DEBUG BUILD) - no assertion on the testcases

do the crashtests still need to be committed?

No, afaict. This bug has the in-testsuite+ flag, so the crashtests are committed.

I interpreted comment 23 as meaning crashtests still need to be added.

That's correct. The crashtests still need adding, I think.

roc: are you going to commit the crashtests?

commited the test: http://hg.mozilla.org/mozilla-central/rev/95d2b19be8f7

verified FIXED (no crashes or assertions) on debug builds: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre ID:20090608122057 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre ID:20090608122028