1935471 - Crash in [@ mozilla::StyleCssUrl::LoadData] due to CSS <url> custom property substitution off the main thread

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Low frequency crash which has existed for some time.

Crash report: https://crash-stats.mozilla.org/report/index/6d58d526-5366-434e-abee-311210241204

MOZ_CRASH Reason:

MOZ_DIAGNOSTIC_ASSERT(NS_IsMainThread() || dom::IsCurrentThreadRunningWorker())

Top 10 frames:

0  xul.dll  mozilla::StyleCssUrl::LoadData() const  layout/style/ServoStyleConstsInlines.h:385
1  xul.dll  mozilla::StyleCssUrl::GetURI() const  layout/style/ServoStyleConstsInlines.h:396
2  xul.dll  mozilla::StyleComputedUrl::GetURI() const  layout/style/ServoStyleConstsInlines.h:428
2  xul.dll  Gecko_GetComputedImageURLSpec(mozilla::StyleComputedUrl const*, nsTString<cha...  layout/style/GeckoBindings.cpp:1119
3  xul.dll  style::gecko::url::ComputedUrl::serialize_with<alloc::string::String>(void (*...  servo/components/style/gecko/url.rs:347
4  xul.dll  style::gecko::url::impl$19::to_css(style::gecko::url::ComputedUrl*, style_tra...  servo/components/style/gecko/url.rs:359
4  xul.dll  style::properties_and_values::value::impl$44::to_css<style::values::computed:...  servo/components/style/properties_and_values/value.rs:94
5  xul.dll  style::properties_and_values::value::impl$18::to_css(enum2$<style::properties...  servo/components/style/properties_and_values/value.rs:239
5  xul.dll  style_traits::values::CssWriter<alloc::string::String>::new(alloc::string::St...  servo/components/style/properties_and_values/value.rs:203
5  xul.dll  style_traits::values::ToCss::to_css_string(style::properties_and_values::valu...  servo/components/style_traits/values.rs:95

Comment 1

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:jwatt, could you have a look please?

For more information, please visit BugBot documentation.

Comment 2

[BugBot [:suhaib / :marco]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 10 content process crashes on beta

For more information, please visit BugBot documentation.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Test-case

Some crashes look like bad hardware but there's a real bug here. Test-case attached.

Relevant code is here

If we substitute <url> custom properties off the main thread we can hit this assert.

I think this is at most a leak as it stands, but it can cause correctness issues if multiple threads race to the same url data (one could incorrectly get an invalid URI).

I think we'd need to make StyleCssUrl::GetURI use some sort of synchronization to make it thread-safe.

Zach, I don't know if you happen to have cycles for this? I think the easiest fix is to make this assignment an atomic compare-and-swap loop, and tweak the flag setup so it also uses atomic or / and with the right memory ordering.

If you don't have the cycles ni? me and I can try to pick it up asap.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Tagging as security sensitive just in case I've missed anything and since this is technically a race. But per the links in comment 3 it seems it's just a leak at most. Tom, mind sanity-checking?

Comment 5

[Tess Hoffman [:zrhoffman]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Created attachment 9460614 [details]
Zach, I don't know if you happen to have cycles for this? I think the easiest fix is to make this assignment an atomic compare-and-swap loop, and tweak the flag setup so it also uses atomic or / and with the right memory ordering.

If you don't have the cycles ni? me and I can try to pick it up asap.

I do have the cycles and will start on it right away, but feel free to pick it up if I'm taking too long for any reason.

Comment 6

[Tess Hoffman [:zrhoffman]]

Sorry, I misunderstood the scope of this, and I do not have cycles. Emilio, would you please pick it up?

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1935471 - Make resolving image urls thread-safe. r=#style

We can resolve them off the main thread now due to registered custom
properties (which requires the computed <url> serialization).

Comment 8

[Andrew McCreight [:mccr8]]

Races in refcounting an object could lead to use-after-frees. The fact that this is a top crash means the race can be triggered without too much difficulty. I'll leave it as sec-moderate because I don't know how exploitable it really is.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Yeah the nice thing is that this is not a smart pointer, turns out. As in, there's no Release() in the assignment at all, so at worst it can be a leak AIUI.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

What's the right time to add a reminder to add a test for this bug? The test-case above is trivial-ish

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2ad43996a02
Make resolving image urls thread-safe. r=dshin

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #10)

What's the right time to add a reminder to add a test for this bug? The test-case above is trivial-ish

135 ships on Feb 4, so assuming you can get this on beta, maybe a week after that?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2ad43996a02

Comment 14

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox135 to wontfix.

For more information, please visit BugBot documentation.

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9460692 [details]
Bug 1935471 - Make resolving image urls thread-safe. r=#style

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Reproducible crash
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Load test-case
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively straight-forward / targeted fix.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9460692 [details]
Bug 1935471 - Make resolving image urls thread-safe. r=#style

Approved for 135.0rc1 and 128.7esr.

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/80492a52d063

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/141ec7cb3511

Comment 19

[Bogdan Maris, Desktop Test Engineering]

Reproduced the crash using an old Beta build 135.0b6 (https://crash-stats.mozilla.org/report/index/ec52e369-0e20-49c9-9fcc-6b19e0250128) verified that using latest RC 135.0 build, 128.7.0esr and latest Nightly 136.0a1 the testcase does not crash anymore across platforms (Windows 11, macOS 13 and Ubuntu 22.04).

Comment 20

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1935471 - Add crashtest. r=#layout

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/59ef2812d34f
Add crashtest. r=layout-reviewers,jfkthame

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/59ef2812d34f

Comment 23

[BugBot [:suhaib / :marco]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)