diff --git a/boa/src/builtins/object/gcobject.rs b/boa/src/builtins/object/gcobject.rs index cf594e5705..6db9b76904 100644 --- a/boa/src/builtins/object/gcobject.rs +++ b/boa/src/builtins/object/gcobject.rs @@ -4,10 +4,14 @@ use super::Object; use gc::{Finalize, Gc, GcCell, GcCellRef, GcCellRefMut, Trace}; -use std::fmt::{self, Display}; +use std::{ + cell::RefCell, + collections::HashSet, + fmt::{self, Debug, Display}, +}; /// Garbage collected `Object`. -#[derive(Debug, Trace, Finalize, Clone)] +#[derive(Trace, Finalize, Clone)] pub struct GcObject(Gc>); impl GcObject { @@ -61,6 +65,76 @@ impl Display for BorrowError { } } +#[derive(Debug)] +/// Prevents infinite recursion during `Debug::fmt`. +struct RecursionLimiter { + /// If this was the first `GcObject` in the tree. + free: bool, + /// If this is the first time a specific `GcObject` has been seen. + first: bool, +} + +impl Clone for RecursionLimiter { + fn clone(&self) -> Self { + Self { + // Cloning this value would result in a premature free. + free: false, + // Cloning this vlaue would result in a value being written multiple times. + first: false, + } + } +} + +impl Drop for RecursionLimiter { + fn drop(&mut self) { + // Typically, calling hs.remove(ptr) for "first" objects would be the correct choice here. This would allow the + // same object to appear multiple times in the output (provided it does not appear under itself recursively). + // However, the JS object hierarchy involves quite a bit of repitition, and the sheer amount of data makes + // understanding the Debug output impossible; limiting the usefulness of it. + // + // Instead, the entire hashset is emptied at by the first GcObject involved. This means that objects will appear + // at most once, throughout the graph, hopefully making things a bit clearer. + if self.free { + Self::VISITED.with(|hs| hs.borrow_mut().clear()); + } + } +} + +impl RecursionLimiter { + thread_local! { + /// The list of pointers to `GcObject` that have been visited during the current `Debug::fmt` graph. + static VISITED: RefCell> = RefCell::new(HashSet::new()); + } + + /// Determines if the specified `GcObject` has been visited, and returns a struct that will free it when dropped. + /// + /// This is done by maintaining a thread-local hashset containing the pointers of `GcObject` values that have been + /// visited. The first `GcObject` visited will clear the hashset, while any others will check if they are contained + /// by the hashset. + fn new(o: &GcObject) -> Self { + // We shouldn't have to worry too much about this being moved during Debug::fmt. + let ptr = (o.as_ref() as *const _) as usize; + let (free, first) = Self::VISITED.with(|hs| { + let mut hs = hs.borrow_mut(); + (hs.is_empty(), hs.insert(ptr)) + }); + + Self { free, first } + } +} + +impl Debug for GcObject { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result { + let limiter = RecursionLimiter::new(&self); + + if limiter.first { + f.debug_tuple("GcObject").field(&self.0).finish() + } else { + f.write_str("{ ... }") + } + } +} + /// An error returned by [`GcObject::try_borrow_mut`](struct.GcObject.html#method.try_borrow_mut). #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct BorrowMutError; diff --git a/boa/src/builtins/value/tests.rs b/boa/src/builtins/value/tests.rs index acec8d475e..362fead94b 100644 --- a/boa/src/builtins/value/tests.rs +++ b/boa/src/builtins/value/tests.rs @@ -449,6 +449,20 @@ fn display_negative_zero_object() { assert_eq!(value.display().to_string(), "Number { -0 }") } +#[test] +fn debug_object() { + let realm = Realm::create(); + let mut engine = Interpreter::new(realm); + let value = forward_val(&mut engine, "new Array([new Date()])").unwrap(); + + // We don't care about the contents of the debug display (it is *debug* after all). In the commit that this test was + // added, this would cause a stack overflow, so executing Debug::fmt is the assertion. + // + // However, we want to make sure that no data is being left in the internal hashset, so executing this twice should + // result in the same output. + assert_eq!(format!("{:?}", value), format!("{:?}", value)); +} + #[test] #[ignore] // TODO: Once objects are printed in a simpler way this test can be simplified and used fn display_object() {