1373767 - Null-deref crash [@ mozilla::BlockReflowInput::BlockReflowInput], with fixed-pos MathML inside of XUL grid

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes a null-deref crash in mozilla-central rev fe809f57bf22.

==10815==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fc69d0dd5ff bp 0x7ffecfad1fe0 sp 0x7ffecfad1f00 T0)
==10815==The signal is caused by a READ memory access.
==10815==Hint: address points to the zero page.
    #0 0x7fc69d0dd5fe in mozilla::BlockReflowInput::BlockReflowInput(mozilla::ReflowInput const&, nsPresContext*, nsBlockFrame*, bool, bool, bool, int) /home/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:106:34
    #1 0x7fc69d1433d4 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1178:20
    #2 0x7fc69d130343 in nsAbsoluteContainingBlock::ReflowAbsoluteFrame(nsIFrame*, nsPresContext*, mozilla::ReflowInput const&, nsRect const&, nsAbsoluteContainingBlock::AbsPosReflowFlags, nsIFrame*, nsReflowStatus&, nsOverflowAreas*) /hom
e/worker/workspace/build/src/layout/generic/nsAbsoluteContainingBlock.cpp:721:14
    #3 0x7fc69d12b053 in nsAbsoluteContainingBlock::Reflow(nsContainerFrame*, nsPresContext*, mozilla::ReflowInput const&, nsReflowStatus&, nsRect const&, nsAbsoluteContainingBlock::AbsPosReflowFlags, nsOverflowAreas*) /home/worker/workspa
ce/build/src/layout/generic/nsAbsoluteContainingBlock.cpp:166:7
    #4 0x7fc69d202a10 in nsFrame::ReflowAbsoluteFrames(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&, bool) /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:5952:24
    #5 0x7fc69d220265 in nsFrame::DoXULLayout(nsBoxLayoutState&) /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:9925:5
    #6 0x7fc69d5d82a1 in nsIFrame::XULLayout(nsBoxLayoutState&) /home/worker/workspace/build/src/layout/xul/nsBox.cpp:501:8
    #7 0x7fc69d66b345 in nsStackLayout::XULLayout(nsIFrame*, nsBoxLayoutState&) /home/worker/workspace/build/src/layout/xul/nsStackLayout.cpp:369:18
    #8 0x7fc69d5de8f0 in nsBoxFrame::DoXULLayout(nsBoxLayoutState&) /home/worker/workspace/build/src/layout/xul/nsBoxFrame.cpp:919:26
    #9 0x7fc69d5dc743 in XULLayout /home/worker/workspace/build/src/layout/xul/nsBox.cpp:501:8
    #10 0x7fc69d5dc743 in nsBoxFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/xul/nsBoxFrame.cpp:715
    #11 0x7fc69d2e594c in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:921:13
    #12 0x7fc69d167bd4 in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4217:15
    #13 0x7fc69d1667e7 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /home/worker/workspace/build/s
rc/layout/generic/nsBlockFrame.cpp:4013:5
    #14 0x7fc69d15df49 in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3887:9
    #15 0x7fc69d157918 in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2873:5
    #16 0x7fc69d14ced4 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2409:7
    #17 0x7fc69d143874 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1230:3
    #18 0x7fc69d1a10da in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&
, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:932:14
    #19 0x7fc69d19fa27 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:742:5
    #20 0x7fc69d1a10da in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&
, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:932:14
    #21 0x7fc69d24c86d in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:549:3
    #22 0x7fc69d24df6e in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:661:3
    #23 0x7fc69d251189 in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:1037:3
    #24 0x7fc69d129bb3 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /home/worker/workspace/build/s
rc/layout/generic/nsContainerFrame.cpp:976:14
    #25 0x7fc69d12854a in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:328:7
    #26 0x7fc69cf38d51 in mozilla::PresShell::DoReflow(nsIFrame*, bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9318:11

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (with redundant -moz-transform styles added as well)

Here's a testcase with an added "-moz-transform" spelling of the "transform" CSS, so that this reproduces in older builds from before we unprefixed.

With that change, I get the following regression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6737b6762eb8&tochange=5ec9524de1af

In that range, this cset looks like the most likely cause, given the fixed-pos + XUL usage inside of this testcase:

>	d069fd0abc23	Daniel Holbert — Bug 640443: Allow positioned display:-moz-box elements to be containers for absolutely-positioned content. r=bz

Before that change, abspos/fixed-pos stuff inside of a XUL container (like the -moz-grid in this testcase) would simply tunnel its way out and position with respect to the viewport (or some other non-XUL ancestor).

Comment 2

[Daniel Holbert [:dholbert]]

So the problem here is that the nsMathMLmathBlockFrame has a null float manager on its ReflowInput, which should never happen.  By default, we use the ReflowInput from our parent's ReflowInput
https://dxr.mozilla.org/mozilla-central/rev/fe809f57bf2287bb937c3422ed03a63740b3448b/layout/generic/ReflowInput.cpp#227
...and in this case, we chain up to a "dummy" ReflowInput that was created just for abspos stuff nsFrame::DoXULLayout, and which lacks a float manager.

Comment 3

[Daniel Holbert [:dholbert]]

So in particular, this reflowState from the regressing commit (now called "reflowInput")...
  https://hg.mozilla.org/mozilla-central/rev/d069fd0abc23#l10.33
...lacks a nsFloatManager, which is why we're left without one on its descendant nsMathMLmathBlockFrame and crash.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: wip

This fixes the crash by passing in "aState.OuterReflowInput()" (where aState is a nsBoxLayoutState) in the places where it matters.

(In this case, aState.OuterReflowInput()->mFrame is the abspos containing block -- the frame with "display:-moz-grid", which is a nsBoxFrame.)

I'm not 100% sure it makes sense for us to share this float manager down to the MathML block in this case -- maybe the MathML block should do something like what we do for normal blocks, and add NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS (which includes the NS_BLOCK_FLOAT_MGR bit which forces us to create a dedicated float manager), if it's abspos?
https://dxr.mozilla.org/mozilla-central/rev/fe809f57bf2287bb937c3422ed03a63740b3448b/layout/base/nsCSSFrameConstructor.cpp#5005,5010

Comment 5

[Boris Zbarsky [:bzbarsky]]

We may want NS_NewMathMLmathBlockFrame to just always set NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1373767 part 1: Always make nsMathMLmathBlockFrame a block formatting context.

(This makes it behave a bit more like a normal CSS block, and it ensures that
it can provide a float manager to its descendants.)

Review commit: https://reviewboard.mozilla.org/r/157080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157080/

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> We may want NS_NewMathMLmathBlockFrame to just always set
> NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS.

That probably makes sense -- let's go with that. Would you mind reviewing the attached few-liner patch, when you get a chance?

Try likes it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07fb2e239b05322865100a36e565e596352024d1

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8886474 [details]
Bug 1373767 part 1: Always make nsMathMLmathBlockFrame a block formatting context.

https://reviewboard.mozilla.org/r/157080/#review162538

r=me

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14868b3c13e3
Always make nsMathMLmathBlockFrame a block formatting context. r=bz

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This caused android reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=115503713&repo=autoland

Backed out in https://hg.mozilla.org/integration/autoland/rev/1aa1c1bd2a658856e1a906e789f9d771730e1070

Comment 11

[Daniel Holbert [:dholbert]]

I'll bet that "block formatting context" == "font inflation container".  So that reftest rendering change is probably expected.  (The reftest failure shows something in red in the testcase vs. not-red in the reference -- but the testcase has a bunch of JS and specifically colors something red if its size is unexpected.  So I think the real issue is that the size is unexpected.)

So, the reftest probably just needs a tweak. I'll investigate more tomorrow.

Comment 12

[Daniel Holbert [:dholbert]]

(For the record, the failing testcase here is from bug 1002526)

Comment 13

[Daniel Holbert [:dholbert]]

(I can reproduce the reftest failure locally on Linux if I apply the patch in bug 1382450, to make this test consistent with other font inflation reftests.)

Comment 14

[Daniel Holbert [:dholbert]]

So the reftest's rendering difference happens because:
 - without the patch, our nsMathMLmathBlockFrame does not have the NS_FRAME_FONT_INFLATION_FLOW_ROOT bit set.
 - with the patch, this frame *does* have that bit set.

It's set at the end of nsBlockFrame::Init:
http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/layout/generic/nsBlockFrame.cpp#7044-7047
...and we only set it if we have NS_FRAME_FONT_INFLATION_CONTAINER | NS_BLOCK_FLOAT_MGR set. (And without this patch, we only have the former bit set -- but after this patch, we have both of those bits set, which then activates this clause & makes us set NS_FRAME_FONT_INFLATION_FLOW_ROOT as well.)

This chunk of code is from bug 706193, in a changeset that is explicitly about making font inflation be a per-BFC thing ("Add a font inflation data structure per block formatting context", https://hg.mozilla.org/mozilla-central/rev/9499f6b28add )

I can reproduce the same rendering difference if I create a similar testcase with "display: flow-root" or "float:left" instead of <math display="block">.

So: bottom-line, I think this rendering change makes sense, as a side-effect of us promoting block-level <math> elements to be BFC's.  And given that font-inflation is disabled by default anyway, I don't think we have to worry much about webcompat risk or anything like that.

So, I'm just going to adjust the test to remove this assumption.  CC'ing jfkthame (and I'll probably tag him for review on the test tweak) since he filed bug 1002526 which spawned this reftest.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: Bug 1373767 part 0: Adjust reftest "font-inflation-1.html" to allow block-level <math> to form its own font inflation container & have different sizing.

Review commit: https://reviewboard.mozilla.org/r/189300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189300/

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8886474 [details]
Bug 1373767 part 1: Always make nsMathMLmathBlockFrame a block formatting context.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157080/diff/1-2/

Comment 17

[Daniel Holbert [:dholbert]]

Circling back to this bug -- the main patch has r+, but bounced due a reftest failure, which I discussed in comment 11 & comment 14.  I've now added a "part 0" patch that adjusts the reftest to allow block-level <math> to have different sizing from inline-level <math> (since now block-level <math> will be its own BFC & hence font inflation controller).

I also rebased the main patch on top of up-to-date mozilla-central, and I filed helper-bug 1408537 to make a few unrelated tweaks to this reftest.  (This bug's patches are layered on top of that bug.)

Comment 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8918456 [details]
Bug 1373767 part 0: Adjust reftest "font-inflation-1.html" to allow block-level <math> to form its own font inflation container & have different sizing.

https://reviewboard.mozilla.org/r/189300/#review194834

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/040dec0a787c
part 0: Adjust reftest "font-inflation-1.html" to allow block-level <math> to form its own font inflation container & have different sizing. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/76176850979d
part 1: Always make nsMathMLmathBlockFrame a block formatting context. r=bz

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/040dec0a787c
https://hg.mozilla.org/mozilla-central/rev/76176850979d

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.