Auto merge of #11113 - jdm:trace_browsingcontext, r=Ms2ger

Trace and finalize BrowsingContext

This is a prerequisite for merging #11044, and is an important correctness fix on its own.

r? @Ms2ger

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11113)
<!-- Reviewable:end -->

7 files changed, 46 insertions, 9 deletions

diff --git a/components/script/dom/bindings/js.rs b/components/script/dom/bindings/js.rs
index 10b9a481382..56193b543ad 100644
--- a/components/script/dom/bindings/js.rs
+++ b/components/script/dom/bindings/js.rs
@@ -38,6 +38,7 @@ use script_thread::STACK_ROOTS;
 use std::cell::UnsafeCell;
 use std::default::Default;
 use std::hash::{Hash, Hasher};
+use std::intrinsics::type_name;
 use std::mem;
 use std::ops::Deref;
 use std::ptr;
@@ -106,7 +107,16 @@ impl<T: Reflectable> Deref for JS<T> {
 
 impl<T: Reflectable> JSTraceable for JS<T> {
     fn trace(&self, trc: *mut JSTracer) {
-        trace_reflector(trc, "", unsafe { (**self.ptr).reflector() });
+        #[cfg(debug_assertions)]
+        let trace_str = format!("for {} on heap", unsafe { type_name::<T>() });
+        #[cfg(debug_assertions)]
+        let trace_info = &trace_str[..];
+        #[cfg(not(debug_assertions))]
+        let trace_info = "for DOM object on heap";
+
+        trace_reflector(trc,
+                        trace_info,
+                        unsafe { (**self.ptr).reflector() });
     }
 }
 
@@ -520,11 +530,12 @@ impl RootCollection {
 
 /// SM Callback that traces the rooted reflectors
 pub unsafe fn trace_roots(tracer: *mut JSTracer) {
+    debug!("tracing stack roots");
     STACK_ROOTS.with(|ref collection| {
         let RootCollectionPtr(collection) = collection.get().unwrap();
         let collection = &*(*collection).roots.get();
         for root in collection {
-            trace_reflector(tracer, "reflector", &**root);
+            trace_reflector(tracer, "on stack", &**root);
         }
     });
 }
diff --git a/components/script/dom/bindings/refcounted.rs b/components/script/dom/bindings/refcounted.rs
index 057c67c0ef4..25239a3dc3b 100644
--- a/components/script/dom/bindings/refcounted.rs
+++ b/components/script/dom/bindings/refcounted.rs
@@ -201,13 +201,14 @@ impl LiveDOMReferences {
 /// A JSTraceDataOp for tracing reflectors held in LIVE_REFERENCES
 pub unsafe extern "C" fn trace_refcounted_objects(tracer: *mut JSTracer,
                                                   _data: *mut os::raw::c_void) {
+    debug!("tracing live refcounted references");
     LIVE_REFERENCES.with(|ref r| {
         let r = r.borrow();
         let live_references = r.as_ref().unwrap();
         let table = live_references.table.borrow();
         for obj in table.keys() {
             let reflectable = &*(*obj as *const Reflector);
-            trace_reflector(tracer, "LIVE_REFERENCES", reflectable);
+            trace_reflector(tracer, "refcounted", reflectable);
         }
     });
 }
diff --git a/components/script/dom/bindings/trace.rs b/components/script/dom/bindings/trace.rs
index 1ec19ac15bc..e7377c82e69 100644
--- a/components/script/dom/bindings/trace.rs
+++ b/components/script/dom/bindings/trace.rs
@@ -171,14 +171,14 @@ impl JSTraceable for Heap<*mut JSObject> {
         if self.get().is_null() {
             return;
         }
-        trace_object(trc, "object", self);
+        trace_object(trc, "heap object", self);
     }
 }
 
 
 impl JSTraceable for Heap<JSVal> {
     fn trace(&self, trc: *mut JSTracer) {
-        trace_jsval(trc, "val", self);
+        trace_jsval(trc, "heap value", self);
     }
 }
 
@@ -552,6 +552,7 @@ impl<A: JSTraceable + Reflectable> FromIterator<Root<A>> for RootedVec<JS<A>> {
 
 /// SM Callback that traces the rooted traceables
 pub unsafe fn trace_traceables(tracer: *mut JSTracer) {
+    debug!("tracing stack-rooted traceables");
     ROOTED_TRACEABLES.with(|ref traceables| {
         let traceables = traceables.borrow();
         traceables.trace(tracer);
diff --git a/components/script/dom/browsingcontext.rs b/components/script/dom/browsingcontext.rs
index b314bce603d..f4f15cdf7c7 100644
--- a/components/script/dom/browsingcontext.rs
+++ b/components/script/dom/browsingcontext.rs
@@ -7,6 +7,7 @@ use dom::bindings::conversions::{ToJSValConvertible, root_from_handleobject};
 use dom::bindings::js::{JS, Root, RootedReference};
 use dom::bindings::proxyhandler::{fill_property_descriptor, get_property_descriptor};
 use dom::bindings::reflector::{Reflectable, Reflector};
+use dom::bindings::trace::JSTraceable;
 use dom::bindings::utils::WindowProxyHandler;
 use dom::bindings::utils::get_array_index_from_id;
 use dom::document::Document;
@@ -14,10 +15,10 @@ use dom::element::Element;
 use dom::window::Window;
 use js::JSCLASS_IS_GLOBAL;
 use js::glue::{CreateWrapperProxyHandler, ProxyTraps, NewWindowProxy};
-use js::glue::{GetProxyPrivate, SetProxyExtra};
+use js::glue::{GetProxyPrivate, SetProxyExtra, GetProxyExtra};
 use js::jsapi::{Handle, HandleId, HandleObject, HandleValue, JSAutoCompartment, JSAutoRequest};
 use js::jsapi::{JSContext, JSPROP_READONLY, JSErrNum, JSObject, PropertyDescriptor, JS_DefinePropertyById};
-use js::jsapi::{JS_ForwardGetPropertyTo, JS_ForwardSetPropertyTo, JS_GetClass};
+use js::jsapi::{JS_ForwardGetPropertyTo, JS_ForwardSetPropertyTo, JS_GetClass, JSTracer, FreeOp};
 use js::jsapi::{JS_GetOwnPropertyDescriptorById, JS_HasPropertyById, MutableHandle};
 use js::jsapi::{MutableHandleValue, ObjectOpResult, RootedObject, RootedValue};
 use js::jsval::{UndefinedValue, PrivateValue};
@@ -261,14 +262,32 @@ static PROXY_HANDLER: ProxyTraps = ProxyTraps {
     fun_toString: None,
     boxedValue_unbox: None,
     defaultValue: None,
-    trace: None,
-    finalize: None,
+    trace: Some(trace),
+    finalize: Some(finalize),
     objectMoved: None,
     isCallable: None,
     isConstructor: None,
 };
 
 #[allow(unsafe_code)]
+unsafe extern fn finalize(_fop: *mut FreeOp, obj: *mut JSObject) {
+    let this = GetProxyExtra(obj, 0).to_private() as *mut BrowsingContext;
+    assert!(!this.is_null());
+    let _ = Box::from_raw(this);
+    debug!("BrowsingContext finalize: {:p}", this);
+}
+
+#[allow(unsafe_code)]
+unsafe extern fn trace(trc: *mut JSTracer, obj: *mut JSObject) {
+    let this = GetProxyExtra(obj, 0).to_private() as *const BrowsingContext;
+    if this.is_null() {
+        // GC during obj creation
+        return;
+    }
+    (*this).trace(trc);
+}
+
+#[allow(unsafe_code)]
 pub fn new_window_proxy_handler() -> WindowProxyHandler {
     unsafe {
         WindowProxyHandler(CreateWrapperProxyHandler(&PROXY_HANDLER))
diff --git a/components/script/lib.rs b/components/script/lib.rs
index 4feaae05845..e8b22c353a0 100644
--- a/components/script/lib.rs
+++ b/components/script/lib.rs
@@ -17,6 +17,7 @@
 #![feature(peekable_is_empty)]
 #![feature(plugin)]
 #![feature(slice_patterns)]
+#![feature(stmt_expr_attributes)]
 
 #![deny(unsafe_code)]
 #![allow(non_snake_case)]
diff --git a/components/script/script_runtime.rs b/components/script/script_runtime.rs
index 783433b9a21..4a6d73949a9 100644
--- a/components/script/script_runtime.rs
+++ b/components/script/script_runtime.rs
@@ -371,7 +371,9 @@ unsafe extern "C" fn debug_gc_callback(_rt: *mut JSRuntime, status: JSGCStatus,
 
 #[allow(unsafe_code)]
 unsafe extern fn trace_rust_roots(tr: *mut JSTracer, _data: *mut os::raw::c_void) {
+    debug!("starting custom root handler");
     trace_thread(tr);
     trace_traceables(tr);
     trace_roots(tr);
+    debug!("done custom root handler");
 }
diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs
index a46c1a28b18..1d3bc4c4bcf 100644
--- a/components/script/script_thread.rs
+++ b/components/script/script_thread.rs
@@ -115,6 +115,7 @@ thread_local!(static SCRIPT_THREAD_ROOT: RefCell<Option<*const ScriptThread>> =
 pub unsafe fn trace_thread(tr: *mut JSTracer) {
     SCRIPT_THREAD_ROOT.with(|root| {
         if let Some(script_thread) = *root.borrow() {
+            debug!("tracing fields of ScriptThread");
             (*script_thread).trace(tr);
         }
     });
@@ -1357,6 +1358,7 @@ impl ScriptThread {
 
         // otherwise find just the matching page and exit all sub-pages
         if let Some(ref mut child_page) = page.remove(id) {
+            debug!("shutting down layout for child context {:?}", id);
             shut_down_layout(&*child_page);
         }
         false