servo: Merge #1915 - Return a JS<T> from *Binding::Wrap rather than a…

… JSObject (from Ms2ger:wrap-return-js); r=jdm

This lets us avoid the sketchy tricks in JS::new and Window::new, where we
kept an unsafe pointer to the native object across the Wrap call that
consumed the owned pointer.

Source-Repo: https://github.com/servo/servo
Source-Revision: 7f188500a12373677c7ef9feb999276c30f1068a

servo/src/components/script/dom/bindings/codegen/CodegenRust.py

@@ -2060,7 +2060,7 @@ def DOMObjectPointerArg(descriptor):
     return DOMObjectPointerType(descriptor) + descriptor.concreteType
 
 def CreateBindingJSObject(descriptor, parent=None):
-    create = "  let raw: *mut %s = &mut *aObject;\n" % descriptor.concreteType;
+    create = "  let mut raw: JS<%s> = JS::from_raw(&mut *aObject);\n" % descriptor.concreteType
     if descriptor.proxy:
         assert not descriptor.createGlobal
         handler = """
@@ -2071,19 +2071,15 @@ def CreateBindingJSObject(descriptor, parent=None):
                            &PrivateValue(squirrel_away_unboxed(aObject) as *libc::c_void),
                            proto, %s,
                            ptr::null(), ptr::null());
-  if obj.is_null() {
-    return ptr::null();
-  }
+  assert!(obj.is_not_null());
 
 """ % (parent)
     else:
         if descriptor.createGlobal:
             create += "  let obj = CreateDOMGlobal(aCx, &Class.base);\n"
         else:
             create += "  let obj = JS_NewObject(aCx, &Class.base, proto, %s);\n" % parent
-        create += """  if obj.is_null() {
-    return ptr::null();
-  }
+        create += """  assert!(obj.is_not_null());
 
   JS_SetReservedSlot(obj, DOM_OBJECT_SLOT as u32,
                      PrivateValue(squirrel_away_unboxed(aObject) as *libc::c_void));
@@ -2099,7 +2095,8 @@ def __init__(self, descriptor):
         else:
             args = [Argument('*JSContext', 'aCx'),
                     Argument(DOMObjectPointerArg(descriptor), 'aObject', mutable=True)]
-        CGAbstractMethod.__init__(self, descriptor, 'Wrap', '*JSObject', args, pub=True)
+        retval = 'JS<%s>' % descriptor.concreteType
+        CGAbstractMethod.__init__(self, descriptor, 'Wrap', retval, args, pub=True)
 
     def definition_body(self):
         if not self.descriptor.createGlobal:
@@ -2110,22 +2107,20 @@ def definition_body(self):
 
   //JSAutoCompartment ac(aCx, scope);
   let proto = GetProtoObject(aCx, scope, scope);
-  if proto.is_null() {
-    return ptr::null();
-  }
+  assert!(proto.is_not_null());
 
 %s
 
-  (*raw).mut_reflector().set_jsobject(obj);
+  raw.mut_reflector().set_jsobject(obj);
 
-  return obj;""" % CreateBindingJSObject(self.descriptor, "scope")
+  return raw;""" % CreateBindingJSObject(self.descriptor, "scope")
         else:
             return """
 %s
   let proto = GetProtoObject(aCx, obj, obj);
   JS_SetPrototype(aCx, obj, proto);
-  (*raw).mut_reflector().set_jsobject(obj);
-  return obj;""" % CreateBindingJSObject(self.descriptor)
+  raw.mut_reflector().set_jsobject(obj);
+  return raw;""" % CreateBindingJSObject(self.descriptor)
 
 class CGAbstractExternMethod(CGAbstractMethod):
     """

servo/src/components/script/dom/bindings/js.rs

@@ -4,7 +4,7 @@
 
 use dom::bindings::utils::{Reflector, Reflectable};
 use dom::window::Window;
-use js::jsapi::{JSContext, JSObject};
+use js::jsapi::JSContext;
 use layout_interface::TrustedNodeAddress;
 
 use std::cast;
@@ -29,17 +29,10 @@ impl <T> Clone for JS<T> {
 }
 
 impl<T: Reflectable> JS<T> {
-    pub fn new(mut obj: ~T,
+    pub fn new(obj: ~T,
                window:  &JS<Window>,
-               wrap_fn: extern "Rust" fn(*JSContext, &JS<Window>, ~T) -> *JSObject) -> JS<T> {
-        let cx = window.get().get_cx();
-        let raw: *mut T = &mut *obj;
-        if wrap_fn(cx, window, obj).is_null() {
-            fail!("Could not eagerly wrap object");
-        }
-        JS {
-            ptr: RefCell::new(raw)
-        }
+               wrap_fn: extern "Rust" fn(*JSContext, &JS<Window>, ~T) -> JS<T>) -> JS<T> {
+        wrap_fn(window.get().get_cx(), window, obj)
     }
 
     pub unsafe fn from_raw(raw: *mut T) -> JS<T> {

servo/src/components/script/dom/bindings/utils.rs

@@ -424,7 +424,7 @@ pub trait Reflectable {
 pub fn reflect_dom_object<T: Reflectable>
         (obj:     ~T,
          window:  &JS<window::Window>,
-         wrap_fn: extern "Rust" fn(*JSContext, &JS<window::Window>, ~T) -> *JSObject)
+         wrap_fn: extern "Rust" fn(*JSContext, &JS<window::Window>, ~T) -> JS<T>)
          ->       JS<T> {
     JS::new(obj, window, wrap_fn)
 }

servo/src/components/script/dom/document.rs

@@ -40,7 +40,7 @@ use servo_util::str::DOMString;
 
 use collections::hashmap::HashMap;
 use extra::url::{Url, from_str};
-use js::jsapi::{JSObject, JSContext};
+use js::jsapi::JSContext;
 use std::ascii::StrAsciiExt;
 
 use serialize::{Encoder, Encodable};
@@ -87,7 +87,7 @@ impl Document {
     pub fn reflect_document<D: Reflectable+DocumentBase>
             (document:  ~D,
              window:    &JS<Window>,
-             wrap_fn:   extern "Rust" fn(*JSContext, &JS<Window>, ~D) -> *JSObject)
+             wrap_fn:   extern "Rust" fn(*JSContext, &JS<Window>, ~D) -> JS<D>)
              -> JS<D> {
         assert!(document.reflector().get_jsobject().is_null());
         let raw_doc = reflect_dom_object(document, window, wrap_fn);

servo/src/components/script/dom/node.rs

@@ -769,7 +769,7 @@ impl Node {
     pub fn reflect_node<N: Reflectable+NodeBase>
             (node:      ~N,
              document:  &JS<Document>,
-             wrap_fn:   extern "Rust" fn(*JSContext, &JS<Window>, ~N) -> *JSObject)
+             wrap_fn:   extern "Rust" fn(*JSContext, &JS<Window>, ~N) -> JS<N>)
              -> JS<N> {
         assert!(node.reflector().get_jsobject().is_null());
         let node = reflect_dom_object(node, &document.get().window, wrap_fn);

servo/src/components/script/dom/window.rs

@@ -284,7 +284,7 @@ impl Window {
                compositor: ~ScriptListener,
                image_cache_task: ImageCacheTask)
                -> JS<Window> {
-        let mut win = ~Window {
+        let win = ~Window {
             eventtarget: EventTarget::new_inherited(WindowTypeId),
             script_chan: script_chan.clone(),
             console: None,
@@ -314,23 +314,22 @@ impl Window {
             next_timer_handle: 0
         };
 
-        let raw: *mut Window = &mut *win;
         let global = WindowBinding::Wrap(cx, win);
-        assert!(global.is_not_null());
-        unsafe {
-            let fn_names = ["window","self"];
-            for str in fn_names.iter() {
-                (*str).to_c_str().with_ref(|name| {
-                    JS_DefineProperty(cx, global,  name,
-                                      ObjectValue(&*global),
+        let fn_names = ["window", "self"];
+        for str in fn_names.iter() {
+            (*str).to_c_str().with_ref(|name| {
+                let object = global.reflector().get_jsobject();
+                assert!(object.is_not_null());
+                unsafe {
+                    JS_DefineProperty(cx, object, name,
+                                      ObjectValue(&*object),
                                       Some(cast::transmute(GetJSClassHookStubPointer(PROPERTY_STUB))),
                                       Some(cast::transmute(GetJSClassHookStubPointer(STRICT_PROPERTY_STUB))),
                                       JSPROP_ENUMERATE);
-                })
-
-            }
+                }
+            })
 
-            JS::from_raw(raw)
         }
+        global
     }
 }