Closed Bug 1321176 Opened 3 years ago Closed 3 years ago

stylo: Assertion failure: aURLString, at Gecko_SetMozBinding

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(3 files)

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]
Attached file 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.
Actually all callsites of SpecifiedUrl::as_slice_components() seems to be broken in this case.
Blocks: 1288302
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?
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.
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
Assignee: nobody → xidorn+moz
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.
Attachment #8820171 - Flags: review?(cam) → review+
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 on attachment 8820225 [details]
Change SpecifiedUrl::for_cascade() to take ServoUrl.

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

Thanks :)
Attachment #8820225 - Flags: review?(emilio+bugs) → review+
Comment on attachment 8820171 [details]
Bug 1321176 - Handle unresolved url value gracefully.

https://reviewboard.mozilla.org/r/99706/#review100230
Attachment #8820171 - Flags: review?(manishearth) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70e65619c68e
Handle unresolved url value gracefully. r=heycam,manishearth
https://hg.mozilla.org/mozilla-central/rev/70e65619c68e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.