1227493 - "ASSERTION: dirtyness out of sync" in table layout with huge sizes

Status: NEW

(Core :: Layout: Tables, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: dirtyness out of sync: '(mMinISize == NS_INTRINSIC_WIDTH_UNKNOWN) == (mPrefISize == NS_INTRINSIC_WIDTH_UNKNOWN)', file layout/tables/BasicTableLayoutStrategy.cpp, line 545

Comment 1

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

Attachment: Crashtest

Comment 2

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

Attachment: Use a value outside the valid nscoord range for NS_INTRINSIC_WIDTH_UNKNOWN

I tested locally that without this patch, the crashtest in the previous
patch fails with an assertion count of 6, and this patch does in fact
change it to 4.

Comment 3

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

The assertion in the bug summary is trivial to fix, and I think it's an improvement to fix it this way.  But this fix doesn't fix the other 4 assertions with the testcase.

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 8691601 [details] [diff] [review]
Use a value outside the valid nscoord range for NS_INTRINSIC_WIDTH_UNKNOWN

This just wallpapers the problem IMO.  Normally I don't care much
about these nscoord overflow assertions but in this case I do think
we should address the root cause, because returning a negative value
from GetMinISize/GetPrefISize seems bad.

Comment 5

[Mats Palmgren (inactive)]

Attachment: wip

The error occurs in BasicTableLayoutStrategy::ComputeIntrinsicISizes
which overflows |min| and assigns a negative value to |mMinISize|.

This is one way fixing it - just let the values overflow and
then clamp negative values at the end.

Alternative 2:
instead of clamping to zero, we could replace the std::max with
a "best-effort saturation" like so:
    mMinISize = min < 0 ? nscoord_MAX : min;

Alternative 3:
make all the arithmetic here use Saturating* functions

Comment 6

[Mats Palmgren (inactive)]

(Personally I don't think 3 is worth it - it's slow and makes the code less readable.)

Comment 7

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

Comment on attachment 8691601 [details] [diff] [review]
Use a value outside the valid nscoord range for NS_INTRINSIC_WIDTH_UNKNOWN

I know that we should still look into the problem of negative intrinsic width, and I said that above.

However, I think it's better for NS_INTRINSIC_WIDTH_UNKNOWN to be something we're less likely to return as an intrinsic width because it means one problem is less likely to compound into multiple problems, so I think we should still do this.

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8691601 [details] [diff] [review]
Use a value outside the valid nscoord range for NS_INTRINSIC_WIDTH_UNKNOWN

I'm not sure I agree that making it less likely to see a problem is
an improvement in this particular case.  But sure, if you insist...

r=mats with the following fixed (as needed):

>+static_assert(nscoord(NS_INTRINSIC_WIDTH_UNKNOWN) < nscoord(nscoord_MIN),
>+              "NS_INTRINSIC_WIDTH_UNKNOWN should be less than nscoord_MIN");

This fails if the nscoord type is float, right?

Comment 9

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

Comment on attachment 8691934 [details] [diff] [review]
wip

This seems reasonable, except that here:

>+    mMinISize = std::max(0, min);
>+    mPrefISize = std::max(0, pref);
>+    mPrefISizePctExpand = std::max(0, pref_pct_expand);

I think you probably also want to std::min() with nscoord_MAX.



Do you want to finish this second patch up, or should I?

Comment 10

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

(In reply to Mats Palmgren (:mats) from comment #8)
> >+static_assert(nscoord(NS_INTRINSIC_WIDTH_UNKNOWN) < nscoord(nscoord_MIN),
> >+              "NS_INTRINSIC_WIDTH_UNKNOWN should be less than nscoord_MIN");
> 
> This fails if the nscoord type is float, right?

Sure.  If anyone ever actually wants to switch to float, they can deal with it then, but the static_assert will make it easy to spot.  I don't think switching to float makes sense, and I don't think we should worry about making code work with it now.

Comment 11

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #9)
> I think you probably also want to std::min() with nscoord_MAX.

Sure.  We could add a NSClampSize specifically for nscoord sizes
(that should not be negative), like I suggested for Alt. 2 above -
if the value is negative we assume it's due to an overflow and
return nscoord_MAX in that case.

> Do you want to finish this second patch up, or should I?

It's probably faster if you take it from here and pick the alternative
you think is best.

(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #10)
> I don't think switching to float makes sense, and I don't think we
> should worry about making code work with it now.

Fair enough. I think we should probably just remove the NS_COORD_IS_FLOAT
flag then, if we're not maintaining it.

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70ed5feb6d47&group_state=expanded

(And I'll try to finish up the other patch tomorrow.)

Comment 13

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

This approach still leaves the 1 assertion:

Table inline-size is less than the sume of its columns' min inline-sizes.