1384266 - Assertion failure: sideToMeasureFrom == eSideBottom (We already checked that we're dealing with a vertical axis, and we're not using the top side, so that only leaves the bottom...), at src/layout/generic/nsFlexContainerFrame.cpp:1987

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Testcase found while fuzzing mozilla-central rev 20170721-098c638791f5.

Assertion failure: !aAxisTracker.IsCrossAxisHorizontal() (Only expecting to be doing baseline computations when the cross axis is vertical), at /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:1907

ASAN:DEADLYSIGNAL
=================================================================
==3989==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7febf7e3f56f bp 0x7ffc1f3481d0 sp 0x7ffc1f3481a0 T0)
==3989==The signal is caused by a WRITE memory access.
==3989==Hint: address points to the zero page.
    #0 0x7febf7e3f56e in nsFlexContainerFrame::FlexItem::GetBaselineOffsetFromOuterCrossEdge(AxisEdgeType, nsFlexContainerFrame::FlexboxAxisTracker const&, bool) const /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:1921:3
    #1 0x7febf7e453d0 in nsFlexContainerFrame::FlexLine::ComputeCrossSizeAndBaseline(nsFlexContainerFrame::FlexboxAxisTracker const&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:3077:15
    #2 0x7febf7e499ae in nsFlexContainerFrame::DoFlexLayout(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&, int, int, nsTArray<nsFlexContainerFrame::StrutInfo>&, nsFlexContainerFrame::FlexboxAxisTracker const&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:4199:11
    #3 0x7febf7e492af in nsFlexContainerFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:3995:3
    #4 0x7febf7dd307f in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:979:14
    #5 0x7febf7e3dc4d in nsFlexContainerFrame::MeasureAscentAndHeightForFlexItem(nsFlexContainerFrame::FlexItem&, nsPresContext*, mozilla::ReflowInput&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:1660:3
    #6 0x7febf7e3d394 in nsFlexContainerFrame::MeasureFlexItemContentHeight(nsPresContext*, nsFlexContainerFrame::FlexItem&, bool, mozilla::ReflowInput const&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:1721:5
    #7 0x7febf7e3c619 in nsFlexContainerFrame::ResolveAutoFlexBasisAndMinSize(nsPresContext*, nsFlexContainerFrame::FlexItem&, mozilla::ReflowInput const&, nsFlexContainerFrame::FlexboxAxisTracker const&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:1574:9
    #8 0x7febf7e3b5f7 in nsFlexContainerFrame::GenerateFlexItemForChild(nsPresContext*, nsIFrame*, mozilla::ReflowInput const&, nsFlexContainerFrame::FlexboxAxisTracker const&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:1285:3
    #9 0x7febf7e47698 in nsFlexContainerFrame::GenerateFlexLines(nsPresContext*, mozilla::ReflowInput const&, int, int, nsTArray<nsFlexContainerFrame::StrutInfo> const&, nsFlexContainerFrame::FlexboxAxisTracker const&, nsTArray<nsIFrame*>&, mozilla::LinkedList<nsFlexContainerFrame::FlexLine>&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:3605:14
    #10 0x7febf7e49706 in nsFlexContainerFrame::DoFlexLayout(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&, int, int, nsTArray<nsFlexContainerFrame::StrutInfo>&, nsFlexContainerFrame::FlexboxAxisTracker const&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:4136:3
    #11 0x7febf7e492af in nsFlexContainerFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsFlexContainerFrame.cpp:3995:3
    #12 0x7febf7e2401a 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:935:14
    #13 0x7febf7e23292 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:752:5
    #14 0x7febf7e2401a 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:935:14
    #15 0x7febf7ec28a9 in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:549:3
    #16 0x7febf7ec3b32 in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:661:3
    #17 0x7febf7ec5e67 in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:1037:3
    #18 0x7febf7dd307f in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:979:14
    #19 0x7febf7dd2672 in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:329:7
    #20 0x7febf7c4829b in mozilla::PresShell::DoReflow(nsIFrame*, bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9348:11

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Still reproducible with or without Stylo enabled.

INFO: Last good revision: f9b7b51d141628fcbd672660ae5cae7849c2ca1eINFO: First bad revision: 951f657c539915a28a3c61216bb65bf445d0ce43INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f9b7b51d141628fcbd672660ae5cae7849c2ca1e&tochange=951f657c539915a28a3c61216bb65bf445d0ce43

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1384266 Part 1: Generalize FlexItem::GetBaselineOffsetFromOuterCrossEdge to allow horizontal cross axis with rotated writing modes.

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

Comment 4

[Brad Werth [:bradwerth]]

Comment on attachment 8916824 [details]
Bug 1384266 Part 1: Generalize FlexItem::GetBaselineOffsetFromOuterCrossEdge to allow horizontal cross axis with rotated writing modes.

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

Comment 5

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0acf367cfc7b655d3cdd6e4bd0cc31edf05c16d5

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1384266 Part 2: Attempt some logical-ization of FlexItem size computations, probably incorrectly.

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

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1384266 Part 3: Add a test of baseline in flex sideways-rl container.

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

Comment 8

[Brad Werth [:bradwerth]]

Comment on attachment 8916824 [details]
Bug 1384266 Part 1: Generalize FlexItem::GetBaselineOffsetFromOuterCrossEdge to allow horizontal cross axis with rotated writing modes.

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

Comment 9

[Brad Werth [:bradwerth]]

Solving this correctly will require a lot of fixes to assumptions about writing modes in nsFlexContainerFrame. Bug 1174003 covers that issue. For this bug, attachment 8918488 [details] attempts to do some of the necessary "logicalization" of axes and writing modes to correctly calculate baselines for rotated writing modes. The approach is incomplete at best and possibly just wrong.

Taking myself off the bug. I will see if I can help complete Bug 1174003.

Comment 10

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 12

[Daniel Holbert [:dholbert]]

I landed a patch stack in bug 1174003 which (among other things) generalized this bug's original assertion and made it more correct.  So now this testcase gets past that assertion to a different one a bit later.  --> Updating bug summary.

Comment 13

[Daniel Holbert [:dholbert]]

I'm calling this "wontfix" for 59. Nothing seriously bad happens -- we just get the layout wrong.*  I'm hoping to finish this off soon, but it wouldn't be worth uplifting (and it'd be highly dependent on bug 1174003's patches which are too complex to uplift).

* (this is perhaps a signal that fatal MOZ_ASSERT was a poor choice for this function. If this takes longer than I'm hoping, we might want to adjust to a nonfatal NS_ASSERTION so that this isn't a fuzz blocker)

Comment 14

[Brad Werth [:bradwerth]]

Attachment: Bug 1384266 Part 1: Make FlexItem::GetBaselineOffsetFromOuterCrossEdge use logical, not physical calculations.

Comment 15

[Brad Werth [:bradwerth]]

Attachment: Bug 1384266 Part 2: Add a test of baseline in flex sideways-rl container.

Depends on D45424

Comment 16

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=90c4b2a98888c0cc7ecf81a48e4c14252be41372

Comment 17

[Brad Werth [:bradwerth]]

Daniel, the test added here now checks the anti-parallel case, and I think it may reveal a failure of the flex algorithm. In the new test added by https://phabricator.services.mozilla.com/D45425, the "container reverse" flex container has a yellow single-line item aligned to the last baseline. What that currently does is align it to the baseline nearest the container start side, but the reference case (which is possibly constructed incorrectly) shows that it should align to the end side baseline.

I've read the spec (https://drafts.csswg.org/css-flexbox-1/#change-2016-flex-direction-baseline) and I can't understand its conclusions on this point since it refers to baseline selection amongst items, not within a multi-line item. One additional data point: removing the sideways-lr writing mode shows that our behavior does not match Chrome in this regard, before or after these patches.

I could remove this flex item to land the test, but it wouldn't be as interesting. Is there a larger issue here that should be a blocker for this test as written?

Comment 18

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3f64b28018b5b7e7644851c0b53289f13c27c0a

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Brad Werth [:bradwerth] from comment #17)
I think the reference case is wrong there, and it's also a bit hard to reason about the testcase since (at least on my system) the flex container is only about as wide as two lines of text. If you make the flex container a bit wider, as I just suggested in phab, then it's easier to visualize & reason about what's happening.

Here's what I see:
(1) the yellow and orange items are aligning their last-baselines with each other
(2) collectively, those two items are snapped to the right side (the cross-end side) of the flex container.
This results in the yellow item looking like it's offset by 1 line from the right edge of the flex container.

That is what the code currently intend to do, I believe, and I think it's correct, modulo my "However" below (which is outside the scope of this bug).

However, in fact it seems (2) may be incorrect per spec there.... at one point, I'm pretty sure "baseline" was specced as falling back to flex-start and (either explicitly or implicitly) last-baseline fell back to flex-end, and that's what we currently aim to do. But now the spec says they fall back to start and end which means they're technically not supposed to care about the wrap-reverse cross-axis reversal here. So I think per spec, the group of last-baseline aligned things are always supposed to be aligned to the flex container's block-end side (the left side in this case). Having said that, that's not what we do or what Chrome does, so there may be some webcompat risk from making that change. In any case, that change would be outside the scope of this bug.

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: screenshot of second flex container in new testcase, after I've made it a bit wider (80px)

Here's a screenshot of what I see in the reftest's lower flex container, in a build with both patches, after I've made the flex container a little wider to help visualize the alignment differences.

I believe this is the correct rendering (modulo the flex-end vs. end "however" in my previous comment).

Comment 21

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63ea3c19d6f8ea9a8751d5c7d37f678718b90e2f

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/402bb5b5966b
Part 1: Make FlexItem::GetBaselineOffsetFromOuterCrossEdge use logical, not physical calculations. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/09570c23c56c
Part 2: Add a test of baseline in flex sideways-rl container. r=dholbert

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/402bb5b5966b
https://hg.mozilla.org/mozilla-central/rev/09570c23c56c

Comment 24

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

WPT import in https://github.com/web-platform-tests/wpt/pull/19451.