Closed Bug 1638765 Opened 6 years ago Closed 2 years ago

Crash in [@ style::rule_collector::RuleCollector<T>::collect_rules_in_map<T>]

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Platform:
ARM64
Windows 10
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
firefox77 + wontfix
firefox78 --- wontfix

People

(Reporter: pascalc|PTO, Unassigned)

Details

(Keywords: crash, regression)

Crash Data

This bug is for crash report bp-cfc8bdf7-f5bd-45eb-a1e1-9a67e0200518.

Top 10 frames of crashing thread:

0 xul.dll style::rule_collector::RuleCollector<style::gecko::wrapper::GeckoElement, closure-0>::collect_rules_in_map<style::gecko::wrapper::GeckoElement, closure-0> servo/components/style/rule_collector.rs:241
1 xul.dll style::rule_collector::RuleCollector<style::gecko::wrapper::GeckoElement, closure-0>::collect_rules_in_map<style::gecko::wrapper::GeckoElement, closure-0> servo/components/style/rule_collector.rs:241
2 xul.dll style::rule_collector::RuleCollector<style::gecko::wrapper::GeckoElement, closure-0>::collect_stylist_rules<style::gecko::wrapper::GeckoElement, closure-0> servo/components/style/rule_collector.rs:176
3 xul.dll style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::match_pseudo<style::gecko::wrapper::GeckoElement> servo/components/style/style_resolver.rs:510
4 xul.dll style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::resolve_style<style::gecko::wrapper::GeckoElement> servo/components/style/style_resolver.rs:242
5 xul.dll style::traversal::compute_style<style::gecko::wrapper::GeckoElement> servo/components/style/traversal.rs:608
6 xul.dll style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement, style::gecko::traversal::RecalcStyleOnly, smallvec::Drain<[style::dom::SendNode<style::gecko::wrapper::GeckoNode>; 128]>> servo/components/style/parallel.rs:277
7 xul.dll style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement, style::gecko::traversal::RecalcStyleOnly, smallvec::Drain<[style::dom::SendNode<style::gecko::wrapper::GeckoNode>; 128]>> servo/components/style/parallel.rs:277
8 xul.dll style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement, style::gecko::traversal::RecalcStyleOnly, smallvec::Drain<[style::dom::SendNode<style::gecko::wrapper::GeckoNode>; 128]>> servo/components/style/parallel.rs:277
9 xul.dll style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement, style::gecko::traversal::RecalcStyleOnly, smallvec::Drain<[style::dom::SendNode<style::gecko::wrapper::GeckoNode>; 128]>> servo/components/style/parallel.rs:277

This is a new 77 crash (or a new signature), seems Windows only, medium volume on beta.

seems like arm64 builds?

Hardware: Unspecified → ARM64

Cameron: Any ideas what landed in 77 that might cause this? Definitely looks like Windows ARM64 only.

Flags: needinfo?(cam)

I think this is just a signature change.

(This function was introduced by bug 1624968.)

Emilio: Does this being a signature change mean this is a duplicate of another bug?

Flags: needinfo?(emilio)

I thought so, but I couldn't find any dupe (as in, any with a related signature from either RuleCollector or SelectorMap), so probably not after all, sorry :/

All reports are arm64 which indicates a potential compiler issue?

Flags: needinfo?(emilio)

(This function was introduced by bug 1624968.)

Looks like that landed April 6.

This signature was first seen in build 20200423214309 although since it's pretty rare, it might have started anywhere in the 3 days before that, to be safe.

[@ smallvec::SmallVec<T>::push<T> ] and [@ style::selector_map::SelectorMap<T>::get_matching_rules<T> ] are additional arm64-only crashes on 77.

Hi Sean, can you help us find an assignee for this one so we can target a fix for 77?

Flags: needinfo?(svoisen)

I had a look at the crashes and I'd say this is most likely a race which will probably make it very hard to debug, here's why:

  • Crashing addresses are all over the place, ranging from 0, small numbers or actual addresses, it means they're being fetched from "randomish" data (but still valid data, there's no poison patterns there so it's not a use-after-free).
  • It's happening only on instances of the StyleThread of which several should be accessing this data in parallel
  • We have crashes which are also ARM-only and beta-only in the same code with actual crash reasons that should be impossible

See this one. The signature is [@ core::ops::function::Fn::call<T> | style::rule_collector::RuleCollector<T>::collect_stylist_rules<T>] and the crash reason is "index out of bounds: the len is 18 but the index is 3"... which doesn't look out-of-bounds to me unless it was changed along the way.

See this other one. The signature [@ core::ops::function::Fn::call<T> | style::rule_collector::RuleCollector<T>::collect_rules_in_map<T>] and the crash reason is "called Option::unwrap() on a None value" which again shouldn't be happening.

As for this being an ARM-only issue I guess it might be due to a different implementation of the synchronization primitives. ARM v8 has a weakly-ordered model of memory which can lead to subtle races that cannot happen on x86 because of it stronger memory-order model.

The index out of bounds one, assuming that it comes from the only indexing operation in that function call (this one) cannot really happen (without any external corruption at least). That is an array that's on the stack.

And... The hashmaps that we're talking around are in fact accessed from multiple threads, but they are not mutated while accessed, because the main thread is paused as long as worker threads are running.

Something that comes to mind is that we migrated those hashmaps to hashbrown in 77 (bug 1631721). That patch was backed out from beta, so maybe there's some subtle issue in the hashbrown implementation, somehow (but again I'm not sure how'd that happen given there's no mutation of the data happening). If that's the case we should see the crash volume go to zero in the coming betas, but keep around in nightly.

Also, a bit weird that this is arm64 but there's no crash on android at all. When we've had other memory-ordering issues in related C++ code (bug 1434994), we've seen android crashes spike heavily.

It might also be an issue somewhere in the toolchain which would explain the difference between Android and Windows. In general AArch64/Windows is not a target that receives a lot of testing so it's more likely to hit obscure issues in the toolchain or standard libraries than AArch64/Android.

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

And... The hashmaps that we're talking around are in fact accessed from multiple threads, but they are not mutated while accessed, because the main thread is paused as long as worker threads are running.

What about the tree itself, does it mutate?

Which tree? The DOM tree? Definitely not.

We have a rule tree that does mutate in parallel, but this is crashing earlier, before we get to insert stuff in it.

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

We have a rule tree that does mutate in parallel, but this is crashing earlier, before we get to insert stuff in it.

I was thinking about the rule tree. There's another odd thing about the crashes I hadn't noticed before: the crashing thread is the only one doing something. I've opened a dozen crashes and all the other threads are idling, waiting for an event, with the sole exception of the crashing thread.

Interestingly, the crashes went down in the last betas (no crash in beta 8, one use crashing in beta 9), maybe what causes most of the crashes crash is a feature behind the EARLY_BETA_OR_EARLIER flag?

BTW, can we get an assignee to this bug please? Thanks!

(In reply to Pascal Chevrel:pascalc from comment #18)

Interestingly, the crashes went down in the last betas (no crash in beta 8, one use crashing in beta 9), maybe what causes most of the crashes crash is a feature behind the EARLY_BETA_OR_EARLIER flag?

Hmm, does it match the uplift of bug 1633410?

(In reply to Pascal Chevrel:pascalc from comment #19)

BTW, can we get an assignee to this bug please? Thanks!

It's still unclear what the root cause is, so not clear who should take it. If the reply to my question above is "yes", we should probably land that backout on mozilla-central, then inform the rust / hashbrown maintainers. I can take care of that.

Flags: needinfo?(pascalc)

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

(In reply to Pascal Chevrel:pascalc from comment #18)

Interestingly, the crashes went down in the last betas (no crash in beta 8, one use crashing in beta 9), maybe what causes most of the crashes crash is a feature behind the EARLY_BETA_OR_EARLIER flag?

Hmm, does it match the uplift of bug 1633410?

Yes, Bug 1633410 was uplifted in beta 8 (https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_77_0b7_RELEASE&tochange=FIREFOX_77_0b8_RELEASE)

Flags: needinfo?(pascalc)

The volume of crashes is low after beta 8 and we build our release candidate today, marking as wontfix for 77.

status-firefox77: affected → wontfix

Sorry — Thanks Emilio for jumping in on this :) Will monitor this after we figure out bug 1633410 and what is going on with Hashbrown.

Flags: needinfo?(svoisen)
Flags: needinfo?(cam)

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

For more information, please visit auto_nag documentation.

Flags: needinfo?(boris.chiou)

The volume of crashes becomes very low in 78, so mark this S3 for now. We should keep monitoring this together with bug 1633410.

Severity: -- → S3
Flags: needinfo?(boris.chiou)
Priority: -- → P3

Closing because no crashes reported for 12 weeks.

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.