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

RESOLVED FIXED in Firefox 58

Status

()

defect
P2
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: truber, Assigned: dholbert)

Tracking

(Blocks 1 bug, {crash, regression, testcase})

Trunk
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 fixed)

Details

Attachments

(5 attachments)

Posted file 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
Assignee

Comment 1

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

Updated

2 years ago
Blocks: 640443
Assignee

Comment 2

2 years ago
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.
Assignee

Comment 3

2 years ago
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.
Assignee

Comment 4

2 years ago
Posted patch wipSplinter Review
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
Assignee

Updated

2 years ago
Assignee: nobody → dholbert
Flags: needinfo?(dholbert)
Assignee

Updated

2 years ago
Flags: needinfo?(dholbert)
Keywords: regression
Assignee

Updated

2 years ago
Flags: needinfo?(dholbert)
We may want NS_NewMathMLmathBlockFrame to just always set NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS.
Comment hidden (mozreview-request)
Assignee

Comment 7

2 years ago
(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
Flags: needinfo?(dholbert) → needinfo?(bzbarsky)

Comment 8

2 years ago
mozreview-review
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
Attachment #8886474 - Flags: review+

Comment 9

2 years ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14868b3c13e3
Always make nsMathMLmathBlockFrame a block formatting context. r=bz
Assignee

Comment 11

2 years ago
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.
Flags: needinfo?(bzbarsky)
Assignee

Comment 12

2 years ago
(For the record, the failing testcase here is from bug 1002526)
Assignee

Updated

2 years ago
Summary: Null-deref crash [@ mozilla::BlockReflowInput::BlockReflowInput] → Null-deref crash [@ mozilla::BlockReflowInput::BlockReflowInput], with MathML inside of XUL grid
Assignee

Comment 13

2 years ago
(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.)
Depends on: 1382450
Assignee

Comment 14

2 years ago
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.
Assignee

Updated

2 years ago
Summary: Null-deref crash [@ mozilla::BlockReflowInput::BlockReflowInput], with MathML inside of XUL grid → Null-deref crash [@ mozilla::BlockReflowInput::BlockReflowInput], with fixed-pos MathML inside of XUL grid
Priority: -- → P2
Assignee

Updated

2 years ago
Depends on: 1408537
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 17

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

2 years ago
mozreview-review
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
Attachment #8918456 - Flags: review?(jfkthame) → review+

Comment 19

2 years ago
Pushed by dholbert@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/040dec0a787c
https://hg.mozilla.org/mozilla-central/rev/76176850979d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee

Updated

2 years ago
Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.