stylo: panicked at 'already mutably borrowed'

VERIFIED FIXED in Firefox 57

Status

()

P2
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: truber, Assigned: emilio)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla57
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57blocking verified)

Details

(Whiteboard: [fuzzblocker], crash signature)

Attachments

(3 attachments)

Created attachment 8907687 [details]
testcase.html

The attached testcase causes a panic in m-c rev 20170913-1888ec2f277f.

In my testing it seems to call reload 3-20 times, usually closer to 3. I left the window.dump in to show this.

Marking fuzzblocker because this is happening a lot and I've had trouble getting a reliable testcase.


thread 'StyleThread#0' panicked at 'already mutably borrowed', /builds/worker/workspace/build/src/third_party/rust/atomic_refcell/src/lib.rs:161
#0: mozalloc_abort, at memory/mozalloc/mozalloc_abort.cpp:33
#1: abort, at memory/mozalloc/mozalloc_abort.cpp:80
#2: panic_abort::__rust_start_panic, at src/libpanic_abort/lib.rs:61
#3: std::panicking::rust_panic, at src/libstd/panicking.rs:580
#4: std::panicking::rust_panic_with_hook, at src/libstd/panicking.rs:565
#5: std::panicking::begin_panic<&str>, at src/libstd/panicking.rs:511
#6: atomic_refcell::AtomicBorrowRef::do_panic, at third_party/rust/atomic_refcell/src/lib.rs:161
#7: atomic_refcell::AtomicBorrowRef::new, at third_party/rust/atomic_refcell/src/lib.rs:130
#8: atomic_refcell::AtomicRefCell<style::data::ElementData>::borrow<style::data::ElementData>, at third_party/rust/atomic_refcell/src/lib.rs:88
#9: style::dom::TElement::borrow_data::{{closure}}<style::gecko::wrapper::GeckoElement>, at servo/components/style/dom.rs:564
#10: core::option::Option<&atomic_refcell::AtomicRefCell<style::data::ElementData>>::map<&atomic_refcell::AtomicRefCell<style::data::ElementData>,atomic_refcell::AtomicRef<style::data::ElementData>,closure>, at src/libcore/option.rs:398
#11: style::dom::TElement::borrow_data<style::gecko::wrapper::GeckoElement>, at servo/components/style/dom.rs:564
#12: style::values::specified::color::{{impl}}::to_computed_value::{{closure}}, at servo/components/style/values/specified/color.rs:278
#13: core::option::Option<&style::gecko::wrapper::GeckoElement>::and_then<&style::gecko::wrapper::GeckoElement,atomic_refcell::AtomicRef<style::data::ElementData>,closure>, at src/libcore/option.rs:605
#14: style::values::specified::color::{{impl}}::to_computed_value, at servo/components/style/values/specified/color.rs:278
#15: style::values::specified::color::{{impl}}::to_computed_value, at servo/components/style/values/specified/color.rs:343
#16: style::properties::longhands::color::cascade_property, at a0a2d645f309019c3894bb437f786b258cfb8211ac567e22d56eddf8d62caed4525ce43711f281e8c046f8a257375d1d8b037b0cfa84a6a22cedd3bcf3384757/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-bccbf2b1c0916c0e/out/properties.rs:17703
#17: style::properties::apply_declarations<closure,core::iter::FlatMap<style::rule_tree::SelfAndAncestors, core::iter::FilterMap<core::iter::Rev<style::properties::declaration_block::DeclarationImportanceIterator>, closure>, closure>>, at
a0a2d645f309019c3894bb437f786b258cfb8211ac567e22d56eddf8d62caed4525ce43711f281e8c046f8a257375d1d8b037b0cfa84a6a22cedd3bcf3384757/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-bccbf2b1c0916c0e/out/properties.rs:137781
#18: style::properties::cascade, at a0a2d645f309019c3894bb437f786b258cfb8211ac567e22d56eddf8d62caed4525ce43711f281e8c046f8a257375d1d8b037b0cfa84a6a22cedd3bcf3384757/toolkit/library/x86_64-unknown-linux-gnu/debug/build/style-bccbf2b1c0916c0e/out/properties.rs:137590
#19: style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::cascade_style<style::gecko::wrapper::GeckoElement>, at servo/components/style/style_resolver.rs:522
#20: style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::resolve_primary_style<style::gecko::wrapper::GeckoElement>, at servo/components/style/style_resolver.rs:159
#21: style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::resolve_style<style::gecko::wrapper::GeckoElement>, at servo/components/style/style_resolver.rs:179
#22: style::style_resolver::{{impl}}::resolve_style_with_default_parents::{{closure}}<style::gecko::wrapper::GeckoElement>, at servo/components/style/style_resolver.rs:218
#23: style::style_resolver::with_default_parent_styles<style::gecko::wrapper::GeckoElement,closure,style::data::ElementStyles>, at servo/components/style/style_resolver.rs:76
#24: style::style_resolver::StyleResolverForElement<style::gecko::wrapper::GeckoElement>::resolve_style_with_default_parents<style::gecko::wrapper::GeckoElement>, at servo/components/style/style_resolver.rs:217
#25: style::traversal::compute_style<style::gecko::wrapper::GeckoElement>, at servo/components/style/traversal.rs:674
#26: style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,closure>, at servo/components/style/traversal.rs:477
#27: style::gecko::traversal::{{impl}}::process_preorder<closure>, at servo/components/style/gecko/traversal.rs:37
#28: style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,smallvec::Drain<style::dom::SendNode<style::gecko::wrapper::GeckoNode>>>, at servo/components/style/parallel.rs:191
#29: style::parallel::traverse_nodes::{{closure}}<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,collections::vec_deque::Drain<style::dom::SendNode<style::gecko::wrapper::GeckoNode>>>, at servo/components/style/parallel.rs:207
#30: rayon_core::scope::{{impl}}::execute_job_closure::{{closure}}<closure,()>, at third_party/rust/rayon-core/src/scope/mod.rs:354
#31: std::panic::{{impl}}::call_once<(),closure>, at src/libstd/panic.rs:296
#32: std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()>, at src/libstd/panicking.rs:454
#33: panic_abort::__rust_maybe_catch_panic, at src/libpanic_abort/lib.rs:40
#34: std::panicking::try<(),std::panic::AssertUnwindSafe<closure>>, at src/libstd/panicking.rs:433
#35: std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()>, at src/libstd/panic.rs:361
#36: rayon_core::unwind::halt_unwinding<closure,()>, at third_party/rust/rayon-core/src/unwind.rs:19
#37: rayon_core::scope::Scope::execute_job_closure<closure,()>, at third_party/rust/rayon-core/src/scope/mod.rs:354
#38: rayon_core::scope::Scope::execute_job<closure>, at third_party/rust/rayon-core/src/scope/mod.rs:343
#39: rayon_core::scope::{{impl}}::spawn::{{closure}}<closure>, at third_party/rust/rayon-core/src/scope/mod.rs:274
#40: rayon_core::job::{{impl}}::execute<closure>, at third_party/rust/rayon-core/src/job.rs:142
#41: rayon_core::registry::WorkerThread::execute, at third_party/rust/rayon-core/src/registry.rs:476
#42: rayon_core::registry::WorkerThread::wait_until_cold<rayon_core::latch::CountLatch>, at third_party/rust/rayon-core/src/registry.rs:460
#43: rayon_core::registry::WorkerThread::wait_until<rayon_core::latch::CountLatch>, at third_party/rust/rayon-core/src/registry.rs:436
#44: rayon_core::registry::main_loop, at third_party/rust/rayon-core/src/registry.rs:559
#45: std::sys_common::backtrace::__rust_begin_short_backtrace<closure,()>, at src/libstd/sys_common/backtrace.rs:136
#46: std::thread::{{impl}}::spawn::{{closure}}::{{closure}}<closure,()>, at src/libstd/thread/mod.rs:364
#47: std::panic::{{impl}}::call_once<(),closure>, at src/libstd/panic.rs:296
#48: std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()>, at src/libstd/panicking.rs:454
#49: panic_abort::__rust_maybe_catch_panic, at src/libpanic_abort/lib.rs:40
#50: std::panicking::try<(),std::panic::AssertUnwindSafe<closure>>, at src/libstd/panicking.rs:433
#51: std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()>, at src/libstd/panic.rs:361
#52: std::thread::{{impl}}::spawn::{{closure}}<closure,()>, at src/libstd/thread/mod.rs:363
#53: alloc::boxed::{{impl}}::call_box<(),closure>, at src/liballoc/boxed.rs:648
#54: std::sys::imp::thread::{{impl}}::new::thread_start, at src/liballoc/boxed.rs:658
#55: libpthread-2.25.so+0x7049
#56: libc-2.25.so+0xedf0f
Flags: in-testsuite?
Looks like this is the InheritFromBodyQuirk. Do we have any guarantees that we're descended from the body, and thus that borrowing its data during the traversal is safe?
Flags: needinfo?(manishearth)
Priority: -- → P2
(Assignee)

Comment 2

a year ago
It's not, see the test-case. Those are siblings of the <body>.

This is pretty annoying. I guess we can do something like for root font-sizes... I can take this I guess.
Assignee: nobody → emilio
Comment hidden (mozreview-request)

Comment 4

a year ago
mozreview-review
Comment on attachment 8907699 [details]
Bug 1399546: Remove broken code for handling the body text color.

https://reviewboard.mozilla.org/r/179368/#review184534

ugh. but ok. can't think of anything better
Attachment #8907699 - Flags: review+

Comment 5

a year ago
mozreview-review
Comment on attachment 8907699 [details]
Bug 1399546: Remove broken code for handling the body text color.

https://reviewboard.mozilla.org/r/179368/#review184536

::: layout/style/ServoStyleSet.cpp:448
(Diff revision 1)
> +      // NB: In the common case, we'll have the style already resolved and this
> +      // should be pretty fast (plus this is compat mode only).

Add a comment explaining that these styles can be stale, but that we don't care?

::: layout/style/ServoStyleSet.cpp:451
(Diff revision 1)
> +  if (mPresContext->CompatibilityMode() == eCompatibility_NavQuirks) {
> +    if (Element* body = mPresContext->Document()->GetBodyElement()) {
> +      // NB: In the common case, we'll have the style already resolved and this
> +      // should be pretty fast (plus this is compat mode only).
> +      RefPtr<ServoStyleContext> computedValues =
> +        Servo_ResolveStyleLazily(body,

Seems like it would be cleaner to call ResolveStyleLazilyInternal here?

::: layout/style/ServoStyleSet.cpp
(Diff revision 1)
> -  UpdateStylistIfNeeded();
>    RefPtr<ServoStyleContext> result =
>      Servo_ResolveStyle(aElement, mRawSet.get()).Consume();
> -  UpdateBodyTextColorIfNeeded(*aElement, *result, *mPresContext);

I don't understand what this UpdateStylistIfNeeded stuff is about. And I also don't understand the relationship between UpdateBodyTextColorIfNeeded and the color.rs code that's being removed.
Attachment #8907699 - Flags: review?(bobbyholley) → review+
Flags: needinfo?(manishearth)
Crash Signature: [@ atomic_refcell::AtomicBorrowRef::do_panic] [@ mozalloc_abort | abort | atomic_refcell::AtomicBorrowRef::do_panic]
Crash Signature: [@ atomic_refcell::AtomicBorrowRef::do_panic] [@ mozalloc_abort | abort | atomic_refcell::AtomicBorrowRef::do_panic] → [@ atomic_refcell::AtomicBorrowRef::do_panic] [@ mozalloc_abort | abort | atomic_refcell::AtomicBorrowRef::do_panic] [@ mozalloc_abort | abort | atomic_refcell::{{impl}}::do_panic]
Was this superseded by  https://hg.mozilla.org/integration/autoland/rev/67769dac78c4 ? If so, shall we land the testcase here and close the bug?
Flags: needinfo?(emilio)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cd3a11354b72
Remove broken code for handling the body text color. r=bholley,Manishearth
https://hg.mozilla.org/integration/autoland/rev/f6de9a593c3c
Add an API to know if an element is it's document body element. r=heycam
(Assignee)

Comment 11

a year ago
I pushed that in a way it didn't break the build. Need to reland the real fix, which is https://github.com/servo/servo/pull/18519
Flags: needinfo?(emilio)
Keywords: leave-open
(Assignee)

Comment 12

a year ago
Oh, and land the crashtest of course.
This is the #3 Window topcrash in Nightly 20170915100121.

Comment 16

a year ago
mozreview-review
Comment on attachment 8908804 [details]
Bug 1399546: Implement the tables inherit from body quirk in a more straight-forward way.

https://reviewboard.mozilla.org/r/180420/#review185796

::: layout/style/ServoBindings.cpp:850
(Diff revision 2)
> -  return aPresContext->Document()->GetBodyElement();
> +  nsIDocument* doc = aElement->GetUncomposedDoc();
> +  return doc && doc->GetBodyElement() == aElement;

Probably more straightforward to do

  return aElement->OwnerDoc()->GetBodyElement() == aElement;

but not a big deal.
Attachment #8908804 - Flags: review?(cam) → review+
(Assignee)

Comment 17

a year ago
(In reply to Nicholas Nethercote [:njn] from comment #15)
> This is the #3 Window topcrash in Nightly 20170915100121.

A few of the crashes had a different stack, I submitted https://github.com/servo/servo/pull/18547 to fix those.
tracking-firefox57: --- → blocking
https://hg.mozilla.org/mozilla-central/rev/f7b1daacf69d
https://hg.mozilla.org/mozilla-central/rev/001d0020adcc
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
status-firefox-esr52: --- → unaffected
Flags: in-testsuite? → in-testsuite+
No more crashes.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.