1223568 - Flex hit MOZ_CRASH(Unexpected justify-content value)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Hit MOZ_CRASH(Unexpected justify-content value) at layout/generic/nsFlexContainerFrame.cpp:2537

Looks like the switch in MainAxisPositionTracker::MainAxisPositionTracker wasn't expecting NS_STYLE_JUSTIFY_STRETCH. This code was all added in
https://hg.mozilla.org/mozilla-central/rev/076d87bf30d0.

(Should NS_STYLE_JUSTIFY_* and the other things in nsStyleConsts.h be enum classes, so they can have compiler-checked exhaustive switches?)

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

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

(In reply to Jesse Ruderman from comment #0)
> (Should NS_STYLE_JUSTIFY_* and the other things in nsStyleConsts.h be enum
> classes, so they can have compiler-checked exhaustive switches?)

I filed bug 1223653 on starting this, and if it's a success we can file more to continue.

Comment 3

[Daniel Holbert [:dholbert]]

brief history:

* The reason "stretch" isn't handled here is that our original "justify-content" impl didn't include "stretch" as an option -- it was based off of flexbox's "justify-content" property-definition, here:
 https://drafts.csswg.org/css-flexbox-1/#justify-content-property

* Mats recently landed support for new "justify-content" values, including "stretch", in bug 1151214.
* That landing was the most proximal cause of this -- it was known to be not-entirely-compatible with flexbox, per the existence of followup bug 1207698. (Bug 1151214 included shims to avoid aborts, but we must've missed one.)

Comment 4

[Daniel Holbert [:dholbert]]

Actually, it's not the "stretch" that's causing us trouble here; it's stretch *combined with* a fallback value.

Minimal testcase that triggers the abort:
  data:text/html,<div style="display:flex; justify-content: stretch end">a

Comment 5

[Daniel Holbert [:dholbert]]

The problem is, this fixup code is failing to activate (due to the fallback value messing up the equality comparison):
> 1742   switch (aDisplay->mDisplay) {
> 1743     case NS_STYLE_DISPLAY_FLEX:
> 1744     case NS_STYLE_DISPLAY_INLINE_FLEX:
> 1745       // XXX maybe map 'auto' too? (ISSUE 8 in the spec)
> 1746       // https://drafts.csswg.org/css-align-3/#content-distribution
> 1747       if (mJustifyContent == NS_STYLE_JUSTIFY_STRETCH) {
> 1748         return NS_STYLE_JUSTIFY_FLEX_START;
> 1749       }
> 1750       break;

https://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleStruct.cpp?rev=5ae04c1c341c#1739

(also: in comment 3, when I talked about bug 1151214, I really meant bug 1176782)

Comment 6

[Daniel Holbert [:dholbert]]

I'm actually not sure the special-case in comment 5 should be there.  It seems to be assuming that "stretch" should be treated as "flex-start" for flexbox, but it's not clear to me that this is true.

The css-align spec says stretch *falls back* to 'flex-start', but the fallback alignment only matters when there's negative excess space.  In cases where there's *positive* excess space (even after the flexbox algorithm has run), I think the spec currently requires that we honor "stretch".

(Such cases are a bit edge-casey; the only scenario I can think of is with fractional "flex-grow" values, which limit how much growing the flexbox algorithm will do. E.g.
   <div style="display:flex; width:150px">
     <div style="flex: 0.1 0 50px"></div>
   </div>
In this case, the flex item's "0.1" flex-grow value means it only gets 10% of the available free space (100px), so it'll end up getting +10px = 60px and the remaining of the free space (90px) will go unused.  But if we add "justify-content:stretch" to the container, I think the spec requires all of the remaining free space (90px) to be distributed in a second pass, as long as we don't violate max-width values.)

Anyway; we can sort this out in bug 1207698 (and I should probably post to www-style to clarify the desired behavior in this scenario).  For now, I'll go with the spirit of the existing code, to work around this crash.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This broadens the code quoted in comment 5.

This isn't quite correct (if there's a fallback value, really we should use the fallback value instead of 'flex-start') -- but I'm not sure this special-case itself is really correct, per comment 6.

So, this patch just extends the current code to treat "stretch [anything]" as "flex-start" for the moment, to avoid this crash, and then we can sort out how to actually handle "stretch" & its fallback values in bug 1207698.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (now w/ more accurate commit message)

(Sorry, previous fix had a misleading commit message from when I thought the problem was something different. Updated commit message here.)

Comment 9

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> The css-align spec says stretch *falls back* to 'flex-start',

That text doesn't apply to 'justify-content' for Flexbox containers because:
https://drafts.csswg.org/css-align-3/#propdef-justify-content
"The 'justify-content' property ... 'stretch' computes to 'flex-start'."
E.g. for 'stretch end' we can't make the computed value 'flex-start end'
because that's an illegal value (can't have two <content-position>).

The only other interpretation I see is that it's only literally 'stretch'
that computes to 'flex-start', and that 'stretch <fallback>' computes
to itself and "behaves as" <fallback> in layout (for 'justify-content').
But I don't see any support for that interpretation in the spec.
https://drafts.csswg.org/css-align-3/#propdef-justify-content
It seems to me what the editor is trying to say is that 'stretch' does
not apply at all to 'justify-content' (since flexing is controlled by
'flex').  Might be worth checking what Chrome Canary does though and/or
asking on www-style@ what the computed value is supposed to be for
'stretch end safe' for example.

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8686181 [details] [diff] [review]
fix v2 (now w/ more accurate commit message)

I think this might be the correct fix actually.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #9)
> That text doesn't apply to 'justify-content' for Flexbox containers because:
> https://drafts.csswg.org/css-align-3/#propdef-justify-content
> "The 'justify-content' property ... 'stretch' computes to 'flex-start'."

Yeah, I realized that after the fact. (right after I posted to www-style, d'oh. :))

> Might be worth checking what Chrome Canary does though and/or
> asking on www-style@ what the computed value is supposed to be for
> 'stretch end safe' for example.

Yup, just asked on www-style about "stretch end" (and mentioned safe|true in a parenthetical):
https://lists.w3.org/Archives/Public/www-style/2015Nov/0214.html

Thanks for review! I'll drop the XXXdholbert comment when landing, too, since you & I agree the new code is likely-correct (disagreeing with me-from-comment-6 who hadn't yet found the flexbox special case in the spec).

Comment 12

[Daniel Holbert [:dholbert]]

Actually, rather than dropping my XXX comment, I replaced it with an explanation so I don't get confused about this in the future. :)
+      // For flex containers, css-align-3 says the justify-content value
+      // "'stretch' computes to 'flex-start'."
+      // https://drafts.csswg.org/css-align-3/#propdef-justify-content

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5abdb4af2442

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93f6fc8cc5d0

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93f6fc8cc5d0