Fix Stack Overflow for `Debug::fmt` (#630)

* Fix infinite recursion for Debug::fmt

* Limit recursion in `Debug for GcObject`

* PR feedback

* Add comment to `RecursionLimiter::VISITED`

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<GcCell<Object>>);
 
 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<HashSet<usize>> = 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;

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() {