Closed Bug 718290 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Layout, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla16
Tracking Status
firefox11 --- unaffected
firefox12 + wontfix
firefox13 + wontfix
firefox14 + verified
firefox15 + verified
firefox16 + verified
firefox-esr10 --- unaffected
blocking-fennec1.0 --- .N+

People

(Reporter: jruderman, Assigned: dbaron)

References

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [sg:critical][advisory-tracking+] readability)

Attachments

(3 files)

Attached file 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.
Attached file stack trace+
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
Group: core-security
Blocks: framedest
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?
Why do you think it's a guaranteed null deref?
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.
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.
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.
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]
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]
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: ? → +
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
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
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)
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.
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.
(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.
Reproducing this bug with this testcase now requires commenting out the lines quoted in bug 764354 comment 12.
(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.
(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.
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
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.
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.
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.
(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 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+
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.)
https://hg.mozilla.org/mozilla-central/rev/dd9f0911976b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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+
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+
Whiteboard: [sg:critical] readability → [sg:critical][advisory-tracking+] readability
Group: core-security
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.
Verified fixed on FF 16 2012-09-17-mozilla-beta-debug build on Mac OS X 10.6.8
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.