Closed Bug 1384266 Opened 7 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Version:
52 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox-esr68 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: jkratzer, Assigned: bradwerth)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [fuzzblocker])

Attachments

(4 files, 3 obsolete files)

trigger.html
(deleted), text/html
Details
Bug 1384266 Part 1: Make FlexItem::GetBaselineOffsetFromOuterCrossEdge use logical, not physical calculations.
(deleted), text/x-phabricator-request
Details
Bug 1384266 Part 2: Add a test of baseline in flex sideways-rl container.
(deleted), text/x-phabricator-request
Details
screenshot of second flex container in new testcase, after I've made it a bit wider (80px)
(deleted), image/png
Details
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
Attached file trigger.html (deleted) —
Keywords: testcase-wanted → testcase
Priority: -- → P3
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
Blocks: 1221524
Has Regression Range: --- → yes
status-firefox56: --- → wontfix
status-firefox58: affected → fix-optional
status-firefox-esr52: --- → wontfix
Version: unspecified → 52 Branch
Assignee: nobody → bwerth
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.
Assignee: bwerth → nobody
Depends on: 1174003
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.
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)
status-firefox59: ? → wontfix
status-firefox60: --- → ?
Summary: 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 → 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
Attachment #8916824 - Attachment is obsolete: true
Attachment #8918488 - Attachment is obsolete: true
Attachment #8918489 - Attachment is obsolete: true

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?

Flags: needinfo?(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.

Flags: needinfo?(dholbert)

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).

Pushed by bwerth@mozilla.com: 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

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

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox71: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Assignee: nobody → bwerth
No longer blocks: 1221524
Regressed by: 1221524
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: