Closed Bug 822910 Opened 12 years ago Closed 11 years ago

Crash [@ nsTextFrame::HasTerminalNewline()] with splitText, floating :first-letter


(Core :: Layout: Text and Fonts, defect)

Not set



Tracking Status
firefox20 --- affected
firefox21 --- fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix


(Reporter: jruderman, Assigned: MatsPalmgren_bugz)



(5 keywords, Whiteboard: [fuzzblocker][adv-main21+])


(4 files, 1 obsolete file)

Attached file testcase
###!!! 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).
Clicking on "details" apparently makes it load the test case, so here's a crash report from a recent Nightly:
CC+ Mats for triage/assignment.
(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.
The assertions sound bad, but until we get a worse crash can only call this sec-moderate.
Keywords: sec-moderate
Whiteboard: [fuzzblocker]
Assignee: nobody → matspal
OS: Mac OS X → All
Hardware: x86_64 → All
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.
Attached patch wip (obsolete) — Splinter Review
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 <> 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
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.
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.
Attached patch wallpaperSplinter Review
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.

I also tested an Opt build on Linux64 with the perf tests in bug 310087 -
there were no difference afaict.
Attachment #712284 - Attachment is obsolete: true
Attachment #715215 - Flags: review?(roc)

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?
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main21+]
Landed a crashtest:

BTW, the wallpaper was later improved in:
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.