984760 - nsCSSValueGridTemplateAreas should be stored in a nsRefPtr in nsStyleStruct, and possibly not have nsCSSValue in its name

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Daniel Holbert [:dholbert]]

SimonSapin and I just realized that nsCSSValueGridTemplateAreas isn't refcounted in its style struct, but should be.

Also, while we're fixing that, we possibly should rename it to not have nsCSSValue in the name, given that we're using it in both specified style & computed style.

(Generally, nsCSSValue* is specified-style-only. We do have a few structs that get shared between nsCSSValue and nsStyleStruct, but those things generally have non-nsCSSValue names, like e.g. mozilla::css::URLValue. That's probably the model we should follow here.)

Comment 1

[Daniel Holbert [:dholbert]]

Additionally, nsCSSValueGridTemplateAreas should define a private destructor (which has an empty body).

The fact that the destructor is private will make it a compile error if we accidentally instantiate nsCSSValueGridTemplateAreas as a stack variable or member-variable.

(We should probably always do this for reference-counted things, though we don't consistently right now.)

Comment 2

[Daniel Holbert [:dholbert]]

(And we should declare the struct as MOZ_FINAL to be sure that nothing inherits from it, because inheriting from something that's refcounted is potentially dangerous [without a virtual destructor])

Comment 3

[Daniel Holbert [:dholbert]]

(That is to say - inheriting from something *that defines AddRef/Delete* (e.g. through NS_INLINE_DECL_REFCOUNTING) is potentially dangerous.)

Comment 4

[Daniel Holbert [:dholbert]]

(er, AddRef/Release, of course (not Delete). Sorry, I'll stop spamming this bug now. :))

Comment 5

[Simon Sapin (:SimonSapin)]

Attachment: Patch 1: Rename

Comment 6

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2: Use nsRefPtr

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8392768 [details] [diff] [review]
Patch 1: Rename

>+++ b/layout/style/Declaration.cpp
>-      const nsCSSValueGridTemplateAreas& areas =
>+      const mozilla::css::GridTemplateAreasValue& areas =

Drop the prefixing here. 

(This file starts with namespace mozilla { namespace css {, and using mozilla::css inside of that is unnecessary.)

r=me with that.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8392768 [details] [diff] [review]
Patch 1: Rename

>@@ -1341,17 +1341,24 @@ nsCSSValue::AppendToString(nsCSSProperty aProperty, nsAString& aResult,
>   } else if (eCSSUnit_GridTemplateAreas == unit) {
>-    GetGridTemplateAreas().AppendToString(aProperty, aResult, aSerialization);
>+    const mozilla::css::GridTemplateAreasValue& areas = GetGridTemplateAreas();
>+    MOZ_ASSERT(!areas.mTemplates.IsEmpty(),
>+               "Unexpected empty array in GridTemplateAreasValue");
[...]
>-nsCSSValueGridTemplateAreas::AppendToString(nsCSSProperty aProperty,
>-                                       nsAString& aResult,
>-                                       nsCSSValue::Serialization aValueSerialization) const
>-{
>-  uint32_t length = mTemplates.Length();
>-  if (length == 0) {
>-    aResult.AppendLiteral("none");

Nit: technically, the empty-array assertion here (and the dropping of "none") isn't valid until patch 2, I think. As of patch 1, you're still using empty arrays to signify "none", I believe, which means we probably fail mochitests at this intermediate point, since this code is all enabled for mochitests.)

If I'm not misunderstanding, could you shift the changes RE zero-length arrays here to patch 2?

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8392769 [details] [diff] [review]
Patch 2: Use nsRefPtr

> bool
> CSSParserImpl::ParseGridTemplateAreas()
> {
>   nsCSSValue value;
>   if (ParseVariant(value, VARIANT_INHERIT | VARIANT_NONE, nullptr)) {
>     AppendValue(eCSSProperty_grid_template_areas, value);
>     return true;
>   }
> 
>-  css::GridTemplateAreasValue& areas = value.SetGridTemplateAreas();
>+  css::GridTemplateAreasValue* areas = value.SetGridTemplateAreas();

This should be a nsRefPtr<css::GridTemplateAreasValue>.

(Otherwise, if anyone added e.g. a special-case "value.SetNoneValue(); break;" below this code, then we'd end up still having |areas| as a local variable to non-obviously freed data, and if we dereferenced it (e.g. in the final NRows() call), that could end up being a scary use-after-free bug.)

The other functions before this one are fine, though -- they're taking a raw GridTemplateAreasValue pointer as a function-argument, and we can trust that (by convention) the caller is keeping the passed-in object alive with its own nsRefPtr. So the functions themselves don't have to bother using a nsRefPtr to keep the object alive.

> CSSParserImpl::ParseGridTemplateAfterString(const nsCSSValue& aColumnsValue,
>                                             const nsCSSValue& aFirstLineNames)
> {
>   MOZ_ASSERT(mToken.mType == eCSSToken_String,
>              "ParseGridTemplateAfterString called with a non-string token");
> 
>   nsCSSValue areasValue;
>   nsCSSValue rowsValue;
>-  css::GridTemplateAreasValue& areas = areasValue.SetGridTemplateAreas();
>+  css::GridTemplateAreasValue* areas = areasValue.SetGridTemplateAreas();

Same here -- |areas| should be a nsRefPtr.

(I wonder if SetGridTemplateAreas() / SetURLValue() / SetImageValue() should actually return an already_AddRefed, which would force you to use a refptr here and hence would prevent this footgun... maybe even the getters should do that, as well.  I'll file a followup on that.)

>diff --git a/layout/style/nsCSSValue.cpp b/layout/style/nsCSSValue.cpp
>-mozilla::css::GridTemplateAreasValue& nsCSSValue::SetGridTemplateAreas()
>+mozilla::css::GridTemplateAreasValue* nsCSSValue::SetGridTemplateAreas()
> {

While you're here, add a newline before "nsCSSValue" to match the preferred gecko style for function-definitions' function-signatures.

>+private:
>+  // Private destructor to make sure this isnât used as a stack variable
>+  // or member variable.
>+  ~GridTemplateAreasValue()

Fancy quote! Fix please.

>+  mozilla::css::GridTemplateAreasValue* GetGridTemplateAreas() const
>+  {
>+    NS_ABORT_IF_FALSE (mUnit == eCSSUnit_GridTemplateAreas,
>+                       "not a grid-template-areas value");

Nit: Drop the space between FALSE and (.

> CSSValue*
> nsComputedDOMStyle::DoGetGridTemplateAreas()
> {
>-  const nsTArray<nsString>& templates =
>-    StylePosition()->mGridTemplateAreas.mTemplates;
>-  if (templates.IsEmpty()) {
>+  const css::GridTemplateAreasValue* areas =
>+    StylePosition()->mGridTemplateAreas;
>+  if (!areas) {
>     nsROCSSPrimitiveValue *val = new nsROCSSPrimitiveValue;
>     val->SetIdent(eCSSKeyword_none);
>     return val;
>   }
> 
>   nsDOMCSSValueList *valueList = GetROCSSValueList(false);
>-  for (uint32_t i = 0; i < templates.Length(); i++) {
>+  for (uint32_t i = 0; i < areas->mTemplates.Length(); i++) {

Assert that areas->mTemplates is non-empty, after the early-return.  (Similar to your assertion in Declaration.cpp mentioned in comment 8.)

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
> static void
> SetGridTemplateAreas(const nsCSSValue& aValue,
>-                     css::GridTemplateAreasValue& aResult,
>-                     const css::GridTemplateAreasValue& aParentValue,
>+                     nsRefPtr<css::GridTemplateAreasValue>& aResult,
>+                     const nsRefPtr<css::GridTemplateAreasValue>& aParentValue,
>                      bool& aCanStoreInRuleTree)

nsRefPtr<css::GridTemplateAreasValue>& is a bit too magical here. We generally avoid nsRefPtr<> as a parameter-type (in part because a raw pointer will suffice, given that you can assume the caller is holding the thing alive).  I'm not sure offhand if nsRefPtr<>& is technically dangerous, but we have better standard ways of doing it.

Change as follows:
 - aParentValue should have type css::GridTemplateAreasValue*
 - aResult should have type css::GridTemplateAreasValue** (and where you assign it, you'd then do *aResult = aParentValue, or *aResult = nullptr, or whatever)
 - The caller will need to wrap its aResult arg in getter_AddRefs().

Note that if we were guaranteed to always set the outparam value, then we could instead just return it directly in an already_AddRefed<> value, as dbaron mentioned earlier today -- but in this case, we don't want to touch the value in case eCSSUnit_Null, so we have to use an outparam which we only conditionally modify.

>diff --git a/layout/style/nsStyleStruct.cpp b/layout/style/nsStyleStruct.cpp
>index b35e0b9..ac3d9f1 100644
>--- a/layout/style/nsStyleStruct.cpp
>+++ b/layout/style/nsStyleStruct.cpp
>@@ -1251,17 +1251,17 @@ nsStylePosition::nsStylePosition(void)
>   mOrder = NS_STYLE_ORDER_INITIAL;
>   mFlexGrow = 0.0f;
>   mFlexShrink = 1.0f;
>   mZIndex.SetAutoValue();
>   mGridAutoPositionColumn.SetToInteger(1);
>   mGridAutoPositionRow.SetToInteger(1);
>   // mGridTemplateRows, mGridTemplateColumns, and mGridTemplateAreas
>   // get their default constructors
>-  // which initialize them to empty arrays,
>+  // which initialize them to empty arrays or nullptr,
>   // which represent the properties' initial value 'none'.

Spin off a separate comment here for mGridTemplateAreas.  ("empty arrays or nullptr" is a bit confusing.)

Maybe:
 // mGridTemplateAreas defaults to nullptr, which represents its
 // initial value 'none'.

r=me with the above addressed.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> >-  css::GridTemplateAreasValue& areas = value.SetGridTemplateAreas();
> >+  css::GridTemplateAreasValue* areas = value.SetGridTemplateAreas();
> 
> This should be a nsRefPtr<css::GridTemplateAreasValue>.
[...]
> (I wonder if SetGridTemplateAreas() / SetURLValue() / SetImageValue() should
> actually return an already_AddRefed

Actually, I just noticed that SetURLValue and SetImageValue expect the *caller* to have allocated the thing, and they take the pointer as an input parameter.

We should follow that pattern here, too. Something like:
  nsRefPtr<css::GridTemplateAreasValue> areas = new css::GridTemplateAreas();
  [...populate the object...]
  nsCSSValue value;
  value.SetGridTemplateAreas(areas);
  AppendValue(eCSSProperty_grid_template_areas, value);

You can even add a new nsCSSValue() constructor like the ones we have that take a URLValue*/ImageValue*, which saves you the trouble of having to call the setter at all. Then, you can just replace those last 3 lines with:
  AppendValue(eCSSProperty_grid_template_areas, nsCSSValue(areas));

Comment 11

[Simon Sapin (:SimonSapin)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Comment on attachment 8392768 [details] [diff] [review]
> Patch 1: Rename
> 
> >@@ -1341,17 +1341,24 @@ nsCSSValue::AppendToString(nsCSSProperty aProperty, nsAString& aResult,
> >   } else if (eCSSUnit_GridTemplateAreas == unit) {
> >-    GetGridTemplateAreas().AppendToString(aProperty, aResult, aSerialization);
> >+    const mozilla::css::GridTemplateAreasValue& areas = GetGridTemplateAreas();
> >+    MOZ_ASSERT(!areas.mTemplates.IsEmpty(),
> >+               "Unexpected empty array in GridTemplateAreasValue");
> [...]
> >-nsCSSValueGridTemplateAreas::AppendToString(nsCSSProperty aProperty,
> >-                                       nsAString& aResult,
> >-                                       nsCSSValue::Serialization aValueSerialization) const
> >-{
> >-  uint32_t length = mTemplates.Length();
> >-  if (length == 0) {
> >-    aResult.AppendLiteral("none");
> 
> Nit: technically, the empty-array assertion here (and the dropping of
> "none") isn't valid until patch 2, I think. As of patch 1, you're still
> using empty arrays to signify "none", I believe, which means we probably
> fail mochitests at this intermediate point, since this code is all enabled
> for mochitests.)
> 
> If I'm not misunderstanding, could you shift the changes RE zero-length
> arrays here to patch 2?

Actually it’s aResult.AppendLiteral("none"); that was bogus here. The array is never empty in specified values (we use eCSSUnit_None instead), only in computed values. I’ll add a comment along these lines.

Comment 12

[Daniel Holbert [:dholbert]]

Comment on attachment 8392769 [details] [diff] [review]
Patch 2: Use nsRefPtr

>diff --git a/layout/style/nsCSSValue.h b/layout/style/nsCSSValue.h
>+private:
>+  // Private destructor to make sure this isnât used as a stack variable
>+  // or member variable.
>+  ~GridTemplateAreasValue()
>+  {
>+  }
> };

One more thing here: please add MOZ_DELETE'd copy-constructor and reassignment operator here, like we have for other structs in this file, for consistency.

(This prevents us from accidentally (and needlessly) creating a copy of a GridTemplateAreasValue instance.)

Comment 13

[Simon Sapin (:SimonSapin)]

Attachment: Rename v2. r=dholbert

Comment 14

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2 v2: Use nsRefPtr. r=dholbert

Comment 15

[Simon Sapin (:SimonSapin)]

This looks like it’ll be ready to land as soon as bug 981752 does.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 8393350 [details] [diff] [review]
Patch 2 v2: Use nsRefPtr. r=dholbert

>diff --git a/layout/style/nsCSSValue.h b/layout/style/nsCSSValue.h
>   eCSSUnit_Gradient     = 42,     // (nsCSSValueGradient*) value
>   eCSSUnit_TokenStream  = 43,     // (nsCSSValueTokenStream*) value
>+  eCSSUnit_GridTemplateAreas   = 44,   // (GridTemplateAreasValue*)
>+                                       // for grid-template-areas
>+
> 
>   eCSSUnit_Pair         = 50,     // (nsCSSValuePair*) pair of values
>   eCSSUnit_Triplet      = 51,     // (nsCSSValueTriplet*) triplet of values

Nit: there's an extra blank line there. (2 blank lines after the end of the comment, before the next thing)

Please collapse that to just 1 blank line, for consistency w/ the rest of this enum list.

>+++ b/layout/style/nsStyleStruct.cpp
>+  // mGrid{Column,Row}{Start,End}: eStyleUnit_Auto for 'auto'

Nit: "eStyleUnit_Auto" is incorrect here. (These member vars don't use eStyleUnit_* -- they have type nsStyleGridLine, which is a special struct and doesn't use nsStyleUnit.)

Maybe replace with:
  // mGrid{Column,Row}{Start,End}: initial struct value represents 'auto'

Comment 17

[Simon Sapin (:SimonSapin)]

Attachment: Patch 2 v3: Use nsRefPtr. r=dholbert

Comment 18

[Simon Sapin (:SimonSapin)]

Batched Try run for this and 981752 https://tbpl.mozilla.org/?tree=Try&rev=a897a2c983b4

Comment 19

[Daniel Holbert [:dholbert]]

The reds there are an issue with bug 981752 which I've worked around, per bug 981752 comment 49 - 50.
 
Landed this:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e6d0a90b04
  https://hg.mozilla.org/integration/mozilla-inbound/rev/cdfc29502d3a

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b6e6d0a90b04
https://hg.mozilla.org/mozilla-central/rev/cdfc29502d3a