Last Comment Bug 471594 - "ASSERTION: negative length" & crash with XBL, rtl
: "ASSERTION: negative length" & crash with XBL, rtl
Status: VERIFIED FIXED
: [sg:critical] post 1.8-branch
: assertion, crash, regression, testcase, verified1.9.0.7, verified1.9.1
Product: Core
Classification: Components
Component: Layout: Text
: Trunk
: All All
: P2 normal (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (Mozilla Corporation)
: layout.fonts-and-text
:
:
: 348483 385270 397961
  Show dependency treegraph
 
Reported: 2008-12-30 14:41 PST by Jesse Ruderman
Modified: 2009-06-09 15:23 PDT (History)
13 users (show)
dbaron: blocking1.9.1+
dveditz: blocking1.9.0.7+
dveditz: wanted1.9.0.x+
samuel.sidler+old: wanted1.8.1.x-
roc: in‑testsuite+
See Also:
Crash Signature:


Attachments
testcase (552 bytes, application/xhtml+xml)
2008-12-30 14:41 PST, Jesse Ruderman
no flags Details
testcase 2 (crashes Firefox when loaded) (607 bytes, application/xhtml+xml)
2009-01-02 14:50 PST, Daniel Holbert [:dholbert]
no flags Details
backtrace of testcase 2 crash, on linux (3.07 KB, text/plain)
2009-01-02 14:57 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 3 (crashes Firefox when loaded) (607 bytes, application/xhtml+xml)
2009-01-02 15:09 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 4 (crashes Firefox when loaded) (607 bytes, application/xhtml+xml)
2009-01-02 16:30 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 5 (crashes onclick instead of onload) (649 bytes, application/xhtml+xml)
2009-01-02 17:48 PST, Daniel Holbert [:dholbert]
no flags Details
backtrace just before testcase 5 crash, on linux (13.21 KB, text/plain)
2009-01-02 18:40 PST, Daniel Holbert [:dholbert]
no flags Details
testcase 6 (580 bytes, application/xml)
2009-01-15 16:03 PST, Robert O'Callahan (:roc) (Mozilla Corporation)
no flags Details
visual test (527 bytes, application/xml)
2009-01-15 16:25 PST, Robert O'Callahan (:roc) (Mozilla Corporation)
no flags Details
fix (3.84 KB, patch)
2009-01-15 16:32 PST, Robert O'Callahan (:roc) (Mozilla Corporation)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
fix, as applied to 1.9.0.x branch (3.69 KB, patch)
2009-02-02 08:40 PST, Daniel Holbert [:dholbert]
dveditz: approval1.9.0.7+
Details | Diff | Splinter Review

Summon comment box

Description Jesse Ruderman 2008-12-30 14:41:38 PST
Created attachment 354882 [details]
testcase

###!!! 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?
Comment 1 Daniel Holbert [:dholbert] 2009-01-02 14:31:35 PST
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
Comment 2 Daniel Holbert [:dholbert] 2009-01-02 14:49:04 PST
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
Comment 3 Daniel Holbert [:dholbert] 2009-01-02 14:50:52 PST
Created attachment 355162 [details]
testcase 2 (crashes Firefox when loaded)

(oops, here's the testcase)
Comment 4 Daniel Holbert [:dholbert] 2009-01-02 14:57:28 PST
Created attachment 355165 [details]
backtrace of testcase 2 crash, on linux

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...
Comment 5 Daniel Holbert [:dholbert] 2009-01-02 15:09:07 PST
Created attachment 355166 [details]
testcase 3 (crashes Firefox when loaded)

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.
Comment 6 Daniel Holbert [:dholbert] 2009-01-02 16:30:56 PST
Created attachment 355181 [details]
testcase 4 (crashes Firefox when loaded)

(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.
Comment 7 Daniel Holbert [:dholbert] 2009-01-02 17:16:51 PST
(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
Comment 8 Daniel Holbert [:dholbert] 2009-01-02 17:48:48 PST
Created attachment 355187 [details]
testcase 5 (crashes onclick instead of onload)
Comment 9 Daniel Holbert [:dholbert] 2009-01-02 18:40:31 PST
Created attachment 355193 [details]
backtrace just before testcase 5 crash, on linux

(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.
Comment 10 Daniel Holbert [:dholbert] 2009-01-02 23:57:42 PST
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
Comment 11 Jeff Walden (remove +bmo to email) 2009-01-03 06:46:21 PST
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).
Comment 12 Daniel Holbert [:dholbert] 2009-01-03 17:20:38 PST
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?
Comment 13 Daniel Holbert [:dholbert] 2009-01-03 20:42:31 PST
(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
Comment 14 Daniel Holbert [:dholbert] 2009-01-05 17:53:58 PST
(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.)
Comment 15 Samuel Sidler (old account; do not CC) 2009-01-08 10:20:52 PST
Not wanted on 1.8.1 given the regression range and bugs involved.
Comment 16 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-15 15:42:08 PST
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.
Comment 17 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-15 16:03:38 PST
Created attachment 357256 [details]
testcase 6

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".
Comment 18 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-15 16:04:58 PST
OK, so GetInsertionPoint is returning the wrong frame. It's returning the first continuation of the span (0x2178ff50 above), not the last continuation.
Comment 19 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-15 16:14:19 PST
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.
Comment 20 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-15 16:25:32 PST
Created attachment 357261 [details]
visual test

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 21 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-15 16:32:27 PST
Created attachment 357266 [details] [review]
fix
Comment 22 Boris Zbarsky (:bz) 2009-01-15 18:12:12 PST
Comment on attachment 357266 [details] [review]
fix

Looks good.
Comment 23 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-01-17 01:36:28 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/7af1fe6b7ab4

I pushed the reftest. The crashtests should be added when this bug gets decloaked.
Comment 24 Samuel Sidler (old account; do not CC) 2009-01-26 12:12:08 PST
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?
Comment 25 Daniel Holbert [:dholbert] 2009-01-26 17:36:45 PST
Patch pushed to 1.9.1 on roc's behalf:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3b8c370b2be9
Comment 26 Samuel Sidler (old account; do not CC) 2009-02-02 07:55:02 PST
roc: Can you work up a 1.9.0 patch for this blocking bug?
Comment 27 Daniel Holbert [:dholbert] 2009-02-02 08:40:09 PST
Created attachment 360094 [details] [review]
fix, as applied to 1.9.0.x branch

This bug's patch (attachment 357266 [details] [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 28 Daniel Holbert [:dholbert] 2009-02-02 09:07:55 PST
Comment on attachment 360094 [details] [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.
Comment 29 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-02-02 13:35:51 PST
Thanks Daniel!
Comment 30 Daniel Veditz 2009-02-02 14:22:26 PST
Comment on attachment 360094 [details] [review]
fix, as applied to 1.9.0.x branch

Approved for 1.9.0.7, a=dveditz for release-drivers.
Comment 31 Daniel Holbert [:dholbert] 2009-02-02 14:36:53 PST
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
Comment 32 Al Billings [:abillings] 2009-02-13 15:30:42 PST
Tomcat, another debug based testcase for verification for 1.9.0.7. This seems to be the day for them.
Comment 33 Carsten Book [:Tomcat] 2009-02-14 08:44:51 PST
(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
Comment 34 Marc Bejarano 2009-03-05 22:36:35 PST
do the crashtests still need to be committed?
Comment 35 Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2009-03-06 00:49:41 PST
No, afaict. This bug has the in-testsuite+ flag, so the crashtests are committed.
Comment 36 Jesse Ruderman 2009-03-06 14:43:26 PST
I interpreted comment 23 as meaning crashtests still need to be added.
Comment 37 Boris Zbarsky (:bz) 2009-03-06 14:59:09 PST
That's correct.  The crashtests still need adding, I think.
Comment 38 Marc Bejarano 2009-04-09 14:18:57 PDT
roc: are you going to commit the  crashtests?
Comment 39 Robert O'Callahan (:roc) (Mozilla Corporation) 2009-04-11 02:51:31 PDT
commited the test: http://hg.mozilla.org/mozilla-central/rev/95d2b19be8f7
Comment 40 Aakash Desai [:aakashd] 2009-06-09 15:23:40 PDT
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

Note You need to log in before you can comment on or make changes to this bug.