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

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: 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).

Comment 1

[Jesse Ruderman]

Attachment: stacks (NS_StackWalk, breakpad)

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

CC+ Mats for triage/assignment.

Comment 4

[Daniel Veditz [:dveditz]]

(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.

Comment 5

[Daniel Veditz [:dveditz]]

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

Comment 6

[Mats Palmgren (inactive)]

Attachment: frame dumps, stacks, more comments

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.

Comment 7

[Mats Palmgren (inactive)]

Attachment: wip

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 <[e-mail]> 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

Comment 8

[Mats Palmgren (inactive)]

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.

Comment 9

[Mats Palmgren (inactive)]

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.

Comment 10

[Mats Palmgren (inactive)]

Attachment: wallpaper

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.

Comment 11

[Mats Palmgren (inactive)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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