1399546 - stylo: panicked at 'already mutably borrowed'

Status: VERIFIED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: 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

Comment 1

[Bobby Holley (:bholley)]

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?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1399546: Remove broken code for handling the body text color.

It still sucks though.

Review commit: https://reviewboard.mozilla.org/r/179368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/179368/

Comment 4

[Manish Goregaokar [:manishearth]]

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 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1399546: Implement the tables inherit from body quirk in a more straight-forward way.

Review commit: https://reviewboard.mozilla.org/r/180420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180420/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8907699 [details]
Bug 1399546: Remove broken code for handling the body text color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/179368/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180420/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Oh, and land the crashtest of course.

Comment 13

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

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

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Crashtest + removal of the leftover code:

remote:   https://hg.mozilla.org/integration/autoland/rev/f7b1daacf69d8d8b49fb881ec53e91667b372354
remote:   https://hg.mozilla.org/integration/autoland/rev/001d0020adccd2fa9e84ee97d5ca01d0f8ee6a7e

Comment 15

[Nicholas Nethercote [inactive]]

This is the #3 Window topcrash in Nightly 20170915100121.

Comment 16

[Cameron McCormack (:heycam)]

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.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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

Comment 18

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

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

Comment 19

[Calixte Denizet (:calixte)]

No more crashes.