Closed Bug 1373767 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: truber, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression, testcase)

Attachments

(5 files)

Attached 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
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).
Blocks: 640443
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.
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.
Attached 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: nobody → dholbert
Flags: needinfo?(dholbert)
Flags: needinfo?(dholbert)
Keywords: regression
Flags: needinfo?(dholbert)
We may want NS_NewMathMLmathBlockFrame to just always set NS_BLOCK_FORMATTING_CONTEXT_STATE_BITS.
(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 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+
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14868b3c13e3
Always make nsMathMLmathBlockFrame a block formatting context. r=bz
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)
(For the record, the failing testcase here is from bug 1002526)
Summary: Null-deref crash [@ mozilla::BlockReflowInput::BlockReflowInput] → Null-deref crash [@ mozilla::BlockReflowInput::BlockReflowInput], with MathML inside of XUL grid
(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
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.
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
Depends on: 1408537
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 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+
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.