989755 - "Assertion failure: nRowItems % 2 == 1 (expected an odd number of items)" with css grid

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase that requires cycle collection

1. Set
     user_pref("layout.css.grid.enabled", true);
2. Load the testcase
3. Trigger a CC or quit Firefox

Result:
Assertion failure: nRowItems % 2 == 1 (expected an odd number of items), at layout/style/Declaration.cpp:1033

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

Why are we serializing stuff during cycle collection?

Comment 3

[Jesse Ruderman]

Attachment: better testcase

Comment 4

[Jesse Ruderman]

Attachment: stack

Comment 5

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

(In reply to Jesse Ruderman from comment #2)
> Why are we serializing stuff during cycle collection?

The code says:

void
Attr::SetMap(nsDOMAttributeMap *aMap)
{
  if (mAttrMap && !aMap && sInitialized) {
    // We're breaking a relationship with content and not getting a new one,
    // need to locally cache value. GetValue() does that.
    GetValue(mValue);
  }

  mAttrMap = aMap;
}

In other words, your testcase has a live Attr object, which, when disconnected from its content node, needs to know what attribute string it represents.  (Moral of the story -- don't use Attr objects!)

Comment 6

[Simon Sapin (:SimonSapin)]

This is a bug introduced in Bug 983175.

Comment 7

[Simon Sapin (:SimonSapin)]

Attachment: Patch

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8399125 [details] [diff] [review]
Patch

Thanks for the quick fix (and thanks, Jesse, for fuzz-testing this!)

>From: Simon Sapin <[e-mail]>
>Bug 989755: Fix assertion failure on grid-template serialization.

"Fix assertion failure" is a bit opaque about what the change is actually doing.

Maybe something along the lines of: "Refuse to serialize 'grid-template' shorthand a non-initial 'grid-template-areas' value is combined with a 'subgrid' value, since shorthand can't represent that combination"

(As much as possible, commit messages should describe the change, rather than describing the problem, per:
 https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment )

>@@ -1059,16 +1075,22 @@ Declaration::GetValue(nsCSSProperty aProperty, nsAString& aValue,
>+          if (rowsItem->mNext &&
>+              rowsItem->mNext->mValue.GetUnit() == eCSSUnit_Null &&
>+              !rowsItem->mNext->mNext) {
>+            // Avoid a trailing space.
>+            addSpaceSeparator = false;
>+          }
>         }
> 
>         rowsItem = rowsItem->mNext;
>         if (!rowsItem) {
>           break;
>         }
> 
>         if (addSpaceSeparator) {

AFAICT, the new code here is checking whether we're reaching the end-of-content-that-needs-to-be-serialized, right?

If so, it seems like it would be more appropriate to just break here (instead of tweaking addSpaceSeparator and allowing ourselves to continue cycling). In fact, maybe this should just merge into the "break" clause below it?

>diff --git a/layout/style/nsCSSValue.cpp b/layout/style/nsCSSValue.cpp
>index 7727965..bb91273 100644
>--- a/layout/style/nsCSSValue.cpp
>+++ b/layout/style/nsCSSValue.cpp
>@@ -1703,16 +1703,23 @@ AppendGridTemplateToString(const nsCSSValueList* val,
>       aResult.AppendLiteral("(");
>       AppendValueListToString(val->mValue.GetListValue(), aProperty,
>                               aResult, aSerialization);
>       aResult.AppendLiteral(")");
> 
>     } else {
>       // <track-size>
>       val->mValue.AppendToString(aProperty, aResult, aSerialization);
>+      if (!isSubgrid &&
>+          val->mNext &&
>+          val->mNext->mValue.GetUnit() == eCSSUnit_Null &&
>+          !val->mNext->mNext) {
>+        // Avoid a trailing space.
>+        addSpaceSeparator = false;
>+      }
>     }
> 
>     val = val->mNext;
>     if (!val) {
>       break;
>     }

Same here, I think -- looks like this should be "break"? (In this case we can't merge with the existing 'break' statement, since it's at a different level of curly-brace nesting)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8399125 [details] [diff] [review]
Patch

>+++ b/layout/style/test/test_grid_shorthand_serialization.html
[...]
>+  <title>Test parsing of grid container shorthands (grid-template, grid)</title>

I think this test wants a different title? (I think this is the title from test_grid_container_shorthands.html)  Maybe just s/parsing/serialization/.

r=me with that & comment 8 addressed / responded to.

Comment 10

[Simon Sapin (:SimonSapin)]

Attachment: Patch v2. r=dholbert

> Same here, I think -- looks like this should be "break"? (In this case we
> can't merge with the existing 'break' statement, since it's at a different
> level of curly-brace nesting)

We can’t in the other case either, for the same reason.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e825b5aa2e89

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e825b5aa2e89

Comment 13

[Firefox Commit Bot]

The following changeset is now in Firefox Nightly:

> e825b5aa2e89 Bug 989755: Fix up serialization of the grid-template shorthand. r=dholbert

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central