Jesse Ruderman Reporter
	Description • 12 years ago

###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file nsTextFrame.h, line 420

###!!! ASSERTION: bad index: 'uint32_t(aIndex) < mState.mLength', file nsTextFragment.h, line 161

Crash [@ nsTextFrame::HasTerminalNewline()]

Oddly enough, Breakpad debug-opt shows this as a -1 deref (dangerous), while ASan opt shows it as a 0 deref (safe).

	Andrew McCreight [:mccr8]
	Comment 2 • 12 years ago

Clicking on "details" apparently makes it load the test case, so here's a crash report from a recent Nightly:
https://crash-stats.mozilla.com/report/index/bp-888c2390-7078-49e1-9ed8-eea912121226

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 3 • 12 years ago

CC+ Mats for triage/assignment.

	Daniel Veditz [:dveditz]
	Comment 4 • 12 years ago

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Clicking on "details" apparently makes it load the test case,

BugzillaTweaks is a great add-on; one feature puts a "source" link next to attachments so you can view the source before blindly loading testcases.

	Daniel Veditz [:dveditz]
	Comment 5 • 12 years ago

The assertions sound bad, but until we get a worse crash can only call this sec-moderate.

	Mats Palmgren (inactive) Assignee
	Comment 6 • 12 years ago

nsCSSFrameConstructor::CharacterDataChanged calls RemoveLetterFrames
which inserts a new text frame.  The problem is that when inserting
into a nsBlockFrame it peeks at the prev-sibling in a way that assumes
that text frame content offsets are correct.  In CharacterDataChanged
they are wrong of course, that's what it's supposed to correct.

	Mats Palmgren (inactive) Assignee
	Comment 7 • 12 years ago

This is a bit wallpaper-ish, but perhaps good enough for this edge case.
I'm just using NS_FRAME_IS_DIRTY to tell ShouldPutNextSiblingOnNewLine
to avoid looking at text frames.  I think it's purely for performance
reasons we do this in the first place - we'll reflow this block anyway
so layout should be correct afterwards.

3.768 <bzbarsky@mit.edu> 2006-04-20 12:00
Treat terminal newlines in preformatted text like we treat <br> when inserting
frames into a block. Bug 310087, r+sr=roc

	Mats Palmgren (inactive) Assignee
	Comment 8 • 12 years ago

An alternative:

+  nsTextFrame* prevText = do_QueryFrame(prevSibling);
+  if (prevText) {
+    prevText = static_cast<nsTextFrame*>(prevText->GetFirstInFlow());
+    prevText->SetLength(prevText->GetContent()->GetText()->GetLength(), nullptr);
+  }

This is a brutal way of fixing up the content offsets on the prev-sibling.

	Mats Palmgren (inactive) Assignee
	Comment 9 • 12 years ago

AFAICT, this bug is completely benign.  We'll either SEGV crash, or read a random
value that only influences taking a slightly more performant code path.

	Mats Palmgren (inactive) Assignee
	Comment 10 • 12 years ago

I think this is an acceptable wallpaper for now.  I don't see any other
fixes other than rewriting all that nasty first-letter frame ctor code.

https://tbpl.mozilla.org/?tree=Try&rev=f132761991b9

I also tested an Opt build on Linux64 with the perf tests in bug 310087 -
there were no difference afaict.

	Mats Palmgren (inactive) Assignee
	Comment 11 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/016923079831

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/016923079831

Not sure if I should be resolving this or if it should be left open for a better fix. Also, should this have a test?

	Mats Palmgren (inactive) Assignee
	Comment 13 • 10 years ago

Landed a crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a136c4ba2bb5

BTW, the wallpaper was later improved in:
http://hg.mozilla.org/mozilla-central/rev/b732738b75f8

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 14 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a136c4ba2bb5