1321176 - stylo: Assertion failure: aURLString, at Gecko_SetMozBinding

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Hit this assertion while running test_value_cloning.html tests.

The issue is that Servo may pass null pointer to Gecko_SetMozBinding. I don't know why the assertion was written like this. SpecifiedUrl::as_slice_components() explicitly returns null when its resolve url is null.

The stack:

Assertion failure: aURLString, at c:/mozilla-source/stylo/layout/style/ServoBindings.cpp:697
#01: style::gecko_properties::GeckoBox::set__moz_binding (C:\mozilla-source\stylo\servo\components\style\lib.rs:159)
#02: style::properties::longhands::_moz_binding::cascade_property::{{closure}} (C:\mozilla-source\stylo\servo\components\style\lib.rs:153)
#03: style::properties::substitute_variables__moz_binding<closure> (C:\mozilla-source\stylo\servo\components\style\lib.rs:153)
#04: style::properties::longhands::_moz_binding::cascade_property (C:\mozilla-source\stylo\servo\components\style\lib.rs:153)
#05: style::properties::apply_declarations::{{closure}}<closure,core::iter::FlatMap<core::slice::Iter<(style::rule_tree::StyleSourceGuard, style::properties::declaration_block::Importance)>, core::iter::FilterMap<core::iter::Rev<core::slice::Iter<(style::prope (C:\mozilla-source\stylo\servo\components\style\lib.rs:153)
#06: style::gecko_properties::ComputedValues::do_cascade_property<closure> (C:\mozilla-source\stylo\servo\components\style\lib.rs:159)
#07: style::properties::apply_declarations<closure,core::iter::FlatMap<core::slice::Iter<(style::rule_tree::StyleSourceGuard, style::properties::declaration_block::Importance)>, core::iter::FilterMap<core::iter::Rev<core::slice::Iter<(style::properties::Proper (C:\mozilla-source\stylo\servo\components\style\lib.rs:153)
#08: style::properties::cascade (C:\mozilla-source\stylo\servo\components\style\lib.rs:153)
#09: style::matching::PrivateMatchMethods::cascade_node_pseudo_element<style::gecko::wrapper::GeckoElement,style::gecko::context::StandaloneStyleContext> (C:\mozilla-source\stylo\servo\components\style\matching.rs:424)
#10: style::matching::MatchMethods::cascade_node<style::gecko::wrapper::GeckoElement,style::gecko::context::StandaloneStyleContext> (C:\mozilla-source\stylo\servo\components\style\matching.rs:751)
#11: style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::context::StandaloneStyleContext,style::gecko::traversal::RecalcStyleOnly> (C:\mozilla-source\stylo\servo\components\style\traversal.rs:357)
#12: style::gecko::traversal::{{impl}}::process_preorder (C:\mozilla-source\stylo\servo\components\style\gecko\traversal.rs:32)
#13: style::parallel::top_down_dom::{{closure}}<style::gecko::wrapper::GeckoNode,style::gecko::traversal::RecalcStyleOnly> (C:\mozilla-source\stylo\servo\components\style\parallel.rs:97)
#14: rayon::scope::{{impl}}::execute_job_closure::{{closure}}<closure> (C:\mozilla-source\stylo\third_party\rust\rayon\src\scope\mod.rs:316)
#15: std::panic::{{impl}}::call_once<(),closure> (C:\bot\slave\stable-dist-rustc-win-msvc-64\build\src\libstd\panic.rs:254)
#16: std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> (C:\bot\slave\stable-dist-rustc-win-msvc-64\build\src\libstd\panicking.rs:352)
#17: _rust_maybe_catch_panic[c:\mozilla-source\stylo\obj-firefox-stylo\dist\bin\xul.dll +0x6aa9ac]

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

A testcase showing this crash.

It is only reproducible with the data uri stylesheet, so I guess it is an issue that Servo fails to resolve the URL because its base url is a data url, and consequently it passes null to Gecko.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Actually all callsites of SpecifiedUrl::as_slice_components() seems to be broken in this case.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Gah, this one is my fault.

Before that we stored just "about:invalid", instead of None when we found an invalid URL, so the pointer could never be null. 

When I changed to keep track of the url and if it was empty or not, I should've expected this breakage, sorry.

I think it's probably fine if we use null to signal an invalid url, though maybe we shouldn't be calling Gecko_SetMozBinding in that case anyway?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Gecko_SetMozBinding is actually my least concern one. It is easy to fix, because Gecko just gives it null if the url is invalid, and this property is mainly used internally so even changing behavior is unlikely breaking too many things.

I'm more concernted about what should we do for other callsites of as_slice_components(). They are mainly for images, and we create nsStyleImageRequest for them. It seems to me Gecko doesn't drop invalid URL for images at computation time, so we probably shouldn't do that either, unless the spec says something different. So CreateStyleImageRequest needs to take care of invalid URLs.

Comment 5

[Cameron McCormack (:heycam)]

Until a short time ago, we did drop URLs that were invalid, and set null for the imgRequestProxy.  Now with nsStyleImageRequest, we maintain the invalid URL, which lets us return the correct value from getComputedStyle.  CSS Values and Units says to use the specified url() value as the computed value if a relative URL cannot be resolved against the base:

https://www.w3.org/TR/css3-values/#relative-urls

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1321176 - Handle unresolved url value gracefully.

This changes as_slice_components() to return the original url if the url
is not resolved. It returns Ok() vs. Err() to distinguish between the
two cases because we cannot assign an unresolvable url to -moz-bindings.

Review commit: https://reviewboard.mozilla.org/r/99706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99706/

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8820171 [details]
Bug 1321176 - Handle unresolved url value gracefully.

https://reviewboard.mozilla.org/r/99706/#review100086

In principle it would be nice if we could treat -moz-binding with unresolvable URLs the same as other URL-typed properties, but since this is a non-standard property I think it's fine to just set it to null.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8820171 [details]
Bug 1321176 - Handle unresolved url value gracefully.

https://reviewboard.mozilla.org/r/99706/#review100094

::: servo/components/style/values/specified/image.rs:52
(Diff revision 1)
>      }
>  
>      /// Creates an already specified image value from an already resolved URL
>      /// for insertion in the cascade.
> +    #[cfg(feature = "servo")]
>      pub fn for_cascade(url: Option<ServoUrl>, extra_data: UrlExtraData) -> Self {

I think the idea is that we're going to need this eventually for animations.

::: servo/components/style/values/specified/url.rs:127
(Diff revision 1)
>      /// Little helper for Gecko's ffi.
> -    pub fn as_slice_components(&self) -> (*const u8, usize) {
> +    #[cfg(feature = "gecko")]
> +    pub fn as_slice_components(&self) -> Result<(*const u8, usize), (*const u8, usize)> {
>          match self.resolved {
> -            Some(ref url) => (url.as_str().as_ptr(), url.as_str().len()),
> -            None => (ptr::null(), 0),
> +            Some(ref url) => Ok((url.as_str().as_ptr(), url.as_str().len())),
> +            None => match self.original {

But in that case we should always inject valid URLs into the cascade, so I think you could make `for_cascade` take directly a `ServoUrl` (I don't know why I wrote it with `Option` actually?) and use `self.original.expect("We should always have either the original or the resolved value!")` instead.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Change SpecifiedUrl::for_cascade() to take ServoUrl.

Review commit: https://reviewboard.mozilla.org/r/99748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99748/

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8820171 [details]
Bug 1321176 - Handle unresolved url value gracefully.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99706/diff/1-2/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8820225 [details]
Change SpecifiedUrl::for_cascade() to take ServoUrl.

https://reviewboard.mozilla.org/r/99748/#review100220

Thanks :)

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8820171 [details]
Bug 1321176 - Handle unresolved url value gracefully.

https://reviewboard.mozilla.org/r/99706/#review100230

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70e65619c68e
Handle unresolved url value gracefully. r=heycam,manishearth

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

https://github.com/servo/servo/pull/14653

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/70e65619c68e