1331272 - Stylo-only incremental restyle assertion: assertion failed: pseudo.is_none() || new_style.get_box().clone_display() != display::T::none

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

I was browsing with Stylo quite randomly to test my patches for bug 1325878, and I hit this pretty consistently.

I haven't dug yet (I may do it if I find the time), but worth filling.

Backtrace:

#0  0x0000000000407715 in mozalloc_abort(char const*) (msg=0x430060 "Redirecting call to abort() to mozalloc_abort\n")
    at /home/emilio/projects/moz/stylo/memory/mozalloc/mozalloc_abort.cpp:33
#1  0x000000000040773f in abort() () at /home/emilio/projects/moz/stylo/memory/mozalloc/mozalloc_abort.cpp:80
#2  0x00007fffe80f0919 in panic_abort::__rust_start_panic::abort () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libpanic_abort/lib.rs:61
#3  0x00007fffe80f0919 in panic_abort::__rust_start_panic () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libpanic_abort/lib.rs:56
#4  0x00007fffe80eca5d in std::panicking::rust_panic () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:584
#5  0x00007fffe80eca1d in std::panicking::rust_panic_with_hook () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:569
#6  0x00007fffe77e2cc4 in std::panicking::begin_panic<&str> (msg=..., file_line=0x7fffeba923f0 <style::matching::MatchMethods::compute_restyle_damage::_FILE_LINE>)
    at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:515
#7  0x00007fffe7aeef5b in style::matching::MatchMethods::compute_restyle_damage<style::gecko::wrapper::GeckoElement> (self=0x7fffb6efaf30, old_style=..., new_style=0x7fffb6ef8740, pseudo=...) at /home/emilio/projects/moz/stylo/servo/components/style/matching.rs:786
#8  0x00007fffe7af01bc in style::matching::MatchMethods::compute_damage_and_cascade_pseudos::{{closure}}<style::gecko::wrapper::GeckoElement> (pseudo=...)
    at /home/emilio/projects/moz/stylo/servo/components/style/matching.rs:959
#9  0x00007fffe7af8814 in style::selector_parser::{{impl}}::each_eagerly_cascaded_pseudo_element::{{closure}}<closure> (pseudo=...)
    at /home/emilio/projects/moz/stylo/servo/components/style/selector_parser.rs:148
#10 0x00007fffe7ad0aba in style::gecko::selector_parser::SelectorImpl::each_pseudo_element<closure> (fun=...)
    at /home/emilio/projects/moz/stylo/servo/components/style/gecko/selector_parser.rs:361
#11 0x00007fffe7af8548 in style::gecko::selector_parser::SelectorImpl::each_eagerly_cascaded_pseudo_element<closure> (fun=...)
    at /home/emilio/projects/moz/stylo/servo/components/style/selector_parser.rs:146
#12 0x00007fffe7aefd8d in style::matching::MatchMethods::compute_damage_and_cascade_pseudos<style::gecko::wrapper::GeckoElement> (self=0x7fffb6efaf30, old_primary=..., old_pseudos=..., new_primary=0x7fffb6efab88, new_pseudos=0x7fffb6efab90, context=0x7fffb6efb460, pseudo_rule_nodes=..., possibly_expired_animations=0x7fffb6efab68)
    at /home/emilio/projects/moz/stylo/servo/components/style/matching.rs:921
#13 0x00007fffe7aef440 in style::matching::MatchMethods::cascade_node<style::gecko::wrapper::GeckoElement> (self=0x7fffb6efaf30, context=0x7fffb6efb460, data=0x7fffb6efb470, parent=..., primary_rule_node=..., pseudo_rule_nodes=..., primary_is_shareable=false) at /home/emilio/projects/moz/stylo/servo/components/style/matching.rs:848
#14 0x00007fffe7b09b46 in style::traversal::compute_style<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (_traversal=0x7fffffffb098, traversal_data=0x7fffb6efb950, context=0x7fffb6efb460, element=..., data=0x7fffb6efb470) at /home/emilio/projects/moz/stylo/servo/components/style/traversal.rs:480
#15 0x00007fffe7b09277 in style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (traversal=0x7fffffffb098, traversal_data=0x7fffb6efb950, context=0x7fffb6efb460, element=..., data=0x7fffb6efb470) at /home/emilio/projects/moz/stylo/servo/components/style/traversal.rs:400
#16 0x00007fffe7adb727 in style::gecko::traversal::{{impl}}::process_preorder (self=0x7fffffffb098, traversal_data=0x7fffb6efb950, thread_local=0x7fffad20b408, node=...)
    at /home/emilio/projects/moz/stylo/servo/components/style/gecko/traversal.rs:43
#17 0x00007fffe7639ff2 in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fffb6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:113
#18 0x00007fffe7639ff2 in style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fffb6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:159
#19 0x00007fffe763a28f in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fffb6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:140
#20 0x00007fffe763a28f in style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fffb6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:159
#21 0x00007fffe763a28f in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fffb6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:140
#22 0x00007fffe763a28f in style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fffb6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:159
#23 0x00007fffe763a28f in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fffb6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:140
#24 0x00007fffe763a28f in style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fffb6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:159
#25 0x00007fffe763a28f in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fffb6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:140
#26 0x00007fffe763a28f in style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., root=..., traversal_data=..., scope=0x7fff---Type <return> to continue, or q <return> to quit---
b6efda10, traversal=0x7fffffffb098, tls=0x7fffffffac98) at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:159
#27 0x00007fffe76399ab in style::parallel::traverse_dom::{{closure}}::{{closure}}<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (scope=0x7fffb6efda10)
    at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:77
#28 0x00007fffe75e6b1e in rayon::scope::{{impl}}::execute_job_closure::{{closure}}<closure> () at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/scope/mod.rs:314
#29 0x00007fffe76e0083 in std::panic::{{impl}}::call_once<(),closure> (self=..., _args=()) at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panic.rs:295
#30 0x00007fffe758f038 in std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> (data=0x7fffb6efd400 "p\002\366\300\377\177\000")
    at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:458
#31 0x00007fffe80f090c in panic_abort::__rust_maybe_catch_panic () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libpanic_abort/lib.rs:40
#32 0x00007fffe758e491 in std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> (f=...) at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:434
#33 0x00007fffe758dd77 in std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> (f=...)
    at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panic.rs:351
#34 0x00007fffe75e719c in rayon::unwind::halt_unwinding<closure,()> (func=...) at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/unwind.rs:19
#35 0x00007fffe75e676c in rayon::scope::Scope::execute_job_closure<closure> (self=0x7fffb6efda10, func=...) at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/scope/mod.rs:314
#36 0x00007fffe75e709d in rayon::scope::scope<closure> (op=...) at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/scope/mod.rs:241
#37 0x00007fffe76398d9 in style::parallel::traverse_dom::{{closure}}<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> ()
    at /home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:76
#38 0x00007fffe76aab0e in rayon::job::{{impl}}::execute::{{closure}}<rayon::latch::LockLatch,closure,()> () at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/job.rs:110
#39 0x00007fffe76dffaa in std::panic::{{impl}}::call_once<(),closure> (self=..., _args=()) at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panic.rs:295
#40 0x00007fffe758f22a in std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> (data=0x7fffb6efe100 "p\002\366\300\377\177\000")
    at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:458
#41 0x00007fffe80f090c in panic_abort::__rust_maybe_catch_panic () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libpanic_abort/lib.rs:40
#42 0x00007fffe758e0ca in std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> (f=...) at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:434
#43 0x00007fffe758dc8e in std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> (f=...)
    at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panic.rs:351
#44 0x00007fffe75e7279 in rayon::unwind::halt_unwinding<closure,()> (func=...) at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/unwind.rs:19
#45 0x00007fffe76aa6db in rayon::job::{{impl}}::execute<rayon::latch::LockLatch,closure,()> (this=0x7fffffffa918, mode=rayon::job::JobMode::Execute)
    at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/job.rs:110
#46 0x00007fffe7f904eb in rayon::job::JobRef::execute (self=0x7fffb6efe788, mode=rayon::job::JobMode::Execute) at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/job.rs:60
#47 0x00007fffe7f926d0 in rayon::thread_pool::main_loop (worker=..., registry=..., index=0) at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/thread_pool.rs:447
#48 0x00007fffe7f912df in rayon::thread_pool::{{impl}}::new::{{closure}} () at /home/emilio/projects/moz/stylo/third_party/rust/rayon/src/thread_pool.rs:87
#49 0x00007fffe7f8e07b in std::panic::{{impl}}::call_once<(),closure> (self=..., _args=()) at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panic.rs:295
#50 0x00007fffe7f7a098 in std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> (data=0x7fffb6efec10 "\200\373߸\377\177\000")
    at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:458
#51 0x00007fffe80f090c in panic_abort::__rust_maybe_catch_panic () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libpanic_abort/lib.rs:40
#52 0x00007fffe7f79dab in std::panicking::try<(),std::panic::AssertUnwindSafe<closure>> (f=...) at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panicking.rs:434
#53 0x00007fffe7f78e1a in std::panic::catch_unwind<std::panic::AssertUnwindSafe<closure>,()> (f=...)
    at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/panic.rs:351
#54 0x00007fffe7f799f5 in std::thread::{{impl}}::spawn::{{closure}}<closure,()> () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/thread/mod.rs:287
#55 0x00007fffe7f86124 in alloc::boxed::{{impl}}::call_box<(),closure> (self=0x7fffb8dfffd0, args=())
    at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/liballoc/boxed.rs:605
#56 0x00007fffe80eb375 in alloc::boxed::{{impl}}::call_once<(),()> () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/liballoc/boxed.rs:615
#57 0x00007fffe80eb375 in std::sys_common::thread::start_thread () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/sys_common/thread.rs:21
#58 0x00007fffe80eb375 in std::sys::imp::thread::{{impl}}::new::thread_start () at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libstd/sys/unix/thread.rs:84
#59 0x00007ffff7bc4454 in start_thread () at /usr/lib/libpthread.so.0
#60 0x00007ffff6e607df in clone () at /usr/lib/libc.so.6

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Minimal test case:

<!doctype html>
<style>
  div::before {
    content: "PASS";
    display: none;
  }
  .foo::before {
    display: none;
  }
</style>
<div></div>
<script>
window.onload = function() {
  document.querySelector('div').className = "foo";
}
</script>

Mainly, we're restyling an element and one of the pseudo-elements transitions from `display: none` to `display: none`. The code there assumes that that has been checked, so it triggers a full reflow of the element that could be avoided.

I believe we used to check for this and one of Bobby's incremental restyle patches stopped doing that apparently?

Funny, I wonder why I can't reproduce on Servo though.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Of course, I can't reproduce because servo _has_ the old computed values (in the hash map), so it actually compares them. We can't get the style context though, so we legitimately fail the assertion.

Adding that check should be easy.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Servo PR

Comment 4

[Bobby Holley (:bholley)]

Nice! Can you check the testcase in as a crashtest, and do a "-b do -p linux64-stylo -u crashtest -t none" try run before landing?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1331272: Crashtest

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

Comment 6

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f01b8dd38be933c14be9d969764b9298156d30b

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8827077 [details]
Bug 1331272: Crashtest

https://reviewboard.mozilla.org/r/104910/#review105740

LGTM. Did you test that it makes the harness crash (./mach crashtest --disable-e10s layout/style/crashtests/crashtests.list) without your patch? There's some weirdness in the crashtest harness where it will unload the page as soon as onload is fired unless you add |reftest-wait| to the root element. In this case it shouldn't be a problem, since your onload handler should run before the harness' onload handler, but it's good to be sure in case something async is going on.

Thanks for adding this! :-)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #7)
> LGTM. Did you test that it makes the harness crash (./mach crashtest
> --disable-e10s layout/style/crashtests/crashtests.list) without your patch?
> There's some weirdness in the crashtest harness where it will unload the
> page as soon as onload is fired unless you add |reftest-wait| to the root
> element. In this case it shouldn't be a problem, since your onload handler
> should run before the harness' onload handler, but it's good to be sure in
> case something async is going on.
> 
> Thanks for adding this! :-)

Yup, just did a revert of my patch locally and the crashtest crashes. No problem! :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08866fd222c6
Crashtest. r=bholley

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/08866fd222c6