Closed Bug 913990 Opened 6 years ago Closed 5 years ago

"ASSERTION: expected pointer" with weird URL in filter

Categories

(Core :: CSS Parsing and Computation, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jruderman, Assigned: mvujovic)

References

(Blocks 1 open bug)

Details

(4 keywords)

Crash Data

Attachments

(4 files, 3 obsolete files)

Attached file testcase
###!!! ASSERTION: expected pointer: 'aURL', file layout/style/nsStyleStruct.cpp, line 1094

This assertion was added in bug 898361.
Dirk, can you look at this?
Flags: needinfo?(krit)
Attached file stack
After asserting, it crashes [@ nsStyleFilter::SetURL]

bp-7e46c563-7d13-4db4-b694-c43b62130909
Crash Signature: [@ nsStyleFilter::SetURL]
Keywords: crash
I am flying today and in a conference tomorrow. I try to keep a look at it. Also adding Max. From the first view:

(In reply to Jesse Ruderman from comment #3)
> After asserting, it crashes [@ nsStyleFilter::SetURL]
> 
> bp-7e46c563-7d13-4db4-b694-c43b62130909

nsRuleNode::SetStyleFilterToCSSValue(nsStyleFilter* aStyleFilter,
                                     const nsCSSValue& aValue,
...
  nsCSSUnit unit = aValue.GetUnit();
  if (unit == eCSSUnit_URL) {
    aStyleFilter->SetURL(aValue.GetURLValue());

Seems like the unit of CSSValue is set to URL, even though the value is invalid. Is that supposed to be that way? It looks like we expect values to be valid at this point.
Flags: needinfo?(krit)
(In reply to Dirk Schulze from comment #4)
> Seems like the unit of CSSValue is set to URL, even though the value is
> invalid. Is that supposed to be that way? It looks like we expect values to
> be valid at this point.

Looks like nsCSSValue just stores a string representation of the URL, without actually having tried to create a nsIURI value from it.  When we call actually call GetURLValue() on it (in the code quoted in comment 4), then that makes us actually call NS_NewURI for that string (inside of css::URLValue::GetURI), and that produces a null nsIURI, since we're now finding out that the string isn't a valid URI.  So GetURLValue ends up returning null.
Ok, I guess the question is then if we should assume that GetURLValue() can return null. It seems like yes. We just clarified in the SVG WG that we treat the filter chain as invalid if URL does not exist. I am not sure though if that just affects rendering, or computed style. If the latter, we would need to make this function nsRuleNode::SetStyleFilterToCSSValue return a bool and don't add an styleFilter element if the URL is not valid.
Attached patch 913990-1.patch (obsolete) — Splinter Review
I made a patch to do Dirk's second suggestion, which sounded reasonable to me:
"...make this function nsRuleNode::SetStyleFilterToCSSValue return a bool and don't add an styleFilter element if the URL is not valid"
Assignee: nobody → mvujovic
Attachment #813335 - Flags: review?(dholbert)
Attachment #813335 - Flags: feedback?(krit)
The strange URL from the test case is probably an edge case, but I wonder if it would be better to treat these invalid URLs the same as if the URL itself was valid but didn't point to an SVG <filter> element.  We ignore the entire filter chain if there is a url() that doesn't point to a <filter> element, is that right?
(In reply to Cameron McCormack (:heycam) from comment #8)
> The strange URL from the test case is probably an edge case, but I wonder if
> it would be better to treat these invalid URLs the same as if the URL itself
> was valid but didn't point to an SVG <filter> element.  We ignore the entire
> filter chain if there is a url() that doesn't point to a <filter> element,
> is that right?

We don't do anything right now, since we don't render filter chains. However, I think you're right- when there is an URL that doesn't point to a filter element, the entire filter chain should not apply. 

So, I suppose when we have:
filter: blur(10px) url("feed:javascript:5") grayscale(1.0);

We should return the same string for the computed style, and not apply the filter chain.

Unrelated to this bug, current and previous behavior in Firefox is that the filtered element does not render with a filter URL that doesn't point to a filter element. I added "filter: url(#a);" to an element and it caused the element to disappear in Firefox 3 as well as in Firefox 24. Chrome ignores the filter and renders the element. I think that behavior makes more sense.
Comment on attachment 813335 [details] [diff] [review]
913990-1.patch

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>+      for (const nsCSSValueList* cur = filterValue->GetListValue();
>+           cur;
>+           cur = cur->mNext) {
>         nsStyleFilter styleFilter;
>-        SetStyleFilterToCSSValue(&styleFilter, cur->mValue, aContext,
>-                                 mPresContext, canStoreInRuleTree);
>+        if (!SetStyleFilterToCSSValue(&styleFilter, cur->mValue, aContext,
>+                                      mPresContext, canStoreInRuleTree))
>+          continue;

Please add braces around this "if" body. (around the "continue")

Also: does this leave us with a partly-constructed value in svgReset->mFilters? It looks like it does, which seems undesirable.

I imagine we need to end up doing one of the following here:

  a) insert a "none" value of some sort into the filter list, in place of the bogus URI (This is what happens for "background-image" if I put this bogus image URI in a "background-image" list, FWIW)
or...
  b) preserve the URI as-specified, even though we don't honor it
or...
  c) Make the whole filter value fail to parse

I'm not sure which option makes the most sense and is most spec-friendly, though I'm currently leaning slightly towards "c".  To get that behavior reliably, you'd probably want to replace "continue" with "break", plus code to ensure that svgReset->mFilters is the same as if we hadn't touched it. (maybe add another Clear() call?)  Not 100% sure that's the correct route, though.
It depends if url() with arbitrary string is valid or not during parsing. From the parsing point of view it has nothing to do with filters.

1) url(script:alert) valid
    It needs to be preserved and be reflected in the computed style
2) url(script:alert) invalid
    The whole filter fails parsing and you end up with 'none'.

I definitely dislike a).

It is a different question what happens with the filter operations should a url() with arbitrary strings be valid. We agreed on the SVG WG that we do not execute the whole filter operation list. The computed style would not be affected. (As it isn't for a non existent fragment identifier: url(#isNotThere).)
url() parsing follows RFC3986 according to CSS 2.1.

Means it depends on http://www.ietf.org/rfc/rfc3986 if a URL is valid. (Or the new URL spec from Anne.)
Comment on attachment 813335 [details] [diff] [review]
913990-1.patch

Did the comment help Max?
Attachment #813335 - Flags: feedback?(krit)
Yes, thanks Dirk and Daniel!

Now I'm wondering whether "feed:javascript:5" is a valid URL. Based on a brief reading of http://www.ietf.org/rfc/rfc3986, I believe it is. There is an example of a valid URL, "urn:example:animal:ferret:nose", which is very similar to "feed:javascript:5". I've quoted the spec text below from section "3.  Syntax Components":

"""
   The following are two example URIs and their component parts:

         foo://example.com:8042/over/there?name=ferret#nose
         \_/   \______________/\_________/ \_________/ \__/
          |           |            |            |        |
       scheme     authority       path        query   fragment
          |   _____________________|__
         / \ /                        \
         urn:example:animal:ferret:nose
"""

What do you guys think?
(In reply to Dirk Schulze from comment #11)
> It depends if url() with arbitrary string is valid or not during parsing.
> From the parsing point of view it has nothing to do with filters.
> 
> 1) url(script:alert) valid
>     It needs to be preserved and be reflected in the computed style
> 2) url(script:alert) invalid
>     The whole filter fails parsing and you end up with 'none'.

Yes, I think we should do one of these two depending on whether "feed:javascript:5" is valid.
Thanks for looking, Daniel!

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 813335 [details] [diff] [review]
> 913990-1.patch
> 
> >diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
> >+      for (const nsCSSValueList* cur = filterValue->GetListValue();
> >+           cur;
> >+           cur = cur->mNext) {
> >         nsStyleFilter styleFilter;
> >-        SetStyleFilterToCSSValue(&styleFilter, cur->mValue, aContext,
> >-                                 mPresContext, canStoreInRuleTree);
> >+        if (!SetStyleFilterToCSSValue(&styleFilter, cur->mValue, aContext,
> >+                                      mPresContext, canStoreInRuleTree))
> >+          continue;
> 
> Please add braces around this "if" body. (around the "continue")

Will do. I'll remember this style.

> 
> Also: does this leave us with a partly-constructed value in
> svgReset->mFilters? It looks like it does, which seems undesirable.

Yes, you're right. This is undesirable.

> 
> I imagine we need to end up doing one of the following here:
> 
>   a) insert a "none" value of some sort into the filter list, in place of
> the bogus URI (This is what happens for "background-image" if I put this
> bogus image URI in a "background-image" list, FWIW)
> or...
>   b) preserve the URI as-specified, even though we don't honor it
> or...
>   c) Make the whole filter value fail to parse
> 
> I'm not sure which option makes the most sense and is most spec-friendly,
> though I'm currently leaning slightly towards "c".  To get that behavior
> reliably, you'd probably want to replace "continue" with "break", plus code
> to ensure that svgReset->mFilters is the same as if we hadn't touched it.
> (maybe add another Clear() call?)  Not 100% sure that's the correct route,
> though.

Yes, I think we should do either (b) or (c), which are equivalent to Dirk's (1) and (2). If it's (c)/(2), then yes, I think we need to replace "continue" with "break" and call Clear() on svgReset->mFilters.
(In reply to Max Vujovic from comment #14)
> Yes, thanks Dirk and Daniel!
> 
> Now I'm wondering whether "feed:javascript:5" is a valid URL. Based on a
> brief reading of http://www.ietf.org/rfc/rfc3986, I believe it is. There is
> an example of a valid URL, "urn:example:animal:ferret:nose", which is very
> similar to "feed:javascript:5". I've quoted the spec text below from section
> "3.  Syntax Components":
> 
> """
>    The following are two example URIs and their component parts:
> 
>          foo://example.com:8042/over/there?name=ferret#nose
>          \_/   \______________/\_________/ \_________/ \__/
>           |           |            |            |        |
>        scheme     authority       path        query   fragment
>           |   _____________________|__
>          / \ /                        \
>          urn:example:animal:ferret:nose
> """
> 
> What do you guys think?

FYI, I'm trying to get a hold of a URL expert like Anne van Kesteren or Larry Masinter to double check.
In the IETF specs, the generic syntax of URIs is very forgiving, but delegates the actual allowed syntax to the (registered) scheme definition. And "feed" isn't registered, but it should be (please do so):

http://en.wikipedia.org/wiki/Feed_URI_scheme 

claims that
<feed_uri> ::= "feed:" <absolute_uri> | "feed://" <authority> <path-abempty>

and so feed:javascript:5 is legal if javascript:5 is an absolute URI.

Now "javascript:" is also not registered, but 
http://tools.ietf.org/html/draft-hoehrmann-javascript-scheme-03 
seems like the latest I can find without a lot of work. I *think* "javascript:5" is legal, but I'm not sure what it means to return the javascript source text "5", or what that would mean when passed to "feed". Even if it were syntactically legal, it makes no sense computationally, and might as well be syntactically incorrect.
Even if feed:javascript:5 is legal, I don't think we want to get that to work in this bug.

(In reply to Max Vujovic from comment #9)
> So, I suppose when we have:
> filter: blur(10px) url("feed:javascript:5") grayscale(1.0);
> 
> We should return the same string for the computed style, and not apply the
> filter chain.

I guess this is going to be a problem, since the computed style is going to be based on the nsIURI*, and if that is null because we fail to create an object for feed:javascript:5, we're not going to have anything to serialise.  That makes me lean towards making the computed value of the entire property in this case "none".  This will have the same visual effect as if we were able to preserve feed:javascript:5 and notice that it doesn't point to a <filter> element.

As I said above, this is an edge case and I don't think it's terribly important to make it do the right thing.
Yeah, I agree with heycam; I think it's probably more work than it's worth to try to preserve the plaintext of the URI, when it fails to parse to a nsIURI. (and to determine which URIs are "valid" enough to get that treatment, vs. which are not)

The simplest step towards non-crashy behavior is just to just make the whole thing fail (per my suggestion (c), dirk's (2)). Let's just go with that.
Attachment #813335 - Flags: review?(dholbert) → feedback+
Attached patch 913990-2.patch (obsolete) — Splinter Review
Attachment #813335 - Attachment is obsolete: true
Attachment #817946 - Flags: review?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #20)
> The simplest step towards non-crashy behavior is just to just make the whole
> thing fail (per my suggestion (c), dirk's (2)). Let's just go with that.

I agree. I've uploaded a new patch with that behavior.
Comment on attachment 817946 [details] [diff] [review]
913990-2.patch

Looks great! Just some prose nits (& one testcase-addition that would be nice):

>Handle null value from CSSValue->GetURLValue() in CSS Filter parsing.

Let's clarify that commit message to be something like:
Bug 913990: When encountering bogus URI during style computation for "filter", fall back to initial value. r=dholbert

>+++ b/layout/style/test/property_database.js
>@@ -4437,16 +4437,20 @@ if (SpecialPowers.getBoolPref("layout.cs
>+			// The CSS parser will accept this weird URL, but it will appear
>+			// as none in the computed style.
>+			"url('feed:javascript:5')",

Two things:
 (1) Let's clarify this comment a bit, to make it less mysterious.  Replace "it will appear as none in the computed style" with something like "we'll fail to resolve it when computing style, so we'll fall back to the initial value ('none')"

 (2) Can you include a value with this in a list of filters, to exercise the "svgReset->mFilters.Clear(); break;" that you added in this patch version?

>+++ b/layout/style/test/test_value_computation.html
> var gBadComputed = {
>+  // The CSS parser will accept this weird URL, but it will appear as none in
>+  // the computed style.
>+  "filter": [ "url('feed:javascript:5')" ],

The same comment-tweak from (1) above applies here.

r=me with
Attachment #817946 - Flags: review?(dholbert) → review+
*with that
Attached patch 913990-3.patchSplinter Review
Attachment #817946 - Attachment is obsolete: true
Thanks for the review!

(In reply to Daniel Holbert [:dholbert] from comment #23)
> Comment on attachment 817946 [details] [diff] [review]
> 913990-2.patch
> 
> Looks great! Just some prose nits (& one testcase-addition that would be
> nice):
> 
> >Handle null value from CSSValue->GetURLValue() in CSS Filter parsing.
> 
> Let's clarify that commit message to be something like:
> Bug 913990: When encountering bogus URI during style computation for
> "filter", fall back to initial value. r=dholbert

Done. That's much better.

> 
> >+++ b/layout/style/test/property_database.js
> >@@ -4437,16 +4437,20 @@ if (SpecialPowers.getBoolPref("layout.cs
> >+			// The CSS parser will accept this weird URL, but it will appear
> >+			// as none in the computed style.
> >+			"url('feed:javascript:5')",
> 
> Two things:
>  (1) Let's clarify this comment a bit, to make it less mysterious.  Replace
> "it will appear as none in the computed style" with something like "we'll
> fail to resolve it when computing style, so we'll fall back to the initial
> value ('none')"

Done. Also much better.

> 
>  (2) Can you include a value with this in a list of filters, to exercise the
> "svgReset->mFilters.Clear(); break;" that you added in this patch version?

Done. I've added "blur(3px) url('feed:javascript:5') grayscale(50%)".

> 
> >+++ b/layout/style/test/test_value_computation.html
> > var gBadComputed = {
> >+  // The CSS parser will accept this weird URL, but it will appear as none in
> >+  // the computed style.
> >+  "filter": [ "url('feed:javascript:5')" ],
> 
> The same comment-tweak from (1) above applies here.

Done.

> 
> r=me with

I'll go ahead and push this to try, and if all goes well, I'll set the checkin-needed flag.
Try results look good. Setting checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fa7709266585
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Re-opening this bug since the test added in the landed patch fails on B2G when filters is enabled [1].

For filter URLs like “feed:”...

In Firefox, nsCSSValue::GetURLValue creates a null nsIURI. This eventually results in a computed style of “none” for the filter property.

In B2G, nsCSSValue::GetURLValue creates a non-null nsIURI. This eventually results in a computed style matching the original “feed:” URL.

dbaron suggested [2] the difference may be that Firefox includes a feed protocol handler [3], which B2G does not.

I don’t think the computed style for this edge case really matters, so I suggest we turn the “feed:” computed style test into a crashtest in order to ignore this platform difference. We’re actually interested in whether this case asserts or not. I’ll post a follow-up patch doing that.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1057180#c18
[2]: https://bugzilla.mozilla.org/show_bug.cgi?id=1057180#c19
[3]: http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/
Blocks: 1057180
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The previous comment describes this patch.
Attachment #8499037 - Flags: review?(dbaron)
Comment on attachment 8499037 [details] [diff] [review]
Patch: Change computed style test to crashtest

You may as well have a test in the initial_values list for this case, though.  If the feed: URL doesn't behave the same across platforms, then use the two tests you have replacing "feed:javascript:5" with "badscheme:badurl" or something like that.  Or is there a reason that doesn't work?

r=dbaron with that
Attachment #8499037 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #33)
> Comment on attachment 8499037 [details] [diff] [review]
> Patch: Change computed style test to crashtest
> 
> You may as well have a test in the initial_values list for this case,
> though.  If the feed: URL doesn't behave the same across platforms, then use
> the two tests you have replacing "feed:javascript:5" with "badscheme:badurl"
> or something like that.  Or is there a reason that doesn't work?

Good idea! I think that should work. I'll update the patch.
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #33)
> Comment on attachment 8499037 [details] [diff] [review]
> Patch: Change computed style test to crashtest
> 
> You may as well have a test in the initial_values list for this case,
> though.  If the feed: URL doesn't behave the same across platforms, then use
> the two tests you have replacing "feed:javascript:5" with "badscheme:badurl"
> or something like that.  Or is there a reason that doesn't work?

Actually, I need to put the "badscheme:badurl" tests in the other_values list, not in the initial_values list.

For the filter property, when Firefox doesn’t specifically handle the scheme, we get the full URL back in the computed style.

Some other CSS properties behave like filter:
[CSS] clip-path: url(“badscheme:badurl”);
[getComputedStyle] clip-path: url(“badscheme:badurl”)

Other CSS properties behave differently and don’t return URLs with bad schemes in computed style:
[CSS] background-image: url(“badscheme:badurl”);
[getComputedStyle] none

background-image goes through the added step of getting an nsStyleImage and serializing that [1], which results in “none” if the URL used an invalid scheme. clip-path and filter just store and serialize the nsIURI directly [2].

Does it sound ok to put “badscheme:badurl” in the other_values list instead of initial_values?

[1]: http://dxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#2033
[2]: http://dxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#5276
Flags: needinfo?(dbaron)
(In reply to Max Vujovic from comment #35)
> Does it sound ok to put “badscheme:badurl” in the other_values list instead
> of initial_values?

If tests pass that way, sure.
Flags: needinfo?(dbaron)
https://hg.mozilla.org/mozilla-central/rev/7fa34faa8330
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
I think the problem is that the notion of a 'valid' URL is not static, and you shouldn't be depending on it.

Almost any string could be a 'valid' URL in some sense, and whether a URL is valid is dynamically changeable, in a variety of ways, including RegisterProtocolHandler.

The best is to not rely on 'valid' at all.
You need to log in before you can comment on or make changes to this bug.