Closed
Bug 913990
Opened 11 years ago
Closed 10 years ago
"ASSERTION: expected pointer" with weird URL in filter
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: jruderman, Assigned: mvujovic)
References
Details
(4 keywords)
Crash Data
Attachments
(4 files, 3 obsolete files)
88 bytes,
text/html
|
Details | |
8.83 KB,
text/plain
|
Details | |
6.92 KB,
patch
|
Details | Diff | Splinter Review | |
3.00 KB,
patch
|
Details | Diff | Splinter Review |
###!!! ASSERTION: expected pointer: 'aURL', file layout/style/nsStyleStruct.cpp, line 1094 This assertion was added in bug 898361.
Reporter | ||
Comment 2•11 years ago
|
||
Reporter | ||
Comment 3•11 years ago
|
||
After asserting, it crashes [@ nsStyleFilter::SetURL] bp-7e46c563-7d13-4db4-b694-c43b62130909
Crash Signature: [@ nsStyleFilter::SetURL]
Keywords: crash
Comment 4•11 years ago
|
||
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)
Comment 5•11 years ago
|
||
(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.
Comment 6•11 years ago
|
||
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.
Assignee | ||
Comment 7•11 years ago
|
||
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)
Comment 8•11 years ago
|
||
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?
Assignee | ||
Comment 9•11 years ago
|
||
(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 10•11 years ago
|
||
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.
Comment 11•11 years ago
|
||
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).)
Comment 12•11 years ago
|
||
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 13•11 years ago
|
||
Comment on attachment 813335 [details] [diff] [review] 913990-1.patch Did the comment help Max?
Attachment #813335 -
Flags: feedback?(krit)
Assignee | ||
Comment 14•11 years ago
|
||
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?
Assignee | ||
Comment 15•11 years ago
|
||
(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.
Assignee | ||
Comment 16•11 years ago
|
||
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.
Assignee | ||
Comment 17•11 years ago
|
||
(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.
Comment 18•11 years ago
|
||
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.
Comment 19•11 years ago
|
||
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.
Comment 20•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #813335 -
Flags: review?(dholbert) → feedback+
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #813335 -
Attachment is obsolete: true
Attachment #817946 -
Flags: review?(dholbert)
Assignee | ||
Comment 22•11 years ago
|
||
(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 23•11 years ago
|
||
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+
Comment 24•11 years ago
|
||
*with that
Assignee | ||
Comment 25•11 years ago
|
||
Attachment #817946 -
Attachment is obsolete: true
Assignee | ||
Comment 26•11 years ago
|
||
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.
Assignee | ||
Comment 27•11 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=8b31d85e5136
Assignee | ||
Comment 28•11 years ago
|
||
Try results look good. Setting checkin-needed.
Keywords: checkin-needed
Comment 29•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7709266585
Flags: in-testsuite+
Keywords: checkin-needed
Comment 30•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fa7709266585
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Assignee | ||
Comment 31•10 years ago
|
||
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/
Assignee | ||
Comment 32•10 years ago
|
||
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+
Assignee | ||
Comment 34•10 years ago
|
||
(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.
Assignee | ||
Comment 35•10 years ago
|
||
(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)
Assignee | ||
Comment 37•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa34faa8330 https://tbpl.mozilla.org/?tree=Try&rev=9e0647f42c14
Attachment #8499037 -
Attachment is obsolete: true
Comment 38•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7fa34faa8330
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Comment 39•10 years ago
|
||
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.
Description
•