1239036 - Crash when inspecting computed style of "grid-template-rows". In debug builds: "Assertion failure: aTrackList.mLineNameLists.Length() == aTrackList.mMinTrackSizingFunctions.Length() + 1 (Unexpected number of line name lists)"

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (warning: crashes Firefox Nightly)

STR:
 1. Load attached testcase.

ACTUAL RESULTS:
 Crash.

EXPECTED RESULTS:
 No crash; lime square should render.


(Note: I originally tripped over this when inspecting attachment 8706032 [details] in devtools. Loading the "computed style" devtools pane for that attachment caused my content process to crash.)


Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=10c8b5b877b9e7bf11c6a472338496a7aa0d3a10&tochange=91d546e91e8a2ea3d0f56f15582b8dcd7168a8d2

Comment 1

[Daniel Holbert [:dholbert]]

In a debug build, we fail this assert as well:
Assertion failure: aTrackList.mLineNameLists.Length() == aTrackList.mMinTrackSizingFunctions.Length() + 1 (Unexpected number of line name lists), at /scratch/work/builds/mozilla-inbound/mozilla/layout/style/nsComputedDOMStyle.cpp:2502

Both Length()s there are 0. Also, 'numSizes' is initially 0, but we update it to 1 at line 2487, in the code that was added for bug 1234644 (which makes us skip the early return clause at line 2493):
2486   if (aTrackSizes) {
2487     numSizes = aTrackSizes->Length();
2488     MOZ_ASSERT(numSizes > 0 ||
2489                (aTrackList.HasRepeatAuto() && !aTrackList.mIsAutoFill),
2490                "only 'auto-fit' can result in zero tracks");
2491   }
2492   // An empty <track-list> is represented as "none" in syntax.
2493   if (numSizes == 0) {
2494     RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
2495     val->SetIdent(eCSSKeyword_none);
2496     return val.forget();
}
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp?rev=ecafedaedd09#2486

Marking as regression from bug 1234644.

Comment 2

[Daniel Holbert [:dholbert]]

(The point of comment 1 is: we used to take the early return, avoiding the assert & crash, but now we do not.)

Crash report in opt build: bp-ba56cc51-814f-42df-b179-ba7bf2160112

Comment 3

[Mats Palmgren (inactive)]

In a DEBUG build:

2500      MOZ_ASSERT(aTrackList.mLineNameLists.Length() ==
2501                   aTrackList.mMinTrackSizingFunctions.Length() + 1,
2502                 "Unexpected number of line name lists");
2503
2504      RefPtr<nsDOMCSSValueList> valueList = GetROCSSValueList(false);
(gdb) p aTrackList.mLineNameLists.Length()
$1 = 0
(gdb) p aTrackList.mMinTrackSizingFunctions.Length()
$2 = 0
(gdb) p numSizes
$3 = 1

Hmm, right, 'numSizes' is 1 is because there is one implicit row there.
So this assertions really only holds for the display:none case where we use
the specified values only. (the 'grid-template-columns' isn't necessary in
the test, accessing gridTemplateRows() with only implicit rows is enough)

After fixing that, we crash trying to access aTrackList.mLineNameLists[0] ...
So this is a design flaw, we need to know how many tracks are implicit
at the beginning of the given array, and NOT try to access line names
for those.

I think the solution here is to add
uint32_t nsGridContainerFrame::ExplicitGridOffsetCol() const
uint32_t nsGridContainerFrame::ExplicitGridOffsetRow() const
and then iterate that many entries from the array in a separate loop
(not accessing style data) before the existing one, and then offset
the style data array indecies by that much in the existing loop.

Comment 4

[Mats Palmgren (inactive)]

Actually, we also need to know the number of explicit tracks so we can differentiate
between repeated tracks and implicit tracks at the end of the given array of values.

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

Comment 6

[Mats Palmgren (inactive)]

Attachment: tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e2e5a676baf

Comment 7

[Mats Palmgren (inactive)]

Attachment: wdiff version of the fix for easier reviewing

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8707544 [details] [diff] [review]
wdiff version of the fix for easier reviewing

Review of attachment 8707544 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the following addressed:

::: inbound.ceccb1580caa/layout/style/nsComputedDOMStyle.cpp
@@ +2492,5 @@
> +    MOZ_ASSERT(isAutoFill ?
> +               numSizes >= aTrackList.mMinTrackSizingFunctions.Length() :
> +               numSizes + 1 >= aTrackList.mMinTrackSizingFunctions.Length(),
> +               "expected at least all explicit tracks or, for 'auto-fit', "
> +               "one less at most");

Two things:
 (1) "at most" is ambiguous here (particularly in a sentence with "at least...at most" phrasing).  Consider replacing the end bit (after "or") with "(or possibly one less, if there's an 'auto-fit' track, since that can collapse away)"

 (2) Consider adjusting the second comparison to bump the "1" to the other side as "- 1". This makes it more clearly match the textual description ("one less").

@@ +2508,1 @@
>                 aTrackList.mMinTrackSizingFunctions.Length() + 1,

What's the reason for adding "aTrackSizes ||" to this assertion here?  Shouldn't the assertion still hold, as it existed before?  It seemed like a (valid) internal-consistency-check of some style-system data. (And adding "aTrackSizes ||" to the condition strictly weakens this assertion.)

@@ +2573,5 @@
> +        val->SetAppUnits((*aTrackSizes)[i + aNumLeadingImplicitTracks]);
> +        valueList->AppendCSSValue(val.forget());
> +      }
> +    }
> +    

whitespace on blank line here.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8707544 [details] [diff] [review]
wdiff version of the fix for easier reviewing

[/me shakes fist at splinter, providing 1 line of patch-context]

Here's more patch-context for my second review comment above (what's the reason for adding "aTrackSizes ||"):

>   // Delimiting N (specified) tracks requires N+1 lines:
>   // one before each track, plus one at the very end.
>-  MOZ_ASSERT(aTrackList.mLineNameLists.Length() ==
>+  MOZ_ASSERT(aTrackSizes ||
>+             aTrackList.mLineNameLists.Length() ==
>                aTrackList.mMinTrackSizingFunctions.Length() + 1,
>              "Unexpected number of line name lists");

Comment 10

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
  (1) "at most" is ambiguous here (particularly in a sentence with "at

OK, I'll rephrase that.

>  (2) Consider adjusting the second comparison to bump the "1" to the other
> side as "- 1". This makes it more clearly match the textual description
> ("one less").

aTrackList.mMinTrackSizingFunctions.Length() may be zero at this point
and it's an unsigned type so -1 might cause an underflow and a false
positive assertion.

> What's the reason for adding "aTrackSizes ||" to this assertion here? 

The assertions for the aTrackSizes case are now handled inside
the 'if (aTrackSizes)' chunks.

> Shouldn't the assertion still hold, as it existed before?

Nope, this doesn't hold in the mMinTrackSizingFunctions.Length() == 0
case (in which case aTrackList.mLineNameLists.Length() is also zero).
(see comment 3)

>  It seemed like a (valid) internal-consistency-check of some
> style-system data. 

I guess I could move it up before the early return for numSizes==0
and add "aTrackList.mLineNameLists.Length() == 0 || ..." to it.
I think that should make it hold in all cases.  I guess that makes
the purpose of the assertion clearer at least.

Would you prefer that?

> (And adding
> "aTrackSizes ||" to the condition strictly weakens this assertion.)

True, but the total effects of the added 'if (aTrackSizes)' assertions
are much stricter now.

Comment 11

[Daniel Holbert [:dholbert]]

Maybe better: should we move this assertion down into the "else" clause of the "if (aTrackSizes) {" check that comes right after this point?

That's where we actually depend on this assertion, anyway. (That's the next place we actually *use* mMinTrackSizingFunctions.)  I think that's the code that this assertion was originally meant to go with.

Comment 12

[Daniel Holbert [:dholbert]]

(Sorry, disregard comment 11; I was mixed up & looking at the wrong assertion)

Comment 13

[Daniel Holbert [:dholbert]]

So, a few things:
 (1) I'd rather not move the assertion further up -- it's nice to have it as close to the code that depends on it as possible. (And here, the code that depends on it is the code that indexes into mLineNameLists, in a loop that runs from 0 up to aNumExplicitTracks, I think).

 (2) Really, I think the condition that we're asserting here has drifted apart from *what we actually depend on in this code*. Originally, this assertion sat before a loop that ran from 0 to numSizes, inclusive, and it asserted that we had numSizes+1 line names, which meant we were OK to be indexing into our line-names array in each iteration of this loop.  But in http://hg.mozilla.org/mozilla-central/diff/814ec51c8f03/layout/style/nsComputedDOMStyle.cpp#l2497 , the asserted condition & the loop-range diverged (at least in terms of the variables that they use).

So, ideally I'd like to go back to a state where the assertion is clearly proving that our indexing inside of the loop is valid. This might mean we need two copies of this assertion -- one for the "if (aTrackSizes) {" clause (and its loop) and a separate one for the "else" clause (and its loop), since those loops have different ranges.

Comment 14

[Daniel Holbert [:dholbert]]

For reference, that original formulation of this assertion is here:
 http://hg.mozilla.org/mozilla-central/diff/96f8a98c9228/layout/style/nsComputedDOMStyle.cpp#l1.81

(We index into mLineNameLists[i], and then break after doing that for i == numSizes. So, the assertion is proving that indexing across this exact range is covers the full scope of our line names [no more, no less].)

Comment 15

[Daniel Holbert [:dholbert]]

If you'd like to keep the assertion as-is & spin off the assertion-refactoring into a followup patch/bug, that's fine by me, too -- it's a bit distinct from the main thrust of the main patch here.

Though in that case, I'd maybe suggest adjusting the current patch to replace "aTrackSizes ||" with "(aTrackSizes && aTrackList.mLineNameLists.Length() == 0) ||" to make it a little clearer that this exception is purely for the only-implicit-tracks-exist scenario.

Comment 16

[Daniel Holbert [:dholbert]]

So then it would look something like:
  // Delimiting N (specified) tracks requires N+1 lines:
  // one before each track, plus one at the very end.
  // (Though we might also get here with *only* implicit tracks,
  // in which case mLineNameLists is empty.)
  MOZ_ASSERT((aTrackSizes && aTrackList.mLineNameLists.IsEmpty()) ||
             aTrackList.mLineNameLists.Length() ==
               aTrackList.mMinTrackSizingFunctions.Length() + 1,
             "Unexpected number of line name lists");

I still think it should be adjusted further eventually, per comment 13; but I'd be happy leaving it with something like this for the time being.

Comment 17

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> So, ideally I'd like to go back to a state where the assertion is clearly
> proving that our indexing inside of the loop is valid.

If that's its only purpose, then I think we should remove it because nsTArray
already asserts that the index is valid on every access.

nsRuleNode already asserts that the style data is correct here:
http://hg.mozilla.org/mozilla-central/annotate/ad1f85f172b7/layout/style/nsRuleNode.cpp#l7891
and the style data is readonly(*) (enforced by 'const' as far as I know)
after that point so I'm reluctant to add assertions anywhere else
about style data alone.  I'd rather have all those assertions in one
place, with comments ideally, rather than spread out in a lot of places.

I think the additional assertions here should only check the data
that comes from layout and that's already covered by:
    MOZ_ASSERT(isAutoFill ?
               numSizes >= aTrackList.mMinTrackSizingFunctions.Length() :
               numSizes + 1 >= aTrackList.mMinTrackSizingFunctions.Length(),
               "expected at least all explicit tracks (or possibly one less, if "
               "there's an 'auto-fit' track, since that can collapse away)");
and
    MOZ_ASSERT(numSizes > 0 &&
               numSizes >= aNumLeadingImplicitTracks + aNumExplicitTracks);

(*) well, OK, there's the ApplyStyleFixups abomination, but let's pretend
that doesn't exist ;-)

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #17)
> If that's its only purpose, then I think we should remove it because nsTArray
> already asserts that the index is valid on every access.

That nsTArray assertion does effectively assert that we don't index past the end of the array, but it doesn't ensure that we actually *reach the end* & include all of the lines that we should be including. (admittedly less of a concern)

> nsRuleNode already asserts that the style data is correct here:

It asserts it in terms of other style data, but not in terms of these "aNumExplicitTracks" / "numSizes" variables that we actually use as our loop indexing limits.

> I think the additional assertions here should only check the data
> that comes from layout and that's already covered by:

OK... I haven't looked too closely, but I think these assertions do effectively verify that we get as far as the end.

So, r=me stands with the assertion removed, if you like.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ee8ff172b58e
https://hg.mozilla.org/integration/mozilla-inbound/rev/08f818df4464

Comment 20

[Mats Palmgren (inactive)]

OK, I used aNumExplicitTracks in the assertion instead which makes it more strict:

    DebugOnly<bool> isAutoFill =
      aTrackList.HasRepeatAuto() && aTrackList.mIsAutoFill;
    DebugOnly<bool> isAutoFit =
      aTrackList.HasRepeatAuto() && !aTrackList.mIsAutoFill;
    MOZ_ASSERT(aNumExplicitTracks == numSizes ||
               (isAutoFill && aNumExplicitTracks >= numSizes) ||
               (isAutoFit && aNumExplicitTracks + 1 >= numSizes),
               "expected all explicit tracks (or possibly one less, if there's "
               "an 'auto-fit' track, since that can collapse away)");

(with numSizes here == aTrackList.mMinTrackSizingFunctions.Length())

I think this is the most precise it can be.  (Since I only tweaked the
assertion I didn't bother with yet a formal review, but feel free
to have a look at what I landed if you want.)

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee8ff172b58e
https://hg.mozilla.org/mozilla-central/rev/08f818df4464