1169420 - "Assertion failure: GetMarginComponentForSide(side) == 0 (Someone else tried to resolve our auto margin)" with flex, writing-mode, vertical text, rtl

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: GetMarginComponentForSide(side) == 0 (Someone else tried to resolve our auto margin), at layout/generic/nsFlexContainerFrame.cpp:1577

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (simpler)

Here's a testcase with just "margin-top" and "margin-bottom" instead of -moz-margin-start|end. This triggers the assertion for me as well.

Comment 3

[Daniel Holbert [:dholbert]]

I'll just focus on testcase 2, but this all applies to testcase 1 as well.  At the point where we crash, we have: this nsMargin on our FlexItem:
  { 
    top = 0,
    right = 0,
    bottom = 4500,
    left = 0
  }

This comes directly from the nsHTMLReflowState::mComputedMargin for this frame. The values here are wrong; as shown in the testcase source, "margin-top" is what has a fixed nonzero length.

I think the buggy code in question is nsCSSOffsetState::ComputeMargin, which does this:
> 2614     LogicalMargin m(mWritingMode);
[...]
> 2628     m.Top(mWritingMode) = nsLayoutUtils::
> 2629       ComputeCBDependentValue(verticalPercentBasis,
> 2630                               styleMargin->mMargin.GetTop());

Looks like we're setting the "logical margin-top", m.Top, based on the actual physical StyleMargin()->mMargin.GetTop() value. That's incorrect.  (Same goes for all the other sides, too, I think.)

MXR Link:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=3d9012207555#2600

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (visual)

Here's a testcase that visually demonstrates this bug, with just blocks.

There's some vertical text with "margin-bottom", which we're incorrectly interpreting as "margin-top".

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> There's some vertical text with "margin-bottom", which we're incorrectly
> interpreting as "margin-top".

(Via the code in comment 3, I suspect.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 0: Soften MOZ_ASSERT to NS_ASSERTION

Not sure if this is as simple as just adjusting the code in comment 3; I'm hesitant to touch it just to fix this bug, lest I break something. (There are known issues with vertical writing modes & rtl, per bug 1131451.)

For now, here's a "part 0" patch to soften the assertion, so that it doesn't force Jesse's fuzzers to abort. (The assertion doesn't *have* to be fatal -- it's not like a safety thing.  It's just a sign that something odd is going on, and we may be doing our margin computations incorrectly. So, no harm in softening it while this bug is open.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 0: Soften MOZ_ASSERT to NS_ASSERTION

(now with a commit message)

Comment 8

[Daniel Holbert [:dholbert]]

(Looks like bug 1147834 is logicalizing nsHTMLReflowState, too -- and landing soon -- I wonder if it will help here?)

Comment 9

[Simon Montagu :smontagu]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> > 2628     m.Top(mWritingMode) = nsLayoutUtils::
> > 2629       ComputeCBDependentValue(verticalPercentBasis,
> > 2630                               styleMargin->mMargin.GetTop());
> 
> Looks like we're setting the "logical margin-top", m.Top, based on the
> actual physical StyleMargin()->mMargin.GetTop() value. That's incorrect. 
> (Same goes for all the other sides, too, I think.)

No, m.Top is (or should be) the physical margin-top, so that code is consistent. One of my patches in bug 1147834 changes ComputeMargin() to use logical coordinates but doesn't fix this.

The good news is that bug 1167930 does seem to fix all the testcases attached here

Comment 10

[Daniel Holbert [:dholbert]]

Ah, thanks for clarifying. We must've been messing something up slightly later when setting up the (physical) computed margin on the nsHTMLReflowState then.

Anyway, I can confirm that this is WORKSFORME (no assertions in testcases 1/2, & no extra space in testcase 3) in my mozilla-inbound debug build that I'm running right now. So, I'm closing this as FIXED by bug 1167930, per comment 9. (& there's no need to land my assertion-softening patch after all)

Comment 11

[Daniel Holbert [:dholbert]]

Landed testcases 1 and 2 as crashtests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d29fb828423

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d29fb828423