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

VERIFIED FIXED in Firefox 14

Status

()

Core
Layout
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: dbaron)

Tracking

(Blocks: 2 bugs, {crash, sec-critical, testcase})

Trunk
mozilla16
x86_64
Mac OS X
crash, sec-critical, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 unaffected, firefox12+ wontfix, firefox13+ wontfix, firefox14+ verified, firefox15+ verified, firefox16+ verified, firefox-esr10 unaffected, blocking-fennec1.0 .N+)

Details

(Whiteboard: [sg:critical][advisory-tracking+] readability)

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 588737 [details]
stack trace+
(Reporter)

Comment 2

6 years ago
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
(Reporter)

Updated

6 years ago
Group: core-security
(Reporter)

Updated

6 years ago
Blocks: 334514
(Assignee)

Updated

6 years ago
Blocks: 627842
Presumably this problem could happen at times other than shutdown if the victim waited long enough that these things got cleaned up?
Assignee: nobody → dbaron
I'm leaning toward calling this a DoS? null deref during shutdown?
(Assignee)

Comment 5

6 years ago
Why do you think it's a guaranteed null deref?
(Reporter)

Comment 6

6 years ago
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.
This was filed against Fx12, and Fx10 doesn't have font inflation. Fx11 is probably affected but needs to be tested.
status1.9.2: --- → unaffected
status-firefox-esr10: --- → unaffected
status-firefox10: --- → unaffected
status-firefox12: --- → affected
status-firefox13: --- → affected
Whiteboard: [sg:critical]

Updated

5 years ago
tracking-firefox-esr10: --- → -
tracking-firefox10: --- → +
tracking-firefox11: --- → +
tracking-firefox12: --- → +
tracking-firefox13: --- → +
Keywords: qawanted
QA, can we get help with checking whether Fx11 is affected here? Thanks!
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.
Keywords: qawanted
status-firefox11: --- → affected
(Assignee)

Comment 10

5 years ago
But we're not shipping font inflation turned on for any Fx11 builds, right?
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?
OK, Firefox Beta on my phone has 0.
status-firefox11: affected → unaffected
tracking-firefox10: + → -
tracking-firefox11: + → -
Keywords: fennecnative-betablocker
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.
Whiteboard: [sg:critical] → [sg:moderate]
(Reporter)

Comment 14

5 years ago
IIRC, I discovered this bug using a less ridiculous font-inflation setting, so it could affect the default settings of mobile Firefox.
Whiteboard: [sg:moderate] → [sg:critical]

Updated

5 years ago
status-firefox14: --- → affected
tracking-firefox14: --- → +
tracking-firefox15: --- → +
Keywords: sec-critical
tracking-fennec: --- → ?
Not sure if this should block fennec-1.0. It is a sg:crit but it seems to be a footgun case.
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +

Comment 16

5 years ago
Should we be fixing this still for first release?
Whiteboard: [sg:critical] → [sg:critical] readability
Aaron, can you confirm default settings for font inflation on fennec beta?
(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
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
(Assignee)

Comment 20

5 years ago
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
(Assignee)

Comment 21

5 years ago
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

5 years ago
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.
(Assignee)

Comment 23

5 years ago
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.
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.
(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.
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
If emPerLine and/or minTwips is non-zero then font-inflation is enabled.
blocking-fennec1.0: + → .N+
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.
(Assignee)

Comment 29

5 years ago
(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.
status1.9.2: unaffected → ---
status-firefox10: unaffected → ---
status-firefox12: affected → wontfix
status-firefox13: affected → wontfix
status-firefox15: --- → affected
status-firefox16: --- → affected
tracking-firefox-esr10: - → ---
tracking-firefox10: - → ---
tracking-firefox11: - → ---
tracking-firefox16: --- → +
(Assignee)

Comment 30

5 years ago
Reproducing this bug with this testcase now requires commenting out the lines quoted in bug 764354 comment 12.
(Assignee)

Comment 31

5 years ago
(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.
(Assignee)

Comment 32

5 years ago
(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.
(Assignee)

Comment 33

5 years ago
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.
Summary: Crash [@ ClearAllTextRunReferences] with font inflation → Crash [@ ClearAllTextRunReferences] with font inflation and 'white-space' that preserves newlines
(Assignee)

Comment 34

5 years ago
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.
(Assignee)

Comment 35

5 years ago
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.
(Assignee)

Comment 36

5 years ago
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);
(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.
(Assignee)

Comment 38

5 years ago
(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.
(Assignee)

Comment 39

5 years ago
Created attachment 635926 [details] [diff] [review]
patch
Attachment #635926 - Flags: review?(roc)
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.
Attachment #635926 - Flags: review?(roc) → review+
(Assignee)

Comment 41

5 years ago
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.)
Right. Never mind then.
(Assignee)

Comment 43

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd9f0911976b
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/dd9f0911976b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 45

5 years ago
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
Attachment #635926 - Flags: approval-mozilla-beta?
Attachment #635926 - Flags: approval-mozilla-aurora?
Comment on attachment 635926 [details] [diff] [review]
patch

[Triage Comment]
Please land on aurora 15 in preparation for beta 14 consideration. Thanks!
Attachment #635926 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 47

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/c3a418c44e8c
status-firefox15: affected → fixed
status-firefox16: affected → fixed
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.
Attachment #635926 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 49

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/4e4dd48e0b31
status-firefox14: affected → fixed
Whiteboard: [sg:critical] readability → [sg:critical][advisory-tracking+] readability
Group: core-security

Comment 50

5 years ago
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.
status-firefox14: fixed → verified
status-firefox15: fixed → verified
Verified fixed on FF 16 2012-09-17-mozilla-beta-debug build on Mac OS X 10.6.8
status-firefox16: fixed → verified
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.