Closed Bug 989755 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: jruderman, Assigned: SimonSapin)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(5 files, 1 obsolete file)

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
Attached file stack
Why are we serializing stuff during cycle collection?
Attachment #8399105 - Attachment description: c.html → testcase that requires cycle collection
Attached file better testcase
Attached file stack
(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!)
This is a bug introduced in Bug 983175.
Assignee: nobody → simon.sapin
Status: NEW → ASSIGNED
Blocks: css-grid
Attached patch Patch (obsolete) — Splinter Review
Attachment #8399125 - Flags: review?(dholbert)
Comment on attachment 8399125 [details] [diff] [review]
Patch

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

>From: Simon Sapin <simon.sapin@exyr.org>
>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 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.
Attachment #8399125 - Flags: review?(dholbert) → review+
> 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.
Attachment #8399125 - Attachment is obsolete: true
Attachment #8399609 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e825b5aa2e89
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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
You need to log in before you can comment on or make changes to this bug.