1502252 - Hit MOZ_CRASH(nsFontCache not thread-safe) at /builds/worker/workspace/build/src/xpcom/base/nsISupportsImpl.cpp:46

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev 3cc04ee79005.

Triggering this issue appears to be a race and the attached testcase is not fully reduced and may require several attempts to trigger.

Hit MOZ_CRASH(nsFontCache not thread-safe) at /builds/worker/workspace/build/src/xpcom/base/nsISupportsImpl.cpp:46

rax = 0x0000560e4d560e80   rdx = 0x0000000000000000
rcx = 0x0000000000000b40   rbx = 0x00007f0fc278a186
rsi = 0x00007f0fcf02f8b0   rdi = 0x00007f0fcf02e680
rbp = 0x00007f0fb47fd130   rsp = 0x00007f0fb47fd120
r8 = 0x00007f0fcf02f8b0    r9 = 0x00007f0fb47ff700
r10 = 0x0000000000000056   r11 = 0x00007f0fcecc07e0
r12 = 0x00007f0fb4326000   r13 = 0x00007f0fb464e100
r14 = 0x00007f0fc1a61e20   r15 = 0x00007f0fb47fd348
rip = 0x0000560e4d323ec2
OS|Linux|0.0.0 Linux 4.15.0-36-generic #39-Ubuntu SMP Mon Sep 24 16:19:09 UTC 2018 x86_64
CPU|amd64|family 6 model 78 stepping 3|1
GPU|||
Crash|SIGSEGV /SEGV_MAPERR|0x0|19
19|0|firefox-bin|MOZ_CrashOOL|hg:hg.mozilla.org/mozilla-central:mfbt/Assertions.cpp:3cc04ee79005058d817daf66da7963dfac3f0a3a|33|0x0
19|1|libxul.so|nsAutoOwningThread::AssertCurrentThreadOwnsMe(char const*) const|hg:hg.mozilla.org/mozilla-central:xpcom/base/nsISupportsImpl.cpp:3cc04ee79005058d817daf66da7963dfac3f0a3a|46|0x14
19|2|libxul.so|nsFontCache::AddRef()|hg:hg.mozilla.org/mozilla-central:xpcom/base/nsISupportsImpl.h:3cc04ee79005058d817daf66da7963dfac3f0a3a|64|0x5
19|3|libxul.so|nsFontCache::GetMetricsFor(nsFont const&, nsFontMetrics::Params const&)|hg:hg.mozilla.org/mozilla-central:mfbt/RefPtr.h:3cc04ee79005058d817daf66da7963dfac3f0a3a|44|0x5
19|4|libxul.so|nsDeviceContext::GetMetricsFor(nsFont const&, nsFontMetrics::Params const&)|hg:hg.mozilla.org/mozilla-central:gfx/src/nsDeviceContext.cpp:3cc04ee79005058d817daf66da7963dfac3f0a3a|297|0x1c
19|5|libxul.so|nsLayoutUtils::GetMetricsFor(nsPresContext*, bool, nsStyleFont const*, int, bool, nsLayoutUtils::FlushUserFontSet)|hg:hg.mozilla.org/mozilla-central:layout/base/nsLayoutUtils.cpp:3cc04ee79005058d817daf66da7963dfac3f0a3a|10141|0x5
19|6|libxul.so|Gecko_GetFontMetrics|hg:hg.mozilla.org/mozilla-central:layout/style/GeckoBindings.cpp:3cc04ee79005058d817daf66da7963dfac3f0a3a|2488|0x25
19|7|libxul.so|<style::gecko::wrapper::GeckoFontMetricsProvider as style::font_metrics::FontMetricsProvider>::query|hg:hg.mozilla.org/mozilla-central:servo/components/style/gecko/wrapper.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|1047|0x9
19|8|libxul.so|style::values::specified::length::FontRelativeLength::to_computed_value|hg:hg.mozilla.org/mozilla-central:servo/components/style/values/specified/length.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|126|0x17
19|9|libxul.so|style::values::computed::length::<impl style::values::computed::ToComputedValue for style::values::specified::calc::CalcLengthOrPercentage>::to_computed_value|hg:hg.mozilla.org/mozilla-central:servo/components/style/values/computed/length.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|274|0xf
19|10|libxul.so|style::values::computed::length::<impl style::values::computed::ToComputedValue for style::values::specified::length::LengthOrPercentage>::to_computed_value|hg:hg.mozilla.org/mozilla-central:servo/components/style/values/computed/length.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|477|0x8
19|11|libxul.so|style::properties::longhands::row_gap::cascade_property|hg:hg.mozilla.org/mozilla-central:servo/components/style/values/generics/mod.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|173|0xb
19|12|libxul.so|style::properties::cascade::Cascade::apply_properties|hg:hg.mozilla.org/mozilla-central:servo/components/style/properties/cascade.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|463|0xd
19|13|libxul.so|style::properties::cascade::cascade_rules|hg:hg.mozilla.org/mozilla-central:servo/components/style/properties/cascade.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|303|0x10
19|14|libxul.so|style::stylist::Stylist::cascade_style_and_visited|hg:hg.mozilla.org/mozilla-central:servo/components/style/properties/cascade.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|93|0x37
19|15|libxul.so|<style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_style_and_visited|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|305|0x2c
19|16|libxul.so|<style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_primary_style|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|215|0x17
19|17|libxul.so|<style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_styles_with_default_parents::{{closure}}|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|335|0x8
19|18|libxul.so|<style::style_resolver::StyleResolverForElement<'a, 'ctx, 'le, E>>::cascade_styles_with_default_parents|hg:hg.mozilla.org/mozilla-central:servo/components/style/style_resolver.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|102|0xb
19|19|libxul.so|style::traversal::compute_style|hg:hg.mozilla.org/mozilla-central:servo/components/style/traversal.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|666|0x5
19|20|libxul.so|<style::gecko::traversal::RecalcStyleOnly<'recalc> as style::traversal::DomTraversal<style::gecko::wrapper::GeckoElement<'le>>>::process_preorder|hg:hg.mozilla.org/mozilla-central:servo/components/style/traversal.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|435|0xb
19|21|libxul.so|<std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once|hg:hg.mozilla.org/mozilla-central:servo/components/style/parallel.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|197|0x20
19|22|libxul.so|std::panicking::try::do_call|git:github.com/rust-lang/rust:src/libstd/panicking.rs:17a9dc7513b9fea883dc9505f09f97c63d1d601b|310|0x18
19|23|libxul.so|__rust_maybe_catch_panic|git:github.com/rust-lang/rust:src/libpanic_abort/lib.rs:17a9dc7513b9fea883dc9505f09f97c63d1d601b|39|0x5
19|24|libxul.so|<rayon_core::job::HeapJob<BODY> as rayon_core::job::Job>::execute|git:github.com/rust-lang/rust:src/libstd/panicking.rs:17a9dc7513b9fea883dc9505f09f97c63d1d601b|289|0x5
19|25|libxul.so|rayon_core::registry::WorkerThread::wait_until_cold|hg:hg.mozilla.org/mozilla-central:third_party/rust/rayon-core/src/job.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|60|0x6
19|26|libxul.so|rayon_core::registry::main_loop|hg:hg.mozilla.org/mozilla-central:third_party/rust/rayon-core/src/registry.rs:3cc04ee79005058d817daf66da7963dfac3f0a3a|543|0x8
19|27|libxul.so|std::panicking::try::do_call|git:github.com/rust-lang/rust:src/libstd/thread/mod.rs:17a9dc7513b9fea883dc9505f09f97c63d1d601b|409|0x5
19|28|libxul.so|__rust_maybe_catch_panic|git:github.com/rust-lang/rust:src/libpanic_abort/lib.rs:17a9dc7513b9fea883dc9505f09f97c63d1d601b|39|0x5
19|29|libxul.so|<F as alloc::boxed::FnBox<A>>::call_box|git:github.com/rust-lang/rust:src/libstd/panicking.rs:17a9dc7513b9fea883dc9505f09f97c63d1d601b|289|0x5
19|30|libxul.so|std::sys_common::thread::start_thread|git:github.com/rust-lang/rust:src/liballoc/boxed.rs:17a9dc7513b9fea883dc9505f09f97c63d1d601b|652|0x3
19|31|libxul.so|std::sys::unix::thread::Thread::new::thread_start|git:github.com/rust-lang/rust:src/libstd/sys/unix/thread.rs:17a9dc7513b9fea883dc9505f09f97c63d1d601b|90|0x5
19|32|libpthread-2.27.so||||0x76db
19|33|libc-2.27.so||||0x12188f

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Ugh, thank you.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(I think this should be s-s for the time being)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

How much time does it take for you to repro? I couldn't repro on a debug build for a while. I'll leave rr's chaos mode running for a bit, but maybe I'm doing something wrong?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Aand I got to repro it in a debug-opt build, nvm. Probably debug no-opt is just too slow.

Comment 5

[Cameron McCormack (:heycam)]

I guess this is caused by bug 1500126.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Sounds very likely...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Any chance you could look at this Jonathan?

Comment 8

[Jonathan Kew [:jfkthame]]

Ah, so the issue is that when stylo calls GetMetricsFor, and we want to post a task to flush some old cache entries, the Runnable that we create needs to hold a reference to the nsFontCache, just in case it might otherwise get torn down (unlikely though that may be).

But nsFontCache doesn't have threadsafe refcounting, hence this assertion.

AFAICS, just switching nsFontCache from NS_DECL_ISUPPORTS to NS_DECL_THREADSAFE_ISUPPORTS should avoid the problem here.

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Use thread-safe refcounting for nsFontCache

I wasn't able to reproduce the assertion locally (in my opt+debug build), but AFAICS this should fix things.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9020802 [details] [diff] [review]
Use thread-safe refcounting for nsFontCache

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

Thanks! And sorry for throwing the ni? at you, but I imagined you had all this code paged in and you would be able to figure out earlier than I even get to take a proper look at this :)

Comment 11

[Jonathan Kew [:jfkthame]]

Marking this sec-moderate, as although it's certainly a bug, it doesn't look like there'd be any clear path to an attack.

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/af5a791e7b7e7fc42646f202db59cfbf4c48cd6e
https://hg.mozilla.org/mozilla-central/rev/af5a791e7b7e

Comment 13

[Jonathan Kew [:jfkthame]]

Jason, if you could verify whether this issue still reproduces now that the patch has landed, that would be great -- thanks! Provided it is indeed fixed, we'll want to uplift to FF64.

Comment 14

[Jason Kratzer [:jkratzer]]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> Jason, if you could verify whether this issue still reproduces now that the
> patch has landed, that would be great -- thanks! Provided it is indeed
> fixed, we'll want to uplift to FF64.

I can no longer reproduce the issue using build c3f01d72374b (from TC).

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 9020802 [details] [diff] [review]
Use thread-safe refcounting for nsFontCache

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: 1498316 + 1502252

User impact if declined: Possible race condition during stylo processing, which could lead to corruption or leakage of the device context's font-metrics cache.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Just makes nsFontCache use thread-safe refcounting, no other change.

String changes made/needed: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9020802 [details] [diff] [review]
Use thread-safe refcounting for nsFontCache

[Triage Comment]
Fixes a moderate sec issue new to Fx64. Approved for 64.0b6.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c5c1526daf28

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Jonathan Kew (:jfkthame) from comment #15)
> Has the fix been verified in Nightly?: Yes
> 
> Needs manual test from QE?: No

Setting qe- per Jonathan's assessment.