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

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?

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.

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

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.

Was this superseded by https://hg.mozilla.org/integration/autoland/rev/67769dac78c4 ? If so, shall we land the testcase here and close the bug?

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

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

Oh, and land the crashtest of course.

https://hg.mozilla.org/mozilla-central/rev/cd3a11354b72 https://hg.mozilla.org/mozilla-central/rev/f6de9a593c3c

Crashtest + removal of the leftover code: remote: https://hg.mozilla.org/integration/autoland/rev/f7b1daacf69d8d8b49fb881ec53e91667b372354 remote: https://hg.mozilla.org/integration/autoland/rev/001d0020adccd2fa9e84ee97d5ca01d0f8ee6a7e

This is the #3 Window topcrash in Nightly 20170915100121.

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.

(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.

https://hg.mozilla.org/mozilla-central/rev/f7b1daacf69d https://hg.mozilla.org/mozilla-central/rev/001d0020adcc

No more crashes.