944909 - Assertion failure: "All flexible lengths should've been resolved" with tall <video>

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: mItems[i].IsFrozen() (All flexible lengths should've been resolved), at layout/generic/nsFlexContainerFrame.cpp:1567

Comment 1

[Daniel Holbert [:dholbert]]

I can reproduce on linux, too.

Right before the fatal assertion-failure, I get:
###!!! ASSERTION: Bogus width: 'mComputedWidth >= 0', file /scratch/work/builds/mozilla-central/mozilla-central.12-12-07.08-38/mozilla/layout/generic/nsHTMLReflowState.cpp, line 2122

So, <video> has an aspect-ratio that it maintains, and I'm betting the huge "min-height" is pushing the mComputedWidth to be so large that it overflows into negative territory. And that confuses flex layout, because it's expecting things to have nonnegative sizes.

We probably should just soften this assertion to be non-fatal.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> We probably should just soften this assertion to be non-fatal.

actually, no - this assertion isn't directly about sizing being wrong; it checks an invariant to be sure we've decided on a size for each flex item ("freezed" them) before moving on with the rest of layout.

So, I think we should keep this assertion as-is, and we need to tweak the code before it to make it freeze flex items more robustly, even if we have bogus negative sizes.

Comment 3

[Daniel Holbert [:dholbert]]

Actually, I think we should do something in between comment 1 and comment 2.

To really "fix" the resolve flexible widths logic to be robust against integer overflow (per comment 2), we'd need to use NSCoordSaturating Add/Subtract (and probably multiply/divide) all over the place, and add additional special cases for values that end up at nscoord_MAX.  This would make things less readable and understandable, with the only upside being that we get the "right" behavior for absurdly huge content.

Rather than doing that, I think we should soften the assertion, and then forcibly freeze items when the assertion fails, to make sure we end up satisfying the asserted condition (for the benefit of later code that depends on it).

(And ideally, we can eventually set the assertion-severity based on bug 765861's "sane sizes" flag)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch v1: soften assertion & freeze any unfrozen items at their hypothetical main sizes

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8351036 [details] [diff] [review]
patch v1: soften assertion & freeze any unfrozen items at their hypothetical main sizes

>+++ b/layout/generic/crashtests/crashtests.list
>+asserts(4-8) load 944909-1.html

NOTE: The "asserts" annotation is because the testcase fails 4 assertions during reflow:
 - Two assertion-failures for "Bogus width: 'mComputedWidth >= 0', in nsHTMLReflowState.cpp*
 - One assertion-failure for the assertion in this patch.
 - And then one more instance of that first assertion.

I also used "4-8" instead of "4" in the annotation because my Try run had one log where we apparently went through reflow twice, and repeated all of those assertions. That's the one orange log here:
 https://tbpl.mozilla.org/?tree=Try&rev=8c5d29ef5f56

That didn't happen again on retriggers, but it apparently happens sometimes, so "asserts(4-8)" should cover it.

(BTW: I think we end up with a negative computed width in the <video> element's reflow state because we have (a) nscoord_MAX computed height and (b) a default 2:1 aspect ratio on <video> elements, and so we end up with a computed width of 2 * nscoord_MAX, which overflows to INT_MIN, IIRC)

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8351036 [details] [diff] [review]
patch v1: soften assertion & freeze any unfrozen items at their hypothetical main sizes

I think we should keep the code as-is -- just change MOZ_ASSERT to
NS_ASSERTION.  I don't think we should add code to non-debug
builds to deal with nscoord overflow unless we have good reasons
to do so and I don't see that here.

Comment 7

[Daniel Holbert [:dholbert]]

That's what I initially thought, too, but in this case, the assertion isn't just dealing with nscoord overflow -- it's ensuring that we satisfy an invariant that the rest of flex layout kind of depends on (and has other assertions about).  See comment 2.

(Nothing particularly dangerous happens if we break this invariant, though -- we'll just fail a few more assertions down the line, and we may end up using intermediate values as the "final" main size for the flex items in question. So maybe making this assertion and the subsequent invariant-checking assertions NS_ASSERTION is ok.)

Comment 8

[Mats Palmgren (inactive)]

Attachment: Alternative fix

I see.  So how about something like this instead?

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8354350 [details] [diff] [review]
Alternative fix

Ah, nice! I like the idea -- only adding (a little) extra code in the "we think things have gone wrong" case.

However, I don't think the logic is quite right yet -- in particular, totalViolation is *allowed* to be nonzero at the end of the final loop iteration -- that's a normal occurrence. 

e.g. in a testcase with only flex item, with a large min-width, like...
  <div style="display:flex; width: 100px">
    <div style="min-width: 500px"></div>
  </div>
...we should run the loop exactly once and end up with totalViolation = 400px. This will make us freeze the item at its min-size, and restart the loop (and then we'll break out of the loop because we'll fail the loop-condition.) The new clause in your patch would break things in that case, by clearing totalViolation before we've had a chance to resolve it.

So -- the invariant here is *not* that totalViolation is 0 at the end, but instead that everything should be frozen by the end of the last loop iteration (and hence, totalViolation *would* end up being 0 in any *subsequent* iterations, if we added them).

I can think of two ways to change your patch to check this invariant:
 (1) Add a little bookkeeping to tell ourselves whether anything will be left unfrozen after our FreezeOrRestoreEachFlexibleSize call, and trigger something like your error-clause (with an additional FreezeOrRestoreEachFlexibleSize call inside it) if we're about to end the final loop iteration with anything unfrozen.

...OR...

 (2) Add one extra iteration to the loop (so, let iterationCounter hit mItems.Length() before ending the loop), and run your clause on *that* final iteration.
    (this will add a little overhead in cases where we have a violation on the final loop iteration)

I think I prefer (1), though I don't have the exact type of bookkeeping crystallized in my head yet.

I'd be happy to give that a try, but I'm also happy to review if you're wanting to keep looking at it -- let me know either way.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] (mostly away until Jan 2) from comment #9)
> I think I prefer (1), though I don't have the exact type of bookkeeping
> crystallized in my head yet.

(The simplest way to do this would probably be for FreezeOrRestoreEachFlexibleSize() to just return the number of not-yet-frozen items, since it's already iterating over all the items and checking whether they're frozen).

Comment 11

[Mats Palmgren (inactive)]

Attachment: Alternative fix

I don't see how my last patch would break anything (in terms of layout),
since we know we're not doing another iteration... maybe I'm missing
something.  Anyway, it *does* trigger a false positive assertion for
the case you mention.

Here's a refined patch that scoops up unfrozen items and freeze them
as we go, in the final iteration only.  I don't see the purpose of
doing another iteration *after* this - can you explain?

I'll leave the rest to you, I was just thinking out loud to see if it
would be possible to integrate it into the final iteration rather than
doing an additional loop over the items.

Comment 12

[Daniel Holbert [:dholbert]]

Oh, you're right -- there wouldn't have been any layout issues.  I was thinking that the "item.SetMainSize(item.GetMainMinSize());" call happened *inside* the FreezeOrRestoreEachFlexibleSize() call (and so your totalViolation clearing would prevent it from happening), but I was misremembering.  Sorry about that.

So, the only problem with the previous patch would be the false positive assertion that you mentioned.

In any case, I like the new patch better.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8354922 [details] [diff] [review]
Alternative fix

r=me with one nit:

>+      } else if (MOZ_UNLIKELY(aFinalIteration)) {
>+        NS_ERROR("Final iteration still has unfrozen items, this shouldn't"
>+                 " happen unless there was nscoord under/overflow.");
>+        item.Freeze();

Please add something like this comment from my original patch:
      // XXXdholbert If & when bug 765861 is fixed, we should upgrade this
      // assertion to be fatal except in documents with enormous lengths.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #11)
> I'll leave the rest to you

(sorry, just remembered this part :))

In light of that, I pushed a Try run, with a bonus patch to address the review comment from Comment 13, and with the crashtest included:
  https://tbpl.mozilla.org/?tree=Try&rev=9ebef0820c51

If that succeeds, I'll land this (with the main patch attributed to mats, with r=me) tomorrow.

Comment 15

[Daniel Holbert [:dholbert]]

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d95b4c617ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/77dc30ebae1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1499ee53b241

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3d95b4c617ca
https://hg.mozilla.org/mozilla-central/rev/77dc30ebae1e
https://hg.mozilla.org/mozilla-central/rev/1499ee53b241

Comment 17

[Paul Silaghi, QA [:pauly]]

I still get:"[4024] ###!!! ASSERTION: Bogus width: 'ComputedWidth() >= 0', file c:/builds/moz
2_slave/m-cen-w32-d-000000000000000000/build/layout/generic/nsHTMLReflowState.cp
p, line 2122" in the latest Nightly 2014-01-09-mozilla-central-debug, Win 7 x64

Thoughts?

Comment 18

[Daniel Holbert [:dholbert]]

That's known (see comment 5) and can be triggered by just
 data:text/html,<video style="min-height: 8041185496px;"></video>

(I think we end up with nscoord_MAX height, and we set the width to try to maintain the intrinsic aspect ratio, which gives us a width of 2 * nscoord_MAX, which is negative. Not sure how much it matters; we probably just end up with broken layout, which is fine.)

In any case, that's a different issue from this bug. Mind filing a followup?

Comment 19

[Paul Silaghi, QA [:pauly]]

(In reply to Daniel Holbert [:dholbert] from comment #18)
> In any case, that's a different issue from this bug. Mind filing a followup?
Done - bug 958085

The "Assertion failure: mItems[i].IsFrozen()" is gone now.
Verified fixed Nightly 2014-01-09-mozilla-central-debug, Mac OS X 10.8.5

Comment 20

[Daniel Holbert [:dholbert]]

Thanks!