898361 - [CSS Filters] Implement parsing for drop-shadow

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Dirk Schulze]

Implement parsing and computation for drop-shadow filter function.

Comment 1

[Dirk Schulze]

Attachment: drop-shadow-moz.patch

Patch

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 781630 [details] [diff] [review]
drop-shadow-moz.patch

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

::: layout/style/nsCSSParser.cpp
@@ +10041,5 @@
>  
>  /**
> + * Reads a drop-shadow value. At the moment the Filter Effects specification
> + * just expects one shadow item. Should this ever change to a list of shadow
> + * items, use ParseShadowList instead.

The spec has

  drop-shadow([<length>{2,3} && <color>?]#)

in the grammar.  Is the spec wrong or this comment?

@@ +10056,5 @@
> +
> +  if (!ExpectSymbol(')', true))
> +    return false;
> +
> +  nsRefPtr<nsCSSValue::Array> dropShadow =

You can just use a bare pointer here; aValue holds on to the array.

@@ +10057,5 @@
> +  if (!ExpectSymbol(')', true))
> +    return false;
> +
> +  nsRefPtr<nsCSSValue::Array> dropShadow =
> +    aValue->InitFunction(eCSSKeyword_drop_shadow, 1U);

No need for the "U" really.

@@ +10059,5 @@
> +
> +  nsRefPtr<nsCSSValue::Array> dropShadow =
> +    aValue->InitFunction(eCSSKeyword_drop_shadow, 1U);
> +
> +  /* Copy things over. */

Use "//" if you're using that style of comment above.

@@ +10093,5 @@
>      return false;
>    }
>  
> +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  // Parse drop-shadow independent of the other filter functions

independently

@@ +10094,5 @@
>    }
>  
> +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  // Parse drop-shadow independent of the other filter functions
> +  // because of the more complex characteristic.

its more complex characteristics

@@ +10095,5 @@
>  
> +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> +  // Parse drop-shadow independent of the other filter functions
> +  // because of the more complex characteristic.
> +  if (eCSSKeyword_drop_shadow == functionName) {

(I prefer "functionName == eCSSKeyword_drop_shadow", but I know this file heavily uses the other way.)

@@ +10102,5 @@
> +      REPORT_UNEXPECTED_TOKEN(PEExpectedNoneOrURLOrFilterFunction);
> +      SkipUntil(')');
> +      return false;
> +    } else {
> +      return true;

Don't put the "return true;" in an "else".

::: layout/style/nsComputedDOMStyle.cpp
@@ +4493,5 @@
>        aString.AssignLiteral("contrast(");
>        break;
> +    case nsStyleFilter::Type::eDropShadow:
> +      aString.AssignLiteral("drop-shadow(");
> +      break;      

Trailing white space.

@@ +4535,5 @@
> +    // Handle drop-shadow()
> +    ErrorResult err;
> +    nsRefPtr<CSSValue> shadowValue =
> +      GetCSSShadowArray(aStyleFilter.mDropShadow,
> +                        StyleColor()->mColor,

Is this right?  The spec says the lacuna value is "0 0 0 black", which is different from say 'text-shadow' where the implied colour value is whatever value 'color' has.

Also, why are we using a shadow array?

@@ +4537,5 @@
> +    nsRefPtr<CSSValue> shadowValue =
> +      GetCSSShadowArray(aStyleFilter.mDropShadow,
> +                        StyleColor()->mColor,
> +                        false);
> +    shadowValue->GetCssText(argumentString, err);

Probably better to use the GetCssText that takes a single value and returns an nsresult.  (The one with the ErrorResult is the Web IDL implementation for the CSSValue interface.)  nsROCSSPrimitiveValue::GetCssText will already assert if it has a bad value, so we can just ignore the nsresult return value.  (See various other calls to GetCssText in this file that do this.)

::: layout/style/nsRuleNode.cpp
@@ +7740,1 @@
>                           const nsCSSValue& aValue,

Fix indentation of arguments.

::: layout/style/nsStyleStruct.cpp
@@ +1037,5 @@
>  
>    if (mType == eURL) {
>      return EqualURIs(mURL, aOther.mURL);
> +  } else if (mType == eDropShadow) {
> +    return CalcShadowDifference(mDropShadow, aOther.mDropShadow);

Can you add an operator== to nsCSSShadowArray that uses the existing operator== on nsCSSShadowItem and then make this "return mDropShadow == aOther.mDropShadow;"?

::: layout/style/nsStyleStruct.h
@@ +2291,5 @@
>  
>    Type mType;
>    nsIURI* mURL;
>    nsStyleCoord mCoord;
> +  nsRefPtr<nsCSSShadowArray> mDropShadow; // [inherited] NULL in case of a zero-length

Don't mention "[inherited]" there, for the reasons dbaron mentioned in bug 898175 comment 3.

I see we're not refcounting mURL probably.  It should be an nsRefPtr<> -- I must have missed that while reviewing bug 895182.

Also, I'm not liking the addition of new members here outside of a union.  I think we should do something about it.  Maybe we can have a union of nsIURI* and nsCSSShadowArray* and do manual refcounting on them?  (There's no common type for these two that we could use and just put them in a nsRefPtr<>.)  You can see how nsStyleContentData does this, for example.

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> Also, why are we using a shadow array?

Meant to remove that comment pending an answer to whether the spec is right in accepting a list of shadows.

Comment 4

[Dirk Schulze]

(In reply to Cameron McCormack (:heycam) from comment #3)
> (In reply to Cameron McCormack (:heycam) from comment #2)
> > Also, why are we using a shadow array?
> 
> Meant to remove that comment pending an answer to whether the spec is right
> in accepting a list of shadows.

The WD was not updated yet. See https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#FilterProperty 

I would like to have the full syntax of box-shadow in Level 2. Some implementations stated that they can't implement it just yet.

Comment 5

[Dirk Schulze]

(In reply to Cameron McCormack (:heycam) from comment #2)
> Comment on attachment 781630 [details] [diff] [review]
> drop-shadow-moz.patch
> 
> Review of attachment 781630 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/nsCSSParser.cpp
> @@ +10041,5 @@
> >  
> >  /**
> > + * Reads a drop-shadow value. At the moment the Filter Effects specification
> > + * just expects one shadow item. Should this ever change to a list of shadow
> > + * items, use ParseShadowList instead.
> 
> The spec has
> 
>   drop-shadow([<length>{2,3} && <color>?]#)
> 
> in the grammar.  Is the spec wrong or this comment?

As discussed. An oversight in the spec.

> 
> @@ +10056,5 @@
> > +
> > +  if (!ExpectSymbol(')', true))
> > +    return false;
> > +
> > +  nsRefPtr<nsCSSValue::Array> dropShadow =
> 
> You can just use a bare pointer here; aValue holds on to the array.

Done.

> 
> @@ +10057,5 @@
> > +  if (!ExpectSymbol(')', true))
> > +    return false;
> > +
> > +  nsRefPtr<nsCSSValue::Array> dropShadow =
> > +    aValue->InitFunction(eCSSKeyword_drop_shadow, 1U);
> 
> No need for the "U" really.

Done.

> 
> @@ +10059,5 @@
> > +
> > +  nsRefPtr<nsCSSValue::Array> dropShadow =
> > +    aValue->InitFunction(eCSSKeyword_drop_shadow, 1U);
> > +
> > +  /* Copy things over. */
> 
> Use "//" if you're using that style of comment above.

Done.

> 
> @@ +10093,5 @@
> >      return false;
> >    }
> >  
> > +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> > +  // Parse drop-shadow independent of the other filter functions
> 
> independently

Done.

> 
> @@ +10094,5 @@
> >    }
> >  
> > +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> > +  // Parse drop-shadow independent of the other filter functions
> > +  // because of the more complex characteristic.
> 
> its more complex characteristics

Done.

> 
> @@ +10095,5 @@
> >  
> > +  nsCSSKeyword functionName = nsCSSKeywords::LookupKeyword(mToken.mIdent);
> > +  // Parse drop-shadow independent of the other filter functions
> > +  // because of the more complex characteristic.
> > +  if (eCSSKeyword_drop_shadow == functionName) {
> 
> (I prefer "functionName == eCSSKeyword_drop_shadow", but I know this file
> heavily uses the other way.)

Done.

> 
> @@ +10102,5 @@
> > +      REPORT_UNEXPECTED_TOKEN(PEExpectedNoneOrURLOrFilterFunction);
> > +      SkipUntil(')');
> > +      return false;
> > +    } else {
> > +      return true;
> 
> Don't put the "return true;" in an "else".

Done.

> 
> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +4493,5 @@
> >        aString.AssignLiteral("contrast(");
> >        break;
> > +    case nsStyleFilter::Type::eDropShadow:
> > +      aString.AssignLiteral("drop-shadow(");
> > +      break;      
> 
> Trailing white space.

Done.

> 
> @@ +4535,5 @@
> > +    // Handle drop-shadow()
> > +    ErrorResult err;
> > +    nsRefPtr<CSSValue> shadowValue =
> > +      GetCSSShadowArray(aStyleFilter.mDropShadow,
> > +                        StyleColor()->mColor,
> 
> Is this right?  The spec says the lacuna value is "0 0 0 black", which is
> different from say 'text-shadow' where the implied colour value is whatever
> value 'color' has.

As discussed, we do not want to differ from text-shadow and box-shadow, but needs discussion on the WG.

> 
> Also, why are we using a shadow array?

The current version does not allow multiple shadows, but it is on the agenda for the next level to do that. Refactoring now would make the code less readable and be obsolete with the next level of Fitler Effects.

> 
> @@ +4537,5 @@
> > +    nsRefPtr<CSSValue> shadowValue =
> > +      GetCSSShadowArray(aStyleFilter.mDropShadow,
> > +                        StyleColor()->mColor,
> > +                        false);
> > +    shadowValue->GetCssText(argumentString, err);
> 
> Probably better to use the GetCssText that takes a single value and returns
> an nsresult.  (The one with the ErrorResult is the Web IDL implementation
> for the CSSValue interface.)  nsROCSSPrimitiveValue::GetCssText will already
> assert if it has a bad value, so we can just ignore the nsresult return
> value.  (See various other calls to GetCssText in this file that do this.)

GetCSSShadowArray can just return CSSValue* (different return types in the function). Therefore, I couldn't use nsROCSSPrimitiveValue::GetCssText.

> 
> ::: layout/style/nsRuleNode.cpp
> @@ +7740,1 @@
> >                           const nsCSSValue& aValue,
> 
> Fix indentation of arguments.

Done.

> 
> ::: layout/style/nsStyleStruct.cpp
> @@ +1037,5 @@
> >  
> >    if (mType == eURL) {
> >      return EqualURIs(mURL, aOther.mURL);
> > +  } else if (mType == eDropShadow) {
> > +    return CalcShadowDifference(mDropShadow, aOther.mDropShadow);
> 
> Can you add an operator== to nsCSSShadowArray that uses the existing
> operator== on nsCSSShadowItem and then make this "return mDropShadow ==
> aOther.mDropShadow;"?

Done.

> 
> ::: layout/style/nsStyleStruct.h
> @@ +2291,5 @@
> >  
> >    Type mType;
> >    nsIURI* mURL;
> >    nsStyleCoord mCoord;
> > +  nsRefPtr<nsCSSShadowArray> mDropShadow; // [inherited] NULL in case of a zero-length
> 
> Don't mention "[inherited]" there, for the reasons dbaron mentioned in bug
> 898175 comment 3.

Done.

> 
> I see we're not refcounting mURL probably.  It should be an nsRefPtr<> -- I
> must have missed that while reviewing bug 895182.
> 
> Also, I'm not liking the addition of new members here outside of a union.  I
> think we should do something about it.  Maybe we can have a union of nsIURI*
> and nsCSSShadowArray* and do manual refcounting on them?  (There's no common
> type for these two that we could use and just put them in a nsRefPtr<>.) 
> You can see how nsStyleContentData does this, for example.

We discussed it on the mailing list and found a way to combine mUrl and mDropShadow in a union.

Done.

I'll upload a new patch soon.

Comment 6

[Dirk Schulze]

Attachment: drop-shadow-moz.patch

This is the updated patch. I added NS_IF_RELEASE(mURL) but this caused crashes in nsReferencedElement.cpp:29. I am not sure what this means. I commented the lines out in the patch. I am not very familiar with the self refcounting in Gecko. Hope to get some help.

Comment 7

[Cameron McCormack (:heycam)]

You'll need to addref the pointer that you copy over in the operator= and the copy constructor.  That's probably why you're crashing.

So how about a design like this.  Rather than having a SetType function, have three functions:

* SetSimpleFunction(Type, const nsStyleCoord&)
* SetURL(nsIURI*)
* SetDropShadow(nsCSSShadowArray*)

Each of these functions would do this:
  - when switching away from eURL, it releases mURL
  - when switching away from eDropShadow, it releases mDropShadow

SetURL addrefs the nsIURI* it gets and stores it in mURL.

SetDropShadow addrefs the nsCSSShadowArray* and stores it in mDropShadow.

The copy constructor and operator= addref the pointer that they copy.

GetFunctionArgument, GetURL and GetDropShadow should assert that mType is one of the expected types for that function to make sense.

Since we might in the future manage to stick the nsStyleCoord in the union too, I don't think it's necessary to ensure that the union has null in it when mType is not eURL or eDropShadow.  So you can also remove the "mURL = nullptr" in the default constructor.

In nsRuleNode::SetStyleFilterToCSSValue, take the return value from GetShadowArray and put it in an nsRefPtr<nsCSSShadowArray> variable.  You should be able to pass that nsRefPtr<nsCSSShadowArray> directly to SetDropShadow (nsRefPtr has an operator T*() that will convert it automatically).  SetDropShadow will addref the object (so refcount = 2), then when nsRefPtr goes out of scope it will release it (ending up with refcount = 1).

(You could maybe make the argument to SetDropShadow be already_AddRefed<nsCSSShadowArray>, to avoid the extra addref/release you get from storing in the nsRefPtr local variable.  But this then would require an explicit construction of an already_AddRefed to pass to the function, and it's not that common in the codebase to have already_AddRefed arguments; it's used mostly for return values.)

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #7)
> Each of these functions would do this:
>   - when switching away from eURL, it releases mURL
>   - when switching away from eDropShadow, it releases mDropShadow

And you can of course have a helper function to do this, which you'd call from these functions as well as from the destructor.  nsStyleSVGPaint uses the destructor itself as that helper function, but that looks a little too clever to me.

Comment 9

[Dirk Schulze]

(In reply to Cameron McCormack (:heycam) from comment #7)
> You'll need to addref the pointer that you copy over in the operator= and
> the copy constructor.  That's probably why you're crashing.
> 

Do you mean using nsRefPtr for mURL and mDropShadow? nsRefPtr seems to have non trivial CTORs which was the problem for nsCoordStyle. This is not supported for C++ < 11.

There is some old code that checks if URL is null. If it is, it is assumed to not be a valid URL. This could be refactored.

Comment 10

[Cameron McCormack (:heycam)]

(In reply to Dirk Schulze from comment #9)
> Do you mean using nsRefPtr for mURL and mDropShadow? nsRefPtr seems to have
> non trivial CTORs which was the problem for nsCoordStyle. This is not
> supported for C++ < 11.

No, we still must use use raw pointers in the union.  But AddRef and Release are member functions on nsIURI and nsCSSShadowArray; they don't need to be wrapped in an nsRefPtr to be able to be called.

> There is some old code that checks if URL is null. If it is, it is assumed
> to not be a valid URL. This could be refactored.

If mURL can be null to indicate that the filter is a <filter> element reference but the reference is invalid, then we can store null in that case.  But I don't think we need to set mURL to null when we are switching away from mType == eURL.

Comment 11

[Dirk Schulze]

Attachment: Patch v4

This patch addresses reviewer feedback.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 783093 [details] [diff] [review]
Patch v4

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

This is looking good, we're almost there. :-)  The main issue remaining is SetDropShadow.

::: layout/style/nsRuleNode.cpp
@@ +7742,5 @@
>        return nsStyleFilter::Type::eNull;
>    }
>  }
>  
> +void nsRuleNode::SetStyleFilterToCSSValue(nsStyleFilter* aStyleFilter,

Newline after "void".

@@ +7767,5 @@
> +      filterFunction->Item(1).GetListValue(),
> +      aStyleContext,
> +      false,
> +      aCanStoreInRuleTree));
> +    return;

See other comments below about storing the GetShadowData return value in a local nsRefPtr variable.

@@ +7779,5 @@
>    }
>  
>    NS_ABORT_IF_FALSE(filterFunction->Count() == 2,
>                      "all filter functions except drop-shadow should have "
>                      "exactly one argument");

Is this assertion not getting tripped because the shadow list argument is a single CSSValue?  Looks like you should change the message here.

::: layout/style/nsRuleNode.h
@@ +626,5 @@
>    already_AddRefed<nsCSSShadowArray>
>                GetShadowData(const nsCSSValueList* aList,
>                              nsStyleContext* aContext,
>                              bool aIsBoxShadow,
> +                            bool& canStoreInRuleTree);

While renaming this, could you make it "aCanStoreInRuleTree" (and in nsRuleNode.cpp)?

::: layout/style/nsStyleStruct.cpp
@@ +1044,5 @@
>  
>    return true;
>  }
>  
> +void nsStyleFilter::ReleaseRef()

Newline after "void".

@@ +1057,5 @@
> +}
> +
> +void
> +nsStyleFilter::SetFilterParameter(const nsStyleCoord& aFilterParameter,
> +                                  Type aType) {

Newline before "{".

@@ +1063,5 @@
> +  mFilterParameter = aFilterParameter;
> +  mType = aType;
> +}
> +
> +void nsStyleFilter::SetURL(nsIURI* aURL) {

Newline after "void" and before "{".

@@ +1066,5 @@
> +
> +void nsStyleFilter::SetURL(nsIURI* aURL) {
> +  NS_ASSERTION(aURL, "expected pointer");
> +  ReleaseRef();
> +  if (aURL) {

We shouldn't assert and null check aURL.  If aURL should never be null, just assume it here and leave the assertion.

@@ +1074,5 @@
> +  }
> +}
> +
> +void
> +nsStyleFilter::SetDropShadow(nsRefPtr<nsCSSShadowArray> aDropShadow) {

Newline before "{".

@@ +1079,5 @@
> +  NS_ASSERTION(aDropShadow, "expected pointer");
> +  ReleaseRef();
> +  mDropShadow = aDropShadow.get();
> +  aDropShadow.forget();
> +  mDropShadow->AddRef();

I don't think this is right.  forget() will clear out the pointer in the nsRefPtr, so there would be no need to AddRef straight afterwards, as we have already skipped the Release that would normally happen when assigning null to the nsRefPtr.

As I mentioned in comment 7, I think it would be better to have SetDropShadow take a raw nsCSSShadowArray pointer, and in nsRuleNode::SetStyleFilterToCSSValue, assign the result of GetShadowData to a local nsRefPtr variable.  You can pass that directly to this function expecting a raw pointer, since nsRefPtrs can be converted to raw pointers.

::: layout/style/nsStyleStruct.h
@@ +2305,5 @@
> +  Type GetType() const {
> +    return mType;
> +  }
> +
> +  nsStyleCoord GetFilterParameter() const {

Can you make this return "const nsStyleCoord&", to avoid copying.

@@ +2334,4 @@
>    nsStyleCoord mFilterParameter; // coord, percent, factor, angle
> +  union {
> +    nsIURI* mURL;
> +    nsCSSShadowArray* mDropShadow; // NULL in case of a zero-length

A zero-length what?  (List of shadows?  Is it actually possible to have no shadows specified within the drop-shadow()?  You're asserting in SetDropShadow otherwise.)

Comment 13

[Dirk Schulze]

Attachment: Patch v4

Addresses even more reviewer feedback. :)

Comment 14

[Dirk Schulze]

Attachment: Patch v5

Typo in previous patch.

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 783615 [details] [diff] [review]
Patch v5

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

Looks great. r=me with this one comment removed.

::: layout/style/nsStyleStruct.h
@@ +2334,4 @@
>    nsStyleCoord mFilterParameter; // coord, percent, factor, angle
> +  union {
> +    nsIURI* mURL;
> +    nsCSSShadowArray* mDropShadow; // shadow

The "// coord, percent, ..." comments are used to indicate which possible types of objects are used in the variable.  So for nsStyleCoord, it indicates what mUnit values it can have.  nsCSSShadowArray doesn't have different types, so I don't think there's any value in putting "// shadow" here.

Comment 16

[Dirk Schulze]

Attachment: Patch for landing

Patch for landing

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Thanks for the patch, Dirk! One request - please make sure that you have Mercurial configured to generate patches with all the necessary commit information so that they're ready for landing. Makes life much easier for those checking in on your behalf.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 18

[Ed Morley [:emorley]]

Unfortunately I had to back this out for crashes, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25971502&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=25971722&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e926c98528ff

Comment 19

[Dirk Schulze]

(In reply to Ed Morley [:edmorley UTC+1] from comment #18)
> Unfortunately I had to back this out for crashes, eg:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=25971502&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=25971722&tree=Mozilla-
> Inbound
> 
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e926c98528ff

This is odd. I can not reproduce these failures on my machine. Just rebuild with the patch applied and all tests (including the tests that failed on the try servers) pass for me. Sadly the log is not precise enough to assume what happened.

I am on OS X 10.8, the servers are on 10.6 and 10.7.

Comment 20

[Max Vujovic]

(In reply to Dirk Schulze from comment #19)
> (In reply to Ed Morley [:edmorley UTC+1] from comment #18)
> > Unfortunately I had to back this out for crashes, eg:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=25971502&tree=Mozilla-
> > Inbound
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=25971722&tree=Mozilla-
> > Inbound
> > 
> > remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e926c98528ff
> 
> This is odd. I can not reproduce these failures on my machine. Just rebuild
> with the patch applied and all tests (including the tests that failed on the
> try servers) pass for me. Sadly the log is not precise enough to assume what
> happened.
> 
> I am on OS X 10.8, the servers are on 10.6 and 10.7.

I just pushed a try job on crashtests OS X 10.6-8, in case it provides some more info.
https://tbpl.mozilla.org/?tree=Try&rev=1761c46aa2de

Comment 21

[Cameron McCormack (:heycam)]

It seems to be a null pointer dereference on this line:

  https://hg.mozilla.org/integration/mozilla-inbound/file/e27bd873b413/layout/style/nsStyleStruct.cpp#l1057

Comment 22

[Cameron McCormack (:heycam)]

It might be that there is no operator= defined for nsStyleFilter.

Comment 23

[Cameron McCormack (:heycam)]

Rather, there is no operator=, and that might be the problem.

Comment 24

[Dirk Schulze]

Attachment: drop-shadow-moz.patch

Comment 25

[Cameron McCormack (:heycam)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=74b6cd7803dc

Comment 26

[Cameron McCormack (:heycam)]

I think I realise the problem now.  nsTArray assumes that its elements can be memmoved around.  nsStyleFilters obviously can't be.

Two solutions: make it a std::vector<nsStyleFilter>, or (suggested by khuey) use an nsTArray<nsAutoPtr<nsStyleFilter> >.  The latter would require fewer code changes.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Or you could use nsTArray_CopyWithConstructors.

Comment 28

[Dirk Schulze]

Attachment: drop-shadow-moz.patch

For try bots.

Comment 29

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=d4b7a72059fe

Comment 30

[Dirk Schulze]

Attachment: drop-shadow-moz.patch

For landing.

Comment 31

[Dirk Schulze]

Attachment: drop-shadow-moz.patch

Accidentally uploaded same patch again. Now the right one for review and landing...

Comment 32

[Cameron McCormack (:heycam)]

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

Comment 33

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And out again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/620bbab02c0b

for:

https://tbpl.mozilla.org/php/getParsedLog.php?id=26134896&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=26134898&tree=Mozilla-Inbound

and more.

Comment 34

[Dirk Schulze]

Well, then I have no idea what I can do more. It passed all try bots and runs locally. I can't figure what is going wrong on mozilla-inbound.

Here the results of the try bots:
https://tbpl.mozilla.org/?tree=Try&rev=d4b7a72059fe

Comment 35

[Dirk Schulze]

I just verified that the right patch was landed, the same that passed the try bots. Just with the difference of

template<>
struct nsTArray_CopyElements<nsStyleFilter>
  : public nsTArray_CopyWithConstructors<nsStyleFilter> {};

to 

template<>
struct nsTArray_CopyElements<nsStyleFilter> : public nsTArray_CopyWithConstructors<nsStyleFilter> {};

Note the newline after the column. (A reviewer request.) I doubt that this is the problem.

Comment 36

[Markus Stange [:mstange]]

Reading through the patch, this part stood out to me:

 nsStyleFilter::nsStyleFilter(const nsStyleFilter& aSource)
-  : mType(aSource.mType)
 {
   MOZ_COUNT_CTOR(nsStyleFilter);
-
-  if (mType == eURL) {
-    mURL = aSource.mURL;
-  } else if (mType != eNull) {
-    mFilterParameter = aSource.mFilterParameter;
+  if (aSource.mType == eURL) {
+    SetURL(aSource.mURL);

You no longer initialize mType, but SetURL and SetDropShadow call ReleaseRef, which calls Release depending on the uninitialized mType value. Could that be it?

Comment 37

[Cameron McCormack (:heycam)]

Good catch Markus, that definitely sounds like it could be the problem.

Comment 38

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

(In reply to Cameron McCormack (:heycam) from comment #26)
> I think I realise the problem now.  nsTArray assumes that its elements can
> be memmoved around.  nsStyleFilters obviously can't be.

Why not?

Comment 39

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #38)
> Why not?

Because they are storing manually refcounted pointers.

Comment 40

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

(In reply to Cameron McCormack (:heycam) from comment #39)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #38)
> > Why not?
> 
> Because they are storing manually refcounted pointers.

Why does that prevent us from memmove-ing them?  (Things having pointers *to* them would prevent that, but I don't see that.)


Also, why is nsStyleFilters doing all that reference counting?  Style structs can't outlive the rules on which their data come from, which in turn (at least for CSS rules, and preferably for all rules although I'm not sure if that's currently true) should hold on to the data they map for their lifetime.  We already rely on this for mSpecifiedTransform, and perhaps ought to do so more.  (Or maybe this isn't he place to start doing so, though?)

Comment 41

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #40)
> Why does that prevent us from memmove-ing them?  (Things having pointers
> *to* them would prevent that, but I don't see that.)

I suppose it prevents us from memcpy-ing them, though not memmove-ing.  So I guess we could not define the nsTArray_CopyElements specialisation, but make sure that we after mFilters gets copied in nsStyleSVGReset's copy constructor, that we then AddRef the mURL/mDropShadow in the copied nsStyleFilter.

> Also, why is nsStyleFilters doing all that reference counting?  Style
> structs can't outlive the rules on which their data come from, which in turn
> (at least for CSS rules, and preferably for all rules although I'm not sure
> if that's currently true) should hold on to the data they map for their
> lifetime.  We already rely on this for mSpecifiedTransform, and perhaps
> ought to do so more.  (Or maybe this isn't he place to start doing so,
> though?)

It's manual refcounting of the nsIURI*/nsCSSShadowArray* members of the union in nsStyleFilter.

Comment 42

[Cameron McCormack (:heycam)]

Without mstange's fix: https://tbpl.mozilla.org/?tree=Try&rev=78d895213183
With it: https://tbpl.mozilla.org/?tree=Try&rev=23f2ae698e93

Comment 43

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

(In reply to Cameron McCormack (:heycam) from comment #41)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #40)
> I suppose it prevents us from memcpy-ing them, though not memmove-ing.

Ah, right.

> It's manual refcounting of the nsIURI*/nsCSSShadowArray* members of the
> union in nsStyleFilter.

I know, but I'm asking why it needs to own a reference to these in the first place.

Comment 44

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

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #43)
> I know, but I'm asking why it needs to own a reference to these in the first
> place.

(And I'd note I'm not saying I'm confident that it doesn't.  But I suspect it doesn't.  It's worth looking at whether we need to treat mSpecifiedTransform specially in any ways.)

Comment 45

[Cameron McCormack (:heycam)]

The nsCSSShadowArray is allocated dynamically, and isn't held on to anywhere else.  So I assume the nsStyleFilter needs to own that.  Same for the nsIURI?  mFilter was an nsCOMPtr<nsIURI> before the recent changes to start supporting the filter shorthand functions.

Comment 46

[Dirk Schulze]

(In reply to Markus Stange [:mstange] from comment #36)
> Reading through the patch, this part stood out to me:
> 
>  nsStyleFilter::nsStyleFilter(const nsStyleFilter& aSource)
> -  : mType(aSource.mType)
>  {
>    MOZ_COUNT_CTOR(nsStyleFilter);
> -
> -  if (mType == eURL) {
> -    mURL = aSource.mURL;
> -  } else if (mType != eNull) {
> -    mFilterParameter = aSource.mFilterParameter;
> +  if (aSource.mType == eURL) {
> +    SetURL(aSource.mURL);
> 
> You no longer initialize mType, but SetURL and SetDropShadow call
> ReleaseRef, which calls Release depending on the uninitialized mType value.
> Could that be it?

ReleaseRef releases the pointers from the new created nsStyleFilter. Since there are no pointers before creating them, this is certainly not what you want to do.
But you are right that we do not call release on aSource. But this is the copy-CTOR and you may want to let aSource cleanup when it's DTOR is called and don't want to do it right after creating the copy.

Comment 47

[Dirk Schulze]

(In reply to Dirk Schulze from comment #46)
> ReleaseRef releases the pointers from the new created nsStyleFilter. Since
> there are no pointers before creating them, this is certainly not what you
> want to do.
> But you are right that we do not call release on aSource. But this is the
> copy-CTOR and you may want to let aSource cleanup when it's DTOR is called
> and don't want to do it right after creating the copy.

Sorry, understand your argument about uninitialized mType now. I wondered why no assertion was fired, but the pointers in the union weren't initialized either. So the assertions were meaningless :/

Comment 48

[Dirk Schulze]

Attachment: drop-shadow-moz.patch

With initializing of member variables.

Comment 49

[Cameron McCormack (:heycam)]

Third time lucky. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/62a4e3707e1c

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/62a4e3707e1c