1041512 - (CVE-2014-1576) Heap-buffer-overflow in nsTransformedTextRun::SetCapitalization

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Atte Kettunen]

Attachment: Repro-file

Tested on:

OS: Ubuntu 12.04

Firefox: ASAN debug prebuild from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/1405918761/

Sometimes the repro-file fails to trigger the crash. If the crash doesn't occur after couple of refresh, restart the browser. Also the crash reproduces a lot more reliably with debug-build than with opt-build. 


ASAN trace:

[7111] ###!!! ASSERTION: Text run should be transformed!: 'mTextRun->GetFlags() & nsTextFrameUtils::TEXT_IS_TRANSFORMED', file /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp, line 970
=================================================================
==7111==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60c0002d1a40 at pc 0x7fdb4338f050 bp 0x7fffaeeeb670 sp 0x7fffaeeeb668
READ of size 8 at 0x60c0002d1a40 thread T0
    #0 0x7fdb4338f04f in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::Length() const /builds/slave/m-cen-l64-asan-d-0000000000000/build/xpcom/build/../glue/nsTArray.h:323
    #1 0x7fdb433fe598 in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::IsEmpty() const /builds/slave/m-cen-l64-asan-d-0000000000000/build/obj-firefox/xpcom/base/../../dist/include/nsTArray.h:326
    #2 0x7fdb47483e48 in nsTransformedTextRun::SetCapitalization(unsigned int, unsigned int, bool*, gfxContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextRunTransformations.cpp:60
    #3 0x7fdb46783ded in nsLineBreaker::AppendText(nsIAtom*, char16_t const*, unsigned int, unsigned int, nsILineBreakSink*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/content/base/src/nsLineBreaker.cpp:291
    #4 0x7fdb46784360 in nsLineBreaker::AppendText(nsIAtom*, unsigned char const*, unsigned int, unsigned int, nsILineBreakSink*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/content/base/src/nsLineBreaker.cpp:327
    #5 0x7fdb4745ddca in BuildTextRunsScanner::SetupBreakSinksForTextRun(gfxTextRun*, void const*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2399
    #6 0x7fdb474580b1 in BuildTextRunsScanner::SetupLineBreakerContext(gfxTextRun*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2304
    #7 0x7fdb474574ea in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:1453
    #8 0x7fdb47460bee in BuildTextRuns(gfxContext*, nsTextFrame*, nsIFrame*, nsLineList_iterator const*, nsTextFrame::TextRunType) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:1401
    #9 0x7fdb4745f4e8 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, gfxContext*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2559
    #10 0x7fdb4747a49e in nsTextFrame::AddInlineMinWidthForFlow(nsRenderingContext*, nsIFrame::InlineMinWidthData*, nsTextFrame::TextRunType) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:7018
    #11 0x7fdb4747b87f in nsTextFrame::AddInlineMinWidth(nsRenderingContext*, nsIFrame::InlineMinWidthData*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:7175
    #12 0x7fdb4731cb1d in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:733
    .
    .
    .
    #60 0x7fdb48107135 in nsAppStartup::Run() /builds/slave/m-cen-l64-asan-d-0000000000000/build/toolkit/components/startup/nsAppStartup.cpp:278
    #61 0x7fdb47f69b3b in XREMain::XRE_mainRun() /builds/slave/m-cen-l64-asan-d-0000000000000/build/toolkit/xre/nsAppRunner.cpp:4013
    #62 0x7fdb47f6af4f in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/toolkit/xre/nsAppRunner.cpp:4084
    #63 0x7fdb47f6b9d2 in XRE_main /builds/slave/m-cen-l64-asan-d-0000000000000/build/toolkit/xre/nsAppRunner.cpp:4298
    #64 0x489fed in do_main(int, char**, nsIFile*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/browser/app/nsBrowserApp.cpp:282
    #65 0x489571 in main /builds/slave/m-cen-l64-asan-d-0000000000000/build/browser/app/nsBrowserApp.cpp:643
    #66 0x7fdb529e276c in ?? ??:0
    #67 0x48925c in _start ??:0

0x60c0002d1a40 is located 0 bytes to the right of 128-byte region [0x60c0002d19c0,0x60c0002d1a40)
allocated by thread T0 here:
    #0 0x471871 in __interceptor_malloc _asan_rtl_
    #1 0x7fdb449893f8 in gfxTextRun::AllocateStorageForTextRun(unsigned long, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/gfx/thebes/gfxFont.cpp:6461
    #2 0x7fdb4497fb13 in gfxTextRun::Create(gfxTextRunFactory::Parameters const*, unsigned int, gfxFontGroup*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/gfx/thebes/gfxFont.cpp:6478
    #3 0x7fdb44980f38 in gfxFontGroup::MakeTextRun(unsigned char const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/gfx/thebes/gfxFont.cpp:5196
    #4 0x7fdb4745d811 in gfxTextRun* MakeTextRun<unsigned char>(unsigned char const*, unsigned int, gfxFontGroup*, gfxTextRunFactory::Parameters const*, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:569
    #5 0x7fdb47459dc5 in BuildTextRunsScanner::BuildTextRunForFrames(void*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2145
    #6 0x7fdb47457642 in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:1472
    #7 0x7fdb47460bee in BuildTextRuns(gfxContext*, nsTextFrame*, nsIFrame*, nsLineList_iterator const*, nsTextFrame::TextRunType) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:1401
    #8 0x7fdb4745f4e8 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, gfxContext*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:2559
    #9 0x7fdb4747a49e in nsTextFrame::AddInlineMinWidthForFlow(nsRenderingContext*, nsIFrame::InlineMinWidthData*, nsTextFrame::TextRunType) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:7018
    #10 0x7fdb4747b87f in nsTextFrame::AddInlineMinWidth(nsRenderingContext*, nsIFrame::InlineMinWidthData*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsTextFrame.cpp:7175
    #11 0x7fdb4731cb1d in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:733
    #12 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #13 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #14 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #15 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #16 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #17 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #18 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #19 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #20 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #21 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #22 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #23 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #24 0x7fdb472ab05a in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicWidthType, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/base/nsLayoutUtils.cpp:3732
    #25 0x7fdb4731c958 in nsBlockFrame::GetMinWidth(nsRenderingContext*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsBlockFrame.cpp:713
    #26 0x7fdb4739806e in nsFrame::ShrinkWidthToFit(nsRenderingContext*, int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsFrame.cpp:4240
    #27 0x7fdb47364d14 in nsContainerFrame::ComputeAutoSize(nsRenderingContext*, nsSize, int, nsSize, nsSize, nsSize, bool) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsContainerFrame.cpp:899
    #28 0x7fdb47396adc in nsFrame::ComputeSize(nsRenderingContext*, nsSize, int, nsSize, nsSize, nsSize, unsigned int) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsFrame.cpp:4030
    #29 0x7fdb473f7c1d in nsHTMLReflowState::InitAbsoluteConstraints(nsPresContext*, nsHTMLReflowState const*, int, int, nsIAtom*) /builds/slave/m-cen-l64-asan-d-0000000000000/build/layout/generic/nsHTMLReflowState.cpp:1458

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ??
Shadow bytes around the buggy address:
  0x0c18800522f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1880052300: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c1880052310: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
  0x0c1880052320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1880052330: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
=>0x0c1880052340: 00 00 00 00 00 00 00 00[fa]fa fa fa fa fa fa fa
  0x0c1880052350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1880052360: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c1880052370: 00 00 00 00 00 00 00 04 fa fa fa fa fa fa fa fa
  0x0c1880052380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c1880052390: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Contiguous container OOB:fc
  ASan internal:           fe
==7111==ABORTING

Comment 1

[Andrew McCreight [:mccr8]]

Nathan, do you know how we could hit a buffer overflow while calling Length() on an nsTArray? That seems weird to me.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Nathan, do you know how we could hit a buffer overflow while calling
> Length() on an nsTArray? That seems weird to me.

I'd guess somebody stomping on the Header pointer for the array.  Just from looking at the stacks above, that sounds plausible...

Comment 3

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

Jonathan, can you recreate this bug? (Or find someone to take a look?)

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Nathan, do you know how we could hit a buffer overflow while calling
> Length() on an nsTArray? That seems weird to me.

I don't think that's exactly what's happening. The problem here is that we've got a normal gfxTextRun, as created via gfxFontGroup::MakeTextRun; but then we're treating it as an nsTransformedTextRun, which is a subclass that has some additional fields. The nsTArray we're trying to look at in SetCapitalization is one of those added fields, but because this textrun was not an nsTransformedTextRun at all, the field isn't present, and the attempt to access its header ends up looking beyond the allocated size of the object. The problem isn't that the nsTArray we're examining has been stomped; it's that there was no nsTArray there to be examined.

The "ASSERTION: Text run should be transformed!" that's firing before the ASan error confirms this, too. That assertion is at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#969, and if it fires, then the static_cast immediately following is clearly invalid, and will lead directly to the failure here.

So we need to figure out why we haven't got the kind of textrun we expected here. Offhand, my guess is that perhaps we're failing to discard and rebuild a textrun at some point when style information changes dynamically. A reduced/simplified testcase that reproduces the assertion would be awesome... maybe something that involves changing line widths, so that we're constantly re-doing linebreaking, along with toggling the text-transform:capitalize property.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #4)
>  A reduced/simplified testcase that reproduces the assertion would be
> awesome... maybe something that involves changing line widths, so that we're
> constantly re-doing linebreaking, along with toggling the
> text-transform:capitalize property.

My naive attempt to make a testcase along these lines doesn't seem to hit the problem at all. The original testcase here does hit the assertion (and then crashes), but it's far from being easily debuggable. Mats, you have any insight on how to track this down?

Comment 6

[Mats Palmgren (inactive)]

Attachment: wip

As you guessed, it's a "text-transform:capitalize" style change
that makes the existing textrun bogus since it's of the wrong type
for the new style data. 

I don't know where we're supposed to detect this and discard the
old textrun, but DidSetStyleContext seems like a good candidate.

Comment 7

[Mats Palmgren (inactive)]

It also crashes a -release DEBUG build on Linux64 so I suspect it affects
all branches.

Comment 8

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

It seems odd that that would be necessary given that nsStyleText::CalcDifference returns NS_STYLE_HINT_REFLOW which includes nsChangeHint_ClearAncestorIntrinsics and nsChangeHint_ClearDescendantIntrinsics, and nsTextFrame::MarkIntrinsicISizesDirty already calls ClearTextRuns.  I'm curious which step along that path isn't happening as it should be.

Comment 9

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

Also, if that WIP fixes things, than anything pre-Gecko 31 is not affected, since this would be a regression from bug 950526.

Comment 10

[Mats Palmgren (inactive)]

I think the frame in question had not been reflowed at all, so
RestyleManager::StyleChangeReflow took the early return.

Comment 11

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

Interesting.

I guess I see two possible fixes for that problem:

 (1) push the not-reflowed-at-all test deeper in, into the dirty-bits part of FrameNeedsReflow

 (2) add a separate style change hint for clearing text runs

Presumably this will show up in the form of correctness bugs as well.

(I'm curious how we managed to build text runs for a frame and not reflow it before going back to the event loop, but I guess that should be allowable, given interruptible reflow.  Or do we build text runs in frame construction?)

Comment 12

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #9)
> Also, if that WIP fixes things, than anything pre-Gecko 31 is not affected,
> since this would be a regression from bug 950526.

The WIP fixes the crash, so the unconditional ClearTextRuns() in
nsTextFrame::DidSetStyleContext in builds before bug 950526 should
make them unaffected.
https://bugzilla.mozilla.org/attachment.cgi?id=8393354&action=diff#a/layout/generic/nsTextFrame.cpp_sec2

FWIW, I confirmed by testing an ESR24 DEBUG build on Linux64 without crashing.

Comment 13

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #11)
> (I'm curious how we managed to build text runs for a frame and not reflow it
> before going back to the event loop, but I guess that should be allowable,
> given interruptible reflow.  Or do we build text runs in frame construction?)

This is the stack where we create the initial text run:
#0  nsTextFrame::SetTextRun
#1  BuildTextRunsScanner::AssignTextRun
#2  BuildTextRunsScanner::BuildTextRunForFrames
#3  BuildTextRunsScanner::FlushFrames
#4  BuildTextRuns
#5  nsTextFrame::EnsureTextRun
#6  nsTextFrame::AddInlineMinISizeForFlow
#7  nsTextFrame::AddInlineMinISize
#8  nsBlockFrame::GetMinISize
#9  nsLayoutUtils::IntrinsicForContainer
... GetMinISize / IntrinsicForContainer ...
#23 nsLayoutUtils::IntrinsicForContainer
#24 nsBlockFrame::GetMinISize
#25 nsFrame::ShrinkWidthToFit
#26 nsContainerFrame::ComputeAutoSize
#27 nsFrame::ComputeSize
#28 nsHTMLReflowState::InitAbsoluteConstraints
#29 nsHTMLReflowState::InitConstraints
#30 nsHTMLReflowState::Init
#31 nsHTMLReflowState::nsHTMLReflowState
#32 nsAbsoluteContainingBlock::ReflowAbsoluteFrame
#33 nsAbsoluteContainingBlock::Reflow
#34 ViewportFrame::Reflow
#35 PresShell::DoReflow
...

The Reflow at #33 is interrupted before we reflow the text frame (at an
ancestor 4-5 levels up).  We exit ViewportFrame::Reflow and then the
restyling happens.  The next time we enter ViewportFrame::Reflow we
crash with the same stack up to FlushFrames, see comment 0.

Comment 14

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #11)
> I guess I see two possible fixes for that problem:
> 
>  (1) push the not-reflowed-at-all test deeper in, into the dirty-bits part
> of FrameNeedsReflow
> 
>  (2) add a separate style change hint for clearing text runs

Both those means clearing text runs for all text frame descendants, no?

It would be nice if we could avoid that on text frames where the
style context didn't change.

Comment 15

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

The properties that require clearing text runs are almost entirely inherited properties, so many if not most changes of those properties will also change all descendants.

When we consider optimization, we should consider it from the starting point of correct code, and consider it when we have reason to think that optimization is necessary.  I don't think it's worth trying to micro-optimize this one case by papering over the bug rather than fixing the real problem, which will show up in a bunch of other ways than just text-transform cases (although those might be the only ones that crash).

It might make some sense to take your patch as a *secondary* belt-and-suspenders fix for the bug.  (It would be nice to assert that we don't hit the need for it, but I think things happen in the wrong order to be able to do that.)

Comment 17

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

Any updates on a patch for this?

Comment 18

[Mats Palmgren (inactive)]

I guess we could take my "wip" patch as a wallpaper for branches if you want.

The preferred fix, described in comment 15, is mostly style system work.
Cameron can take it perhaps?

Comment 19

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (UTC+2) (needinfo? for questions) (away/busy until Sep 11) from comment #11)
> I guess I see two possible fixes for that problem:
> 
>  (1) push the not-reflowed-at-all test deeper in, into the dirty-bits part
> of FrameNeedsReflow
> 
>  (2) add a separate style change hint for clearing text runs

I went with the first approach.  I tried adding a change hint initially, but since I'd need to treat this new hint in pretty much the same way as the existing Clear{Ancestor,Descendant}Intrinsics hint, it didn't seem worth it in the end.

Comment 20

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 21

[Cameron McCormack (:heycam)]

By the way, is there a way to write a test that relies on interruptible reflow interrupting at a certain point?

Comment 22

[Cameron McCormack (:heycam)]

Comment on attachment 8487740 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not obvious from the patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

Whichever branches are based on Gecko 31.

If not all supported branches, which bug introduced the flaw?

Bug 950526.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

No but I suspect the patch will apply to older branches since the changed bits of code haven't been touched lately.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely to cause regressions.  Not sure how to make an automated regression test for it.  Manual testing shows that the ASAN issue with the test case in this bug is resolved by the patch.

Comment 23

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

sec-approval+ for trunk. We should get Aurora, Beta, and ESR31 patches nominated as well.

Comment 24

[Cameron McCormack (:heycam)]

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

Comment 25

[Cameron McCormack (:heycam)]

Comment on attachment 8487740 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 35
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: bug 950526
[User impact if declined]: sg:high bug remains
[Describe test coverage new/current, TBPL]: tested locally with the test case in this bug; only just landed on inbound
[Risks and why]: should be low, as we're only calling MarkIntrinsicISizesDirty on some frames where we didn't before
[String/UUID change made/needed]: none

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80e3a7e6be35
https://hg.mozilla.org/releases/mozilla-beta/rev/dafe68644b45
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/cfedc95605d4
https://hg.mozilla.org/releases/mozilla-esr31/rev/d7140f3afd61

Comment 28

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on Fx31.1.0esr, ASan build
Verified fixed on Fx31.1.1esr.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/cfedc95605d4

Comment 30

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build that was mentioned in comment #0:
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/1405918761/

fx35: Passed
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/1412330524/

fx34: Passed
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan-debug/1412347357/

fx33: Passed
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan-debug/1412347593/

Test Cases used on all the channels:

- Opened the poc from comment #0 in three different windows/tabs and let each page refresh several times
- Opened the poc from comment #0 in three different private window/tabs and let each page refresh several times
- Opened the poc from comment #0 in three different e10s window/tabs and let each page refresh several times (m-c only)