Bugzilla

Comment 1

•

6 years ago

Attached file testcase 2 — Details

Here's a second testcase which is a tad simpler / more targeted than the first.

Still includes a bunch of bizarre stuff -- multicol, perspective, <dialog>, <label>, <li>.

Updated

•

6 years ago

Summary: heap-buffer-overflow in [@ nsInlineFrame::UpdateStyleOfOwnedAnonBoxesForIBSplit] → heap-buffer-overflow in [@ nsInlineFrame::UpdateStyleOfOwnedAnonBoxesForIBSplit] with multicol, perspective, <dialog>, <label>, <li>

Comment 2

•

6 years ago

This crashes a normal Nightly build, too, BTW -- no ASAN required.

If I disable stylo (layout.css.servo.enabled), neither testcase crashes (in either ASAN or Nightly builds).  So this superficially looks like a regression from shipping stylo. And indeed, mozregression traces it to this range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5
...which includes "enable stylo by default".

Though -- while Firefox Nightly 57 *was* affected (as I can verify by running "mozregression --launch 7b3215e18443 -a /path/to/testcase.html"), I *cannot* reproduce in Firefox 57 release (latest version).  So there's some other variable involved which was enabled on Nightly57 & disabled on release-57.

But Firefox 58 Beta *is* affected (I can reproduce the crash there). So from a user's perspective, it looks like this will manifest as a regression in Firefox 58. Setting flags accordingly.

emilio, maybe you could take a look?

status-firefox57: --- → unaffected

status-firefox58: --- → affected

Flags: needinfo?(emilio)

Keywords: regression

Summary: heap-buffer-overflow in [@ nsInlineFrame::UpdateStyleOfOwnedAnonBoxesForIBSplit] with multicol, perspective, <dialog>, <label>, <li> → [stylo] heap-buffer-overflow in [@ nsInlineFrame::UpdateStyleOfOwnedAnonBoxesForIBSplit] with multicol, perspective, <dialog>, <label>, <li>

Comment 3

•

6 years ago

Note: in a debug m-c build with stylo enabled (i.e. the default), I hit the following assertions (the last one fatal, inside the same call to UpdateStyleOfOwnedAnonBoxesForIBSplit that crashes in the backtrace quoted in comment 0):

=======
###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file $SRC/layout/base/nsLayoutUtils.cpp, line 7993

###!!! ASSERTION: why should we have flushed style again?: 'target == FlushTarget::ParentOnly || (mPresShell && currentGeneration == mPresShell->GetPresContext()->GetUndisplayedRestyleGeneration())', file $SRC/layout/style/nsComputedDOMStyle.cpp, line 1084

Assertion failure: nextInline (There is always a trailing inline in an IB split), at $SRC/layout/generic/nsInlineFrame.cpp:989
=======


If I disable stylo, then I get these nonfatal assertions instead (the first assertion is the same, the other one is different):

=======
###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file $SRC/layout/base/nsLayoutUtils.cpp, line 7993

###!!! ASSERTION: How did that happen?: 'nextCont', file $SRC/layout/painting/nsCSSRendering.cpp, line 320
=======

In both cases, the final assertion seems to be about the same thing -- a null next-continuation pointer on a frame that's marked as being part of an IB split.  Looking for recent bugs with similar buzzwords -- it looks like this is related to bug 1405443, which starts with the same assertion ("frame tree not empty"), and which mentions page-break-inside:avoid, columns, and ib splits.  Or maybe this is bug 1402766 resurfacing in a different way? (which was also a servo-specific crash in UpdateStyleOfOwnedAnonBoxesForIBSplit, with multicol & page-break-inside:avoid)

Comment 4

•

6 years ago

(Also: strangely, in an --enable-optimize --enable-debug-symbols build, we crash one line *earlier* than we do in a debug build, as suggested by the line numbers in Comment 0's backtrace vs. the line number of the fatal assertion in comment 3. Specifically, we crash in a InvalidArrayIndex_CRASH(...) invocation, inside of nsTArray_Impl<mozilla::FrameProperties::PropertyValue, nsTArrayInfallibleAllocator>::ElementAt for the property-table lookup here just before the assertion:
https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/layout/generic/nsInlineFrame.cpp#988

InvalidArrayIndex_CRASH looks like an intentionally-caught safe crash (per its name), but I don't know enough about it to be sure whether we hit it reliably on user machines & release builds.

Comment 5

•

6 years ago

Hmm, this looks familiar indeed...

So this seems to be crashing on one of the scrollLeft setters of marquee in: https://searchfox.org/mozilla-central/source/layout/style/xbl-marquee/xbl-marquee.xml

Why is this a HBO though? Seems like nsTArray::IndexOf should never ever read out of bounds...

Comment 6

•

6 years ago

Attached file Stack from which the restyle is posted. — Details

This is the restyle that causes the:

 "why should we have flushed style again?"

Assertion.

That looks pretty bogus.

Comment 7

•

6 years ago

That was with test-case 1, let's see the second.

Comment 8

•

6 years ago

Attached file Frame tree of test-case 2 before the inline gets deleted. — Details

Just for reference.

Boris Zbarsky [:bzbarsky]

Comment 9

•

6 years ago

Attached file Stack of the check that makes us report complete, thus deleting the inline. — Details

This is the page-break: avoid check (I think) that makes us mess up, report a complete reflow and delete the inline frame incorrectly.

`kidFrame` in nsAbsoluteContainingBlock, is the dialog frame, nextFrame a continuation of it, that we will delete next, and where the inline holds from:

Block(li)(1)@7f1691c5abd8 parent=7f1691c5aa50 next-in-flow=7f1691c5b6c0 {0,0,23760,1140} vis-overflow=-840,0,24600,2240 scr-overflow=-840,0,24600,2240 [state=0001122041100201] perspective [content=7f168e742790] [sc=7f168e748208]<
  line 7f1691c5b2a8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x500] {0,0,0,1140} vis-overflow=-840,427,840,473 scr-overflow=-840,427,840,473 <
    Placeholder(dialog)(1)@7f1691c5ad38 parent=7f1691c5abd8 {0,900,0,0} [state=0000000000200000] [content=7f168e742820] [sc=7f168e748808:-moz-oof-placeholder] outOfFlowFrame=Block(dialog)(1)@7f1691c5ac88
  >
  AbsoluteList 0x7f16a051a3e0 <
    Block(dialog)(1)@7f1691c5ac88 parent=7f1691c5abd8 next-in-flow=7f1691c5b600 {10800,0,2160,2240} [state=0000162000d00301] [content=7f168e742820] [sc=7f168e748308]<
      line 7f1691c5b1b8: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x300] {1080,1080,0,0} <
        Inline(label)(1)@7f1691c5ada8 parent=7f1691c5ac88 next=7f1691c5b028 IBSplitSibling=7f1691c5b028 {1080,180,0,1140} vis-overflow=0,0,0,0 scr-overflow=0,900,0,0 [state=0080100000008200] [content=7f168e7428b0] [sc=7f168e748408]<
          Text(0)"\n        "@7f1691c5ae38 parent=7f1691c5ada8 {0,900,0,0} [state=4000000028600000] [content=7f16b2869d80] [sc=7f168e748a08:-moz-text] [run=7f16942bfba0][0,9,T] 
        >
      >
      line 7f1691c5b208: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8] {1080,1080,0,1140} vis-overflow=240,1080,840,1140 scr-overflow=240,1080,840,1140 <
        Block(label)(1)@7f1691c5b028 parent=7f1691c5ac88 IBSplitSibling=7f1691c5b128 IBSplitPrevSibling=7f1691c5ada8 {1080,1080,0,1140} vis-overflow=-840,0,840,1140 scr-overflow=-840,0,840,1140 [state=0000020000108000] [content=7f168e7428b0] [sc=7f168e748d08:-moz-block-inside-inline-wrapper]<
          line 7f1691c5b0d8: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x108] {0,0,0,1140} vis-overflow=-840,0,840,1140 scr-overflow=-840,0,840,1140 <
            Block(li)(1)@7f1691c5aec0 parent=7f1691c5b028 {0,0,0,1140} vis-overflow=-840,0,840,1140 scr-overflow=-840,0,840,1140 [state=0000120040100200] [content=7f168e742940] [sc=7f168e748508]<
              BulletList 0x7f1691c5b018 <
                Bullet(li)(1)@7f1691c5af70 parent=7f1691c5aec0 {-840,427,840,473} [content=7f168e742940] [sc=7f168e748c08:-moz-list-bullet]
              >
            >
          >
        >
      >
    >
  >
  BulletList 0x7f1691c5b3a0 <
    Bullet(li)(1)@7f1691c5b2f8 parent=7f1691c5abd8 {-840,427,840,473} [content=7f168e742790] [sc=7f168e748e08:-moz-list-bullet]
  >
>

Comment 10

•

6 years ago

So the history here is that bug 1402766 landed a wallpaper null-check in the UpdateStyleOfOwnedAnonBoxesForIBSplit code.  This landed on nightly 58 and was backported to beta 57.

Bug 1405443 was filed to fix the real underlying issue.  Once that was fixed, the wallpaper was removed.  That happened in 58.

This testcase seems to trigger the issue bug 1405443 was filed on, in a slightly different way.  That is, the fix for bug 1405443 was not complete.  The above history is why it crashes in 57 nightly but not 57 release (which has the wallpaper), and in 58 beta (which does not have the wallpaper).

I think we should reland bug 1402766 on beta 58 (I filed bug 1431282 on this) and work on fixing the real underlying issue here: page-break-inside:avoid can trigger the "frame tree not empty, but caller reported complete status" assertion, after which we destroy random frames that other parts of the frame tree might be referencing.  We clean up the pointers, so it's not UAF, but we get nulls where we should never have nulls.

Boris Zbarsky [:bzbarsky]

Updated

•

6 years ago

Flags: needinfo?(mats)

Andrew McCreight [:mccr8]

Comment 11

•

6 years ago

The check for completion that doesn't check for page-break-inside is:

  https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/layout/generic/nsAbsoluteContainingBlock.cpp#170

What's the right thing to do there where you get to that situation, I wish I knew :)

Flags: needinfo?(emilio)

Updated

•

6 years ago

Keywords: sec-high

Boris Zbarsky [:bzbarsky]

Comment 12

•

6 years ago

See also the publicly-visible bug 1431781, which was just filed with a testcase that triggers some of the same assertions in debug build (but doesn't cause any obvious security problems / crashes in opt builds).

Perhaps it might be easier to fix the [shared?] root cause on that bug, in the open, rather than this one.

As expected, bug 1431282 fixed the crash and fatal assertion.
The non-fatal assertions still occurs though:
ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', layout/base/nsLayoutUtils.cpp, line 7995
ASSERTION: frame tree not empty, but caller reported complete status: 'start == end || IsInLetterFrame(aSubtreeRoot)', layout/base/nsLayoutUtils.cpp, line 8009
ASSERTION: How did that happen?: 'nextCont', layout/painting/nsCSSRendering.cpp, line 320
ASSERTION: why should we have flushed style again?: 'target == FlushTarget::ParentOnly || (mPresShell && currentGeneration == mPresShell->GetPresContext()->GetUndisplayedRestyleGeneration())', layout/style/nsComputedDOMStyle.cpp, line 1084

Depends on: 1431282

Assignee

Comment 14

•

6 years ago

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Perhaps it might be easier to fix the [shared?] root cause on that bug, in
> the open, rather than this one.

This bug is unrelated to bug 1431781.

Assignee

Comment 15

•

6 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> What's the right thing to do there where you get to that situation, I wish I
> knew :)

I think we could either introduce some sort of (empty) overflow container
frames *at the start* of the continuation chain, or allow abs.pos. frames
to be pushed.  Either one is probably non-trivial to implement.

Assignee

Comment 16

•

6 years ago

Attached patch wip (obsolete) — Details — Splinter Review

This might work as a wallpaper for now...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d6f60e6523b240a0d3ee61fecb8b765ffa8c2ba

Flags: needinfo?(mats)

Assignee

Comment 17

•

6 years ago

Attached patch fix — Details — Splinter Review

I think this is a reasonable wallpaper for now.

Assignee: nobody → mats

Attachment #8944221 - Attachment is obsolete: true

Attachment #8944310 - Flags: review?(dholbert)

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 18

•

6 years ago

Comment on attachment 8944310 [details] [diff] [review]
fix

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

Commit message nit:
> Bug 1431232 - Treat break-before status as Incomplete for now. r=dholbert

Please add "for abspos frames" or something like that, to avoid giving the impression that we're suppressing break-before entirely.

::: layout/generic/nsAbsoluteContainingBlock.cpp
@@ +172,4 @@
>            aDelegatingFrame->IsFrameOfType(nsIFrame::eCanContainOverflowContainers)) {
> +        // XXX it's unclear how we should handle 'page-break-inside:avoid' on
> +        // abs.pos. boxes -- ignore it for now by setting the status to
> +        // Incomplete (which will fragment it).

s/will/will probably/

(Hypothetically, 'kidFrame' here might not be a fragmentable frame-type -- so stating that we "will fragment it" might be wrong.)

Attachment #8944310 - Flags: review?(dholbert) → review+

Comment 19

•

6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0a943fd35d07a935d7ac027f3d33aa16f7cd29
https://hg.mozilla.org/mozilla-central/rev/9a0a943fd35d

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox60: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla60

Ryan VanderMeulen [:RyanVM]

Comment 20

•

6 years ago

I believe this should have had sec-approval before landing since it's a sec-high affecting more than just trunk. Please do the request when you get a chance and also request Beta uplift.

status-firefox-esr52: --- → unaffected

Flags: needinfo?(mats)

Version: 59 Branch → 58 Branch

Comment 21

•

6 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> I believe this should have had sec-approval before landing since it's a
> sec-high affecting more than just trunk. Please do the request when you get
> a chance and also request Beta uplift.

I think this is just a null deref fwiw, when I was looking at it at least.

Assignee

Comment 22

•

6 years ago

I believe the sec-high part of this bug was fixed by bug 1431282 (see
comment 10 above).  With that fix, the testcases here don't crash, so
this bug is now only about the non-fatal assertions in comment 13.
I believe those could at most lead to stale frame pointers so any
potential crashes should be covered by frame-poisoning.

The conditions to trigger the assertions seems like an edge case to me,
so I don't think it needs to be uplifted.  It's fairly low-risk though
(for the same reason) so feel free to request it if you want it anyway.

Flags: needinfo?(mats)

Keywords: csectype-bounds, sec-high → csectype-framepoisoning, sec-other

OS: Unspecified → All

Hardware: Unspecified → All