STR: 1. Load the attached test case in a debug Stylo build 2. Allow pop-ups for the test case 3. Reload the test case For me it hits the panic here: http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/servo/components/style/stylist.rs#1305 i.e. we fail to GC the rule nodes. In this test case we request computed style on a node in a bfcache document that is also the root of a display:none subtree. I *think* the key factors are: 1. The node is part of a display:none subtree so getComputedStyle can't just pull the style context off the frame but needs to recompute it. 2. The node is the *root* of the display:none subtree so we *do* have styles for it that we return instead of calling resolve_style which takes care to drop the resolved styles. (2) might not actually be important, however. Assigning to Bobby since I believe he had an idea for how to fix this.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=c386506645d46109504da8ad73e7f8fbb7ea4413 This is orange due to the refcount logging I added. We're green without it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=928ebab2ccc4f3486ece6b1652639a86fddf1721

We already gc before dropping the RuleTree.

Since we're going to stop asserting that all RuleNodes have been destroyed by the time the RuleTree is destroyed, we want reliable leak checking.

We're going to use null to indicate that the final GC has already occurred.

Comment on attachment 8882653 [details] [diff] [review] Part 2 - Hook into the Gecko leak checking machinery. v1 Review of attachment 8882653 [details] [diff] [review]: ----------------------------------------------------------------- r=me with those ::: servo/components/style/rule_tree/mod.rs @@ +586,5 @@ > + pub fn NS_LogDtor(aPtr: *const c_void, aTypeName: *const c_char, aSize: u32); > +} > + > +lazy_static! { > + static ref NAME: CString = CString::new("RuleNode").unwrap(); instead of lazy_static!: static NAME: &'static [u8] = b"RuleNode\0"; @@ +592,5 @@ > + > +/// Logs the creation of a heap-allocated object to Gecko's leak-checking machinery. > +pub fn log_ctor(ptr: *const RuleNode) { > + unsafe { > + NS_LogCtor(ptr as *const c_void, NAME.as_ptr(), size_of::<RuleNode>() as u32); You need to cast the pointer to *const c_char, which is not guaranteed to be unsigned. @@ +599,5 @@ > + > +/// Logs the destruction of a heap-allocated object to Gecko's leak-checking machinery. > +pub fn log_dtor(ptr: *const RuleNode) { > + unsafe { > + NS_LogDtor(ptr as *const c_void, NAME.as_ptr(), size_of::<RuleNode>() as u32); ditto. @@ +778,5 @@ > fn new(n: Box<RuleNode>) -> Self { > debug_assert!(n.parent.is_none() == !n.source.is_some()); > > let ptr = Box::into_raw(n); > + log_new(ptr); You're missing two while inserting in the rule tree, please update them too. @@ +997,5 @@ > } > > debug!("GC'ing {:?}", weak.ptr()); > node.remove_from_child_list(); > + let ptr = Box::from_raw(weak.ptr()); You can also: log_drop(weak.ptr()); let _ = Box::from_raw(weak.ptr()); Which avoids the &*.

Comment on attachment 8882655 [details] [diff] [review] Part 4 - Allow StrongRuleNode drops after RuleTree destruction. v1 Review of attachment 8882655 [details] [diff] [review]: ----------------------------------------------------------------- ::: servo/components/style/rule_tree/mod.rs @@ +57,5 @@ > + > + // Remove the sentinel. This indicates that GCs will no longer occur. > + // Any further drops of StrongRuleNodes must occur on the main thread, > + // and will trigger synchronous dropping of the Rule nodes. > + self.root.get().next_free.store(ptr::null_mut(), Ordering::Relaxed); Please document it properly in the free list pointer :)

https://github.com/servo/servo/pull/17585 https://hg.mozilla.org/integration/autoland/rev/281a72862b4883a67b3c0370d12100ac8396707c

Pushed by bholley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/be418ade341b Test case for getting the computed style for a node in a document in the bfcache. r=bholley