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

RESOLVED FIXED in Firefox 21

Status

()

defect
--
critical
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jruderman, Assigned: mats)

Tracking

(Blocks 2 bugs, 5 keywords)

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox20 affected, firefox21 fixed, firefox-esr17 wontfix, b2g18 wontfix)

Details

(Whiteboard: [fuzzblocker][adv-main21+])

Attachments

(4 attachments, 1 obsolete attachment)

Posted 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:
https://crash-stats.mozilla.com/report/index/bp-888c2390-7078-49e1-9ed8-eea912121226
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.
Posted 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 <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
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.
Posted 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.

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.
Attachment #712284 - Attachment is obsolete: true
Attachment #715215 - Flags: review?(roc)
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?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main21+]
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
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.