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

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.

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

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

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

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 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 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?

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

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70ed5feb6d47&group_state=expanded (And I'll try to finish up the other patch tomorrow.)

This approach still leaves the 1 assertion: Table inline-size is less than the sume of its columns' min inline-sizes.