From 31b1233a734819d355d4582e306c818208190a59 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Fri, 30 Jun 2017 00:15:23 -0700 Subject: [PATCH 1/4] Stop GCing before dropping the root rule node. We already gc before dropping the RuleTree. --- components/style/rule_tree/mod.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index f4fe5da37be..d6f5abfe3d4 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -1329,10 +1329,8 @@ impl Drop for StrongRuleNode { ptr::null_mut()); if node.parent.is_none() { debug!("Dropping root node!"); - // NOTE: Calling this is fine, because the rule tree root - // destructor needs to happen from the layout thread, where the - // stylist, and hence, the rule tree, is held. - unsafe { self.gc() }; + // The free list should be null by this point + debug_assert!(node.next_free.load(Ordering::Relaxed).is_null()); let _ = unsafe { Box::from_raw(self.ptr()) }; return; } From 654661df3615439891084a2f8ace89a63973275c Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 29 Jun 2017 23:56:23 -0700 Subject: [PATCH 2/4] Hook into the Gecko leak checking machinery. 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. --- components/style/rule_tree/mod.rs | 50 +++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index d6f5abfe3d4..abbfe7f3efb 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -571,6 +571,53 @@ pub struct RuleNode { unsafe impl Sync for RuleTree {} unsafe impl Send for RuleTree {} +// On Gecko builds, hook into the leak checking machinery. +#[cfg(feature = "gecko")] +#[cfg(debug_assertions)] +mod gecko_leak_checking { +use std::mem::size_of; +use std::os::raw::{c_char, c_void}; +use super::RuleNode; + +extern "C" { + pub fn NS_LogCtor(aPtr: *const c_void, aTypeName: *const c_char, aSize: u32); + pub fn NS_LogDtor(aPtr: *const c_void, aTypeName: *const c_char, aSize: u32); +} + +static NAME: &'static [u8] = b"RuleNode\0"; + +/// Logs the creation of a heap-allocated object to Gecko's leak-checking machinery. +pub fn log_ctor(ptr: *const RuleNode) { + let s = NAME as *const [u8] as *const u8 as *const c_char; + unsafe { + NS_LogCtor(ptr as *const c_void, s, size_of::() as u32); + } +} + +/// Logs the destruction of a heap-allocated object to Gecko's leak-checking machinery. +pub fn log_dtor(ptr: *const RuleNode) { + let s = NAME as *const [u8] as *const u8 as *const c_char; + unsafe { + NS_LogDtor(ptr as *const c_void, s, size_of::() as u32); + } +} + +} + +#[inline(always)] +fn log_new(_ptr: *const RuleNode) { + #[cfg(feature = "gecko")] + #[cfg(debug_assertions)] + gecko_leak_checking::log_ctor(_ptr); +} + +#[inline(always)] +fn log_drop(_ptr: *const RuleNode) { + #[cfg(feature = "gecko")] + #[cfg(debug_assertions)] + gecko_leak_checking::log_dtor(_ptr); +} + impl RuleNode { fn new(root: WeakRuleNode, parent: StrongRuleNode, @@ -730,6 +777,7 @@ impl StrongRuleNode { debug_assert!(n.parent.is_none() == !n.source.is_some()); let ptr = Box::into_raw(n); + log_new(ptr); debug!("Creating rule node: {:p}", ptr); @@ -948,6 +996,7 @@ impl StrongRuleNode { debug!("GC'ing {:?}", weak.ptr()); node.remove_from_child_list(); + log_drop(weak.ptr()); let _ = Box::from_raw(weak.ptr()); } @@ -1331,6 +1380,7 @@ impl Drop for StrongRuleNode { debug!("Dropping root node!"); // The free list should be null by this point debug_assert!(node.next_free.load(Ordering::Relaxed).is_null()); + log_drop(self.ptr()); let _ = unsafe { Box::from_raw(self.ptr()) }; return; } From 67cd9746364e327e2da9ebceabff9d284d72c268 Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 29 Jun 2017 23:30:33 -0700 Subject: [PATCH 3/4] Use a different sentinel value to lock the free list. We're going to use null to indicate that the final GC has already occurred. --- components/style/rule_tree/mod.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index abbfe7f3efb..07b14370f40 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -122,6 +122,10 @@ impl StyleSource { /// The root node doesn't have a null pointer in the free list, but this value. const FREE_LIST_SENTINEL: *mut RuleNode = 0x01 as *mut RuleNode; +/// A second sentinel value for the free list, indicating that it's locked (i.e. +/// another thread is currently adding an entry). We spin if we find this value. +const FREE_LIST_LOCKED: *mut RuleNode = 0x02 as *mut RuleNode; + impl RuleTree { /// Construct a new rule tree. pub fn new() -> Self { @@ -1395,7 +1399,7 @@ impl Drop for StrongRuleNode { return; } - // Ensure we "lock" the free list head swapping it with a null pointer. + // Ensure we "lock" the free list head swapping it with FREE_LIST_LOCKED. // // Note that we use Acquire/Release semantics for the free list // synchronization, in order to guarantee that the next_free @@ -1404,11 +1408,11 @@ impl Drop for StrongRuleNode { let mut old_head = free_list.load(Ordering::Relaxed); loop { match free_list.compare_exchange_weak(old_head, - ptr::null_mut(), + FREE_LIST_LOCKED, Ordering::Acquire, Ordering::Relaxed) { Ok(..) => { - if !old_head.is_null() { + if old_head != FREE_LIST_LOCKED { break; } }, From 6ade9c11ecaff8c8cfedd5a2b1d8e66c543e904a Mon Sep 17 00:00:00 2001 From: Bobby Holley Date: Thu, 29 Jun 2017 23:20:26 -0700 Subject: [PATCH 4/4] Allow StrongRuleNode drops after RuleTree destruction. --- components/style/rule_tree/mod.rs | 40 ++++++++++++++++++++++++++----- components/style/stylist.rs | 15 ------------ 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/components/style/rule_tree/mod.rs b/components/style/rule_tree/mod.rs index 07b14370f40..b4603dbb51c 100644 --- a/components/style/rule_tree/mod.rs +++ b/components/style/rule_tree/mod.rs @@ -47,6 +47,21 @@ pub struct RuleTree { root: StrongRuleNode, } +impl Drop for RuleTree { + fn drop(&mut self) { + // GC the rule tree. + unsafe { self.gc(); } + + // After the GC, the free list should be empty. + debug_assert!(self.root.get().next_free.load(Ordering::Relaxed) == FREE_LIST_SENTINEL); + + // 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); + } +} + /// A style source for the rule node. It can either be a CSS style rule or a /// declaration block. /// @@ -139,11 +154,6 @@ impl RuleTree { self.root.clone() } - /// Returns true if there are no nodes in the tree aside from the rule node. - pub fn is_empty(&self) -> bool { - self.root.get().first_child.load(Ordering::Relaxed).is_null() - } - fn dump(&self, guards: &StylesheetGuards, writer: &mut W) { let _ = writeln!(writer, " + RuleTree"); self.root.get().dump(guards, writer, 0); @@ -569,6 +579,11 @@ pub struct RuleNode { prev_sibling_or_free_count: PrevSiblingOrFreeCount, /// The next item in the rule tree free list, that starts on the root node. + /// + /// When this is set to null, that means that the rule tree has been torn + /// down, and GCs will no longer occur. When this happens, StrongRuleNodes + /// may only be dropped on the main thread, and teardown happens + /// synchronously. next_free: AtomicPtr, } @@ -1391,6 +1406,20 @@ impl Drop for StrongRuleNode { let root = unsafe { &*node.root.as_ref().unwrap().ptr() }; let free_list = &root.next_free; + let mut old_head = free_list.load(Ordering::Relaxed); + + // If the free list is null, that means the last GC has already occurred. + // We require that any callers freeing at this point are on the main + // thread, and we drop the rule node synchronously. + if old_head.is_null() { + debug_assert!(!thread_state::get().is_worker() && + (thread_state::get().is_layout() || + thread_state::get().is_script())); + unsafe { node.remove_from_child_list(); } + log_drop(self.ptr()); + let _ = unsafe { Box::from_raw(self.ptr()) }; + return; + } // We're sure we're already in the free list, don't spinloop if we're. // Note that this is just a fast path, so it doesn't need to have an @@ -1405,7 +1434,6 @@ impl Drop for StrongRuleNode { // synchronization, in order to guarantee that the next_free // reads/writes we do below are properly visible from multiple threads // racing. - let mut old_head = free_list.load(Ordering::Relaxed); loop { match free_list.compare_exchange_weak(old_head, FREE_LIST_LOCKED, diff --git a/components/style/stylist.rs b/components/style/stylist.rs index da2bfd6a1e6..1eb4965e3e9 100644 --- a/components/style/stylist.rs +++ b/components/style/stylist.rs @@ -1369,21 +1369,6 @@ impl Stylist { } } -impl Drop for Stylist { - fn drop(&mut self) { - // This is the last chance to GC the rule tree. If we have dropped all - // strong rule node references before the Stylist is dropped, then this - // will cause the rule tree to be destroyed correctly. If we haven't - // dropped all strong rule node references before now, then we will - // leak them, since there will be no way to call gc() on the rule tree - // after this point. - unsafe { self.rule_tree.gc(); } - - // Assert against leaks. - debug_assert!(self.rule_tree.is_empty()); - } -} - /// Visitor to collect names that appear in attribute selectors and any /// dependencies on ElementState bits. struct AttributeAndStateDependencyVisitor<'a> {