Last Comment Bug 718290 - Crash [@ ClearAllTextRunReferences] with font inflation and 'white-space' that preserves newlines
: Crash [@ ClearAllTextRunReferences] with font inflation and 'white-space' tha...
Status: VERIFIED FIXED
[sg:critical][advisory-tracking+] rea...
: crash, sec-critical, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla16
Assigned To: David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
:
Mentors:
Depends on:
Blocks: stirdom randomstyles framedest font-inflation
  Show dependency treegraph
 
Reported: 2012-01-15 07:12 PST by Jesse Ruderman
Modified: 2013-12-10 10:00 PST (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
wontfix
+
wontfix
+
verified
+
verified
+
verified
unaffected
.N+


Attachments
testcase (412 bytes, text/html)
2012-01-15 07:12 PST, Jesse Ruderman
no flags Details
stack trace+ (3.93 KB, text/plain)
2012-01-15 07:12 PST, Jesse Ruderman
no flags Details
patch (7.28 KB, patch)
2012-06-22 14:48 PDT, David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch)
roc: review+
akeybl: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-01-15 07:12:28 PST
Created attachment 588736 [details]
testcase

1. Create a new profile.
2. Set user_pref("font.size.inflation.minTwips", 3000);
3. Load the testcase in a debug build of Firefox.
4. Quit Firefox.

Result: Crash [@ ClearAllTextRunReferences] during shutdown.

I think it crashes while trying to evaluate an assertion condition, which might explain why it's debug-only.
Comment 1 Jesse Ruderman 2012-01-15 07:12:57 PST
Created attachment 588737 [details]
stack trace+
Comment 2 Jesse Ruderman 2012-01-15 07:16:27 PST
This was reduced from a testcase that, instead of crashing, triggers:

###!!! ASSERTION: Some pres arena objects were not freed: 'mPresArenaAllocCount == 0', file layout/base/nsPresShell.cpp, line 973
Comment 3 Daniel Veditz [:dveditz] 2012-01-18 17:15:37 PST
Presumably this problem could happen at times other than shutdown if the victim waited long enough that these things got cleaned up?
Comment 4 Daniel Veditz [:dveditz] 2012-01-25 17:46:45 PST
I'm leaning toward calling this a DoS? null deref during shutdown?
Comment 5 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-01-25 18:17:46 PST
Why do you think it's a guaranteed null deref?
Comment 6 Jesse Ruderman 2012-01-27 14:32:07 PST
I made the bug security-sensitive because of comment 2, fwiw.  That assertion often indicates that there could be dangling pointers into the pres arena.
Comment 7 Daniel Veditz [:dveditz] 2012-02-15 17:03:34 PST
This was filed against Fx12, and Fx10 doesn't have font inflation. Fx11 is probably affected but needs to be tested.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-23 13:19:22 PST
QA, can we get help with checking whether Fx11 is affected here? Thanks!
Comment 9 Al Billings [:abillings] 2012-02-24 16:39:07 PST
Using the beta4 debug build on OS X 10.7.3, I got bp-b25c3d6f-9244-4855-9266-76b9c2120225 when I ran the testcase from comment 0.
Comment 10 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-02-28 17:04:37 PST
But we're not shipping font inflation turned on for any Fx11 builds, right?
Comment 11 Daniel Veditz [:dveditz] 2012-03-01 13:37:27 PST
Dunno, looks turned on in mobile beta (fx11) to me:

https://mxr.mozilla.org/mozilla-beta/source/mobile/android/app/mobile.js#421

Or should I be looking somewhere else?
Comment 12 Daniel Veditz [:dveditz] 2012-03-01 13:40:27 PST
OK, Firefox Beta on my phone has 0.
Comment 13 Daniel Veditz [:dveditz] 2012-03-06 17:59:12 PST
lowering to sg:moderate on the assumption most users won't change their minTwips pref to a ridiculous value. Can content CSS achieve the equivalent of inflation through a combination of font properties? That would change the calculus.
Comment 14 Jesse Ruderman 2012-03-09 06:29:26 PST
IIRC, I discovered this bug using a less ridiculous font-inflation setting, so it could affect the default settings of mobile Firefox.
Comment 15 Kevin Brosnan [:kbrosnan] 2012-05-03 10:44:59 PDT
Not sure if this should block fennec-1.0. It is a sg:crit but it seems to be a footgun case.
Comment 16 JP Rosevear [:jpr] 2012-05-18 12:29:19 PDT
Should we be fixing this still for first release?
Comment 17 David Bolter [:davidb] 2012-05-18 13:14:47 PDT
Aaron, can you confirm default settings for font inflation on fennec beta?
Comment 18 Aaron Train [:aaronmt] 2012-05-18 13:16:35 PDT
(In reply to David Bolter [:davidb] from comment #17)
> Aaron, can you confirm default settings for font inflation on fennec beta?

font.size.inflation.minTwips: 120
Comment 19 Aaron Train [:aaronmt] 2012-05-18 13:20:23 PDT
Fennec's UI settings:

Tiny: font.size.inflation.minTwips: 0
Small: font.size.inflation.minTwips: 80
Medium: font.size.inflation.minTwips: 120 -- default
Large: font.size.inflation.minTwips: 160
Extra Large: font.size.inflation.minTwips: 240
Comment 20 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-23 13:44:11 PDT
Valgrind isn't particularly helpful.  With DEBUG_TRACEMALLOC_PRESARENA, I see:

==25555== Invalid read of size 8
==25555==    at 0x6EC9020: ClearAllTextRunReferences(nsTextFrame*, gfxTextRun*, nsTextFrame*) (nsIFrame.h:1354)
==25555==    by 0x6EC915C: _ZL23UnhookTextRunFromFramesP10gfxTextRunP11nsTextFrame.part.204 (nsTextFrameThebes.cpp:451)
==25555==    by 0x6ECD2A2: FrameTextRunCache::NotifyExpired(gfxTextRun*) (gfxFont.h:2624)
==25555==    by 0x6ED2932: nsExpirationTracker<gfxTextRun, 3u>::AgeOneGeneration() (nsExpirationTracker.h:188)
==25555==    by 0x6ED2A6E: nsExpirationTracker<gfxTextRun, 3u>::TimerCallback(nsITimer*, void*) (nsExpirationTracker.h:311)
==25555==    by 0x844C907: nsTimerImpl::Fire() (nsTimerImpl.cpp:473)
==25555==    by 0x844CDB4: nsTimerEvent::Run() (nsTimerImpl.cpp:556)
==25555==    by 0x844407F: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:624)
==25555==    by 0x83C770C: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:213)
==25555==    by 0x8259B7A: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:113)
==25555==    by 0x8494742: MessageLoop::Run() (message_loop.cc:208)
==25555==    by 0x80AE971: nsBaseAppShell::Run() (nsBaseAppShell.cpp:163)
==25555==    by 0x7D8F802: nsAppStartup::Run() (nsAppStartup.cpp:256)
==25555==    by 0x6A1E569: XREMain::XRE_mainRun() (nsAppRunner.cpp:3768)
==25555==    by 0x6A253CB: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:3845)
==25555==    by 0x6A2576A: XRE_main (nsAppRunner.cpp:3921)
==25555==    by 0x4023EF: main (nsBrowserApp.cpp:157)
==25555==  Address 0x268c4400 is not stack'd, malloc'd or (recently) free'd

(twice), followed by:

pure virtual method called
terminate called without an active exception
Comment 21 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-05-23 16:02:29 PDT
But with a larger --freelist-vol, I get:

==28053== Invalid read of size 8
==28053==    at 0x6EC9020: ClearAllTextRunReferences(nsTextFrame*, gfxTextRun*, nsTextFrame*) (nsIFrame.h:1354)
==28053==    by 0x6EC915C: _ZL23UnhookTextRunFromFramesP10gfxTextRunP11nsTextFrame.part.204 (nsTextFrameThebes.cpp:451)
==28053==    by 0x6ECD2A2: FrameTextRunCache::NotifyExpired(gfxTextRun*) (gfxFont.h:2624)
==28053==    by 0x6ED2932: nsExpirationTracker<gfxTextRun, 3u>::AgeOneGeneration() (nsExpirationTracker.h:188)
==28053==    by 0x6ED2A6E: nsExpirationTracker<gfxTextRun, 3u>::TimerCallback(nsITimer*, void*) (nsExpirationTracker.h:311)
==28053==    by 0x844C907: nsTimerImpl::Fire() (nsTimerImpl.cpp:473)
==28053==    by 0x844CDB4: nsTimerEvent::Run() (nsTimerImpl.cpp:556)
==28053==    by 0x844407F: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:624)
==28053==    by 0x83C770C: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:213)
==28053==    by 0x8259B7A: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:113)
==28053==    by 0x8494742: MessageLoop::Run() (message_loop.cc:208)
==28053==    by 0x80AE971: nsBaseAppShell::Run() (nsBaseAppShell.cpp:163)
==28053==    by 0x7D8F802: nsAppStartup::Run() (nsAppStartup.cpp:256)
==28053==    by 0x6A1E569: XREMain::XRE_mainRun() (nsAppRunner.cpp:3768)
==28053==    by 0x6A253CB: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:3845)
==28053==    by 0x6A2576A: XRE_main (nsAppRunner.cpp:3921)
==28053==    by 0x4023EF: main (nsBrowserApp.cpp:157)
==28053==  Address 0x2fb8b380 is 64 bytes inside a block of size 120 free'd
==28053==    at 0x4C282E0: free (vg_replace_malloc.c:366)
==28053==    by 0x8482FC3: free (nsTraceMalloc.c:1289)
==28053==    by 0x6E3036B: nsContainerFrame::DeleteNextInFlowChild(nsPresContext*, nsIFrame*, bool) (nsIFrame.h:564)
==28053==    by 0x6E9A09E: nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) (nsLineLayout.cpp:965)
==28053==    by 0x6E9055E: nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) (nsInlineFrame.cpp:680)
==28053==    by 0x6E909A9: nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) (nsInlineFrame.cpp:543)
==28053==    by 0x6E91D4C: nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsInlineFrame.cpp:397)
==28053==    by 0x6E99711: nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) (nsLineLayout.cpp:825)
==28053==    by 0x6E0B75D: _ZN12nsBlockFrame17ReflowInlineFrameER18nsBlockReflowStateR12nsLineLayout19nsLineList_iteratorP8nsIFrameP16LineReflowStatus.constprop.192 (nsBlockFrame.cpp:3884)
==28053==    by 0x6E17836: nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) (nsBlockFrame.cpp:3680)
==28053==    by 0x6E187C3: nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) (nsBlockFrame.cpp:2550)
==28053==    by 0x6E1A949: nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) (nsBlockFrame.cpp:2000)
==28053==    by 0x6E1F162: nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsBlockFrame.cpp:1043)
==28053==    by 0x6E212A3: nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) (nsBlockReflowContext.cpp:262)
==28053==    by 0x6E0F486: nsBlockFrame::ReflowFloat(nsBlockReflowState&, nsRect const&, nsIFrame*, nsMargin&, bool, unsigned int&) (nsBlockFrame.cpp:5966)
==28053==    by 0x6E23C15: nsBlockReflowState::FlowAndPlaceFloat(nsIFrame*) (nsBlockReflowState.cpp:757)
==28053==    by 0x6E24318: nsBlockReflowState::AddFloat(nsLineLayout*, nsIFrame*, int) (nsBlockReflowState.cpp:509)
==28053==    by 0x6E9A6D0: nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) (nsLineLayout.h:195)
==28053==    by 0x6E0B75D: _ZN12nsBlockFrame17ReflowInlineFrameER18nsBlockReflowStateR12nsLineLayout19nsLineList_iteratorP8nsIFrameP16LineReflowStatus.constprop.192 (nsBlockFrame.cpp:3884)
==28053==    by 0x6E17836: nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) (nsBlockFrame.cpp:3680)
==28053==    by 0x6E187C3: nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) (nsBlockFrame.cpp:2550)
==28053==    by 0x6E1A949: nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) (nsBlockFrame.cpp:2000)
==28053==    by 0x6E1F162: nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsBlockFrame.cpp:1043)
==28053==    by 0x6E30027: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) (nsContainerFrame.cpp:907)
==28053==    by 0x6E79327: nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsCanvasFrame.cpp:426)
==28053==    by 0x6E30027: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) (nsContainerFrame.cpp:907)
==28053==    by 0x6E68B19: nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) (nsGfxScrollFrame.cpp:518)
==28053==    by 0x6E6BCE1: nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) (nsGfxScrollFrame.cpp:616)
==28053==    by 0x6E6E313: nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsGfxScrollFrame.cpp:857)
==28053==    by 0x6E30027: nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) (nsContainerFrame.cpp:907)
==28053==    by 0x6EEA7C3: ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsViewportFrame.cpp:201)
==28053==    by 0x6DB6D4F: PresShell::DoReflow(nsIFrame*, bool) (nsPresShell.cpp:7361)
==28053==    by 0x6DBB5F5: PresShell::ProcessReflowCommands(bool) (nsPresShell.cpp:7508)
==28053==    by 0x6DBBD59: PresShell::FlushPendingNotifications(mozFlushType) (nsPresShell.cpp:3839)
==28053==    by 0x6DC9EFC: _ZN15nsRefreshDriver6NotifyEP8nsITimer.part.60 (nsRefreshDriver.cpp:395)
==28053==    by 0x6DCA4C7: nsRefreshDriver::Notify(nsITimer*) (nsRefreshDriver.cpp:306)
==28053==    by 0x844C536: nsTimerImpl::Fire() (nsTimerImpl.cpp:476)
==28053==    by 0x844CDB4: nsTimerEvent::Run() (nsTimerImpl.cpp:556)
==28053==    by 0x844407F: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:624)
==28053==    by 0x83C770C: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:213)
==28053==    by 0x8445CED: nsThread::Shutdown() (nsThread.cpp:466)
==28053==    by 0x6A311BD: nsRunnableMethodImpl<void (nsPACMan::*)(), true>::Run() (nsThreadUtils.h:313)
==28053==    by 0x844407F: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:624)
==28053==    by 0x83C770C: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:213)
==28053==    by 0x8259B0A: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (MessagePump.cpp:82)
==28053==    by 0x8494742: MessageLoop::Run(==28053==    by 0x80AE971: nsBaseAppShell::Run() (nsBaseAppShell.cpp:163)
==28053==    by 0x7D8F802: nsAppStartup::Run() (nsAppStartup.cpp:256)
==28053==    by 0x6A1E569: XREMain::XRE_mainRun() (nsAppRunner.cpp:3768)
==28053==    by 0x6A253CB: XREMain::XRE_main(int, char**, nsXREAppData const*) (nsAppRunner.cpp:3845)
) (message_loop.cc:208)
Comment 22 JP Rosevear [:jpr] 2012-05-30 05:31:59 PDT
Based on current analysis, does this crash impact us when using default settings?  If so, we should keep it as a release blocker, if not I think we track it for 15.
Comment 23 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-01 16:49:09 PDT
One thing I'm suspicious of is that TEXT_IN_TEXTRUN_USER_DATA probably needs to be tracked separately for the inflated and non-inflated text runs.
Comment 24 Allison Naaktgeboren :ally 2012-06-04 10:15:04 PDT
dbaron, have you investigated or vetted that suspicion? I for one, am interested in knowing if there is more than one issue in play.

JP, did you ever get an answer for comment 22, and if not, who should? It is not clear to me who owns that analysis.
Comment 25 Mats Palmgren (vacation) 2012-06-04 10:24:32 PDT
(In reply to JP Rosevear [:jpr] from comment #22)
> Based on current analysis, does this crash impact us when using default
> settings?

IIUC, it only affects products where font-inflation is enabled by default.
So, desktop Firefox: no.  Fennec: yes.
Comment 26 Kevin Brosnan [:kbrosnan] 2012-06-04 10:36:43 PDT
That was understood. The question is what values of font.size.inflation.minTwips causes the issue? Are any of the default values we ship with a concern?

Tiny:          0
Small:        80
Normal:      120
Large:       160
Extra large: 240
Comment 27 Mats Palmgren (vacation) 2012-06-04 10:56:01 PDT
If emPerLine and/or minTwips is non-zero then font-inflation is enabled.
Comment 28 Alex Keybl [:akeybl] 2012-06-04 13:12:25 PDT
We considered blocking Fennec 1.0 on this bug given it has font inflation enabled by default. We decided to instead continue tracking for FF14. This is a very desirable bug to resolve, however, and we would definitely consider taking this before FN14's 1.0 release.
Comment 29 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-05 19:29:46 PDT
(In reply to David Baron [:dbaron] from comment #23)
> One thing I'm suspicious of is that TEXT_IN_TEXTRUN_USER_DATA probably needs
> to be tracked separately for the inflated and non-inflated text runs.

So removing the line:
  aFrame->RemoveStateBits(TEXT_IN_TEXTRUN_USER_DATA);
from ClearAllTextRunReferences doesn't make the crash go away, so maybe this isn't the problem.
Comment 30 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-21 14:37:07 PDT
Reproducing this bug with this testcase now requires commenting out the lines quoted in bug 764354 comment 12.
Comment 31 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-21 15:19:03 PDT
(In reply to Jesse Ruderman from comment #2)
> This was reduced from a testcase that, instead of crashing, triggers:
> 
> ###!!! ASSERTION: Some pres arena objects were not freed:
> 'mPresArenaAllocCount == 0', file layout/base/nsPresShell.cpp, line 973

I think I've figured out why this bit is important:  it looks like the problem is that the crash is because a text run has a dangling pointer to a text frame.  That text frame has its memory full of 0xdadadada, but it's never had its DestroyFrom method called.  So I think the "not freed" is the underlying cause of the dangling pointer.
Comment 32 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-21 16:27:46 PDT
(In reply to David Baron [:dbaron] from comment #31)
> (In reply to Jesse Ruderman from comment #2)
> > This was reduced from a testcase that, instead of crashing, triggers:
> > 
> > ###!!! ASSERTION: Some pres arena objects were not freed:
> > 'mPresArenaAllocCount == 0', file layout/base/nsPresShell.cpp, line 973
> 
> I think I've figured out why this bit is important:  it looks like the
> problem is that the crash is because a text run has a dangling pointer to a
> text frame.  That text frame has its memory full of 0xdadadada, but it's
> never had its DestroyFrom method called.  So I think the "not freed" is the
> underlying cause of the dangling pointer.

Er, never mind.  I just wasn't looking at nsContinuingTextFrame::DestroyFrom.

That said, I think this may be related to there being another case in which nsContinuingTextFrame::DestroyFrom needs to clear text runs.
Comment 33 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-21 16:53:22 PDT
So, more details:

the problem in this case is the *uninflated* text run has a dangling back-pointer to the frame.

The frame in question is the third of three continuations for the piece of text "aaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbb\nc".  However, it's destroyed during reflow, whereas the others stick around.

When the continuations are initially set up, this third continuation has a different *uninflated* text run from the previous two continuations, but the same *inflated* text run as the previous two continuations, thanks to:
http://hg.mozilla.org/mozilla-central/file/da85d45f39ef/layout/generic/nsTextFrameThebes.cpp#l1458
  1458   nsStyleContext* sc1 = aFrame1->GetStyleContext();
  1459   const nsStyleText* textStyle1 = sc1->GetStyleText();
  1460   // If the first frame ends in a preformatted newline, then we end the textrun
  1461   // here. This avoids creating giant textruns for an entire plain text file.
  1462   // Note that we create a single text frame for a preformatted text node,
  1463   // even if it has newlines in it, so typically we won't see trailing newlines
  1464   // until after reflow has broken up the frame into one (or more) frames per
  1465   // line. That's OK though.
  1466   if (textStyle1->NewlineIsSignificant() && HasTerminalNewline(aFrame1))
  1467     return false;
Disabling this optimization makes the crash go away.

I think the conditions in nsContinuingTextFrame::Destroy for whether we clear text runs need to account for this optimization better.
Comment 34 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-21 17:00:15 PDT
Then again, given that the problem is that the textrun has a reference to the text frame, I'd have thought that TEXT_IN_TEXTRUN_USER_DATA would be being set, and it's not.
Comment 35 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-21 17:01:31 PDT
Though it's not clear to me whether TEXT_IN_TEXTRUN_USER_DATA is supposed to be set for *all* cases when the textrun has a pointer to a frame, or just some of them.
Comment 36 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-21 17:10:14 PDT
And, to be clear, the pointer from the text run to the frame is the one we dereference here:

http://hg.mozilla.org/mozilla-central/file/da85d45f39ef/layout/generic/nsTextFrameThebes.cpp#l435
   435   if (aTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_SIMPLE_FLOW) {
   436     nsIFrame* userDataFrame = static_cast<nsIFrame*>(aTextRun->GetUserData());
   437     DebugOnly<bool> found =
   438       ClearAllTextRunReferences(static_cast<nsTextFrame*>(userDataFrame),
   439                                 aTextRun, aStartContinuation);
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-21 19:39:56 PDT
(In reply to David Baron [:dbaron] from comment #34)
> Then again, given that the problem is that the textrun has a reference to
> the text frame, I'd have thought that TEXT_IN_TEXTRUN_USER_DATA would be
> being set, and it's not.

Yes, it should have been set.
Comment 38 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-22 13:56:02 PDT
(In reply to David Baron [:dbaron] from comment #29)
> (In reply to David Baron [:dbaron] from comment #23)
> > One thing I'm suspicious of is that TEXT_IN_TEXTRUN_USER_DATA probably needs
> > to be tracked separately for the inflated and non-inflated text runs.
> 
> So removing the line:
>   aFrame->RemoveStateBits(TEXT_IN_TEXTRUN_USER_DATA);
> from ClearAllTextRunReferences doesn't make the crash go away, so maybe this
> isn't the problem.

Actually, maybe removing that line isn't a sufficient experiment to determine if this is the problem, because of this code in UnhookTextRunFromFrames:

  if (aTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_SIMPLE_FLOW) {
    nsIFrame* userDataFrame = static_cast<nsIFrame*>(aTextRun->GetUserData());
    DebugOnly<bool> found =
      ClearAllTextRunReferences(static_cast<nsTextFrame*>(userDataFrame),
                                aTextRun, aStartContinuation);
    NS_ASSERTION(!aStartContinuation || found,
                 "aStartContinuation wasn't found in simple flow text run");
    if (!(userDataFrame->GetStateBits() & TEXT_IN_TEXTRUN_USER_DATA)) {
      aTextRun->SetUserData(nsnull);
    }
  } else {

which seems to assume that ClearAllTextRunReferences does that removal.  If I also comment out this test:

    //if (!(userDataFrame->GetStateBits() & TEXT_IN_TEXTRUN_USER_DATA)) {
      aTextRun->SetUserData(nsnull);
    //}

then that does fix the crash.  So maybe the need to split this bit is the problem.
Comment 39 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-22 14:48:46 PDT
Created attachment 635926 [details] [diff] [review]
patch
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-22 16:49:47 PDT
Comment on attachment 635926 [details] [diff] [review]
patch

Review of attachment 635926 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrameThebes.cpp
@@ +462,5 @@
>        nsTextFrame* userDataFrame = userData->mMappedFlows[i].mStartFrame;
> +      nsFrameState whichTextRunState =
> +        userDataFrame->GetTextRun(nsTextFrame::eInflated) == aTextRun
> +          ? TEXT_IN_TEXTRUN_USER_DATA
> +          : TEXT_IN_UNINFLATED_TEXTRUN_USER_DATA;

Hoist this out of the if so you don't have to duplicate it.
Comment 41 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-22 18:05:10 PDT
The frame variables are inside the if, though, and in the second half inside the loop.  (And there may be cases where the result could actually vary across frames.)
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-24 16:03:29 PDT
Right. Never mind then.
Comment 43 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-24 16:27:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9f0911976b
Comment 44 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-24 20:13:51 PDT
https://hg.mozilla.org/mozilla-central/rev/dd9f0911976b
Comment 45 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-25 13:31:43 PDT
Comment on attachment 635926 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 627842
User impact if declined: potential for crashes or exploits
Testing completed (on m-c, etc.): on mozilla-central; tested that it fixes testcase
Risk to taking this patch (and alternatives if risky): I think it's relatively low risk; it tracks one bit of information separately for the inflated and non-inflated text runs so we don't get confused.  The biggest risk is probably that something could go wrong in the code to decide which bit to use.
String or UUID changes made by this patch: none
Comment 46 Alex Keybl [:akeybl] 2012-06-26 09:35:59 PDT
Comment on attachment 635926 [details] [diff] [review]
patch

[Triage Comment]
Please land on aurora 15 in preparation for beta 14 consideration. Thanks!
Comment 47 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-26 11:48:24 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/c3a418c44e8c
Comment 48 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-27 09:51:53 PDT
Comment on attachment 635926 [details] [diff] [review]
patch

Looks good on Aurora, go ahead with landing to Beta if possible in the next hour so that it stands a chance of being in the beta go to build later today.
Comment 49 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-06-27 10:00:45 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/4e4dd48e0b31
Comment 50 Vlad [QA] 2012-07-23 04:43:20 PDT
Verified on:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:15.0) Gecko/20120720 Firefox/15.0 beta debug build
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120713 Firefox/14.0.1 release debug build

Following the steps from the description and using the testcase attached I didn't manage to crash Firefox thus marking this as verified. 

I've verified this on Firefox 13 debug build with the same steps and it crashes every time.
Comment 51 Paul Silaghi, QA [:pauly] 2012-09-17 08:46:08 PDT
Verified fixed on FF 16 2012-09-17-mozilla-beta-debug build on Mac OS X 10.6.8

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