718290 - Crash [@ ClearAllTextRunReferences] with font inflation and 'white-space' that preserves newlines

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Attachment: stack trace+

Comment 2

[Jesse Ruderman]

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]]

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]]

I'm leaning toward calling this a DoS? null deref during shutdown?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Why do you think it's a guaranteed null deref?

Comment 6

[Jesse Ruderman]

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]]

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)]

QA, can we get help with checking whether Fx11 is affected here? Thanks!

Comment 9

[Al Billings [:abillings - ex-MoCo]]

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-4, no longer working on Mozilla)]

But we're not shipping font inflation turned on for any Fx11 builds, right?

Comment 11

[Daniel Veditz [:dveditz]]

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]]

OK, Firefox Beta on my phone has 0.

Comment 13

[Daniel Veditz [:dveditz]]

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]

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 [Ex-Mozilla]]

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]]

Should we be fixing this still for first release?

Comment 17

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

Aaron, can you confirm default settings for font inflation on fennec beta?

Comment 18

[Aaron Train [:aaronmt]]

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

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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]]

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-4, no longer working on Mozilla)]

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]

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 (inactive)]

(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 [Ex-Mozilla]]

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 (inactive)]

If emPerLine and/or minTwips is non-zero then font-inflation is enabled.

Comment 28

[Alex Keybl [:akeybl]]

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-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

Reproducing this bug with this testcase now requires commenting out the lines quoted in bug 764354 comment 12.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

(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-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

Attachment: patch

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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-4, no longer working on Mozilla)]

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) (email my personal email if necessary)]

Right. Never mind then.

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 45

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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]]

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-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c3a418c44e8c

Comment 48

[Lukas Blakk [:lsblakk] use ?needinfo]

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-4, no longer working on Mozilla)]

https://hg.mozilla.org/releases/mozilla-beta/rev/4e4dd48e0b31

Comment 50

[Vlad [QA]]

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]]

Verified fixed on FF 16 2012-09-17-mozilla-beta-debug build on Mac OS X 10.6.8