1328223 - stylo: layout/style/crashtests/498036-1.html crashes due to invalid URL

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

Servo and Gecko have different ideas about what it means to be a valid URL.  In layout/style/crashtests/498036-1.html, we try to @import the URL "data:css".  This parses OK in Servo, but Gecko's NS_NewURI fails.  This causes the MOZ_ALWAYS_SUCCEEDS in Gecko_LoadStyleSheet to assert:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a0b78462186a67395041d8ab509cba8b7dc7716&selectedJob=65710460

It looks like we can just return early and do nothing for an invalid URL in Gecko_LoadStyleSheet.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8823188 [details] [diff] [review]
patch

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

Gah, I expected this to not happen given the rust url oxidation patches (that basically try to unify it).

Can you leave a comment there saying that eventually it should be MOZ_ALWAYS_SUCCEEDS?

::: layout/style/ServoBindings.cpp
@@ +1060,2 @@
>  
> +  if (NS_FAILED(rv)) {

Also, you can annotate this with MOZ_UNLIKELY, though maybe this is not hot enough to be worth?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

CC'ing Manish and Valentin (this is a divergence between Gecko's URL and rust-url).

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> Gah, I expected this to not happen given the rust url oxidation patches
> (that basically try to unify it).

Gecko still uses various concrete URL classes for different protocols, e.g. for data: vs http: vs about: vs res:, so I expect we're still beholden to the kinds of URL strings they accept or reject.

> Also, you can annotate this with MOZ_UNLIKELY, though maybe this is not hot
> enough to be worth?

I think it's not worth it, since it only runs once when processing an @import rule.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38d1799bb189
stylo: Ignore invalid URLs from @import rules. r=emilio

Comment 6

[Valentin Gosu [:valentin]]

Thanks for fixing this. Manish and I are currently working on a bunch of issues that prevent us from replacing all of the individual URI parsers with rust-url. I trust this would fix will suffice until then.

Comment 7

[Manish Goregaokar [:manishearth]]

> CC'ing Manish and Valentin (this is a divergence between Gecko's URL and rust-url).

rust-url is correct here, and since this is a divergence where Gecko (incorrectly) fails to parse, it's not something that matters as much for oxidation (would be nice to fix on the gecko side, but not really necessary).

> given the rust url oxidation patches

off by default :)

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/38d1799bb189