servo: Merge #17056 - Make dictionaries and unions containing GC values safer (from jdm:heapdict); r=emilio
authorJosh Matthews <josh@joshmatthews.net>
Mon, 25 Sep 2017 16:07:47 -0500
changeset 670382 c47c1d957c1968e040cbc2a5475f7f1fd1b5959f
parent 670381 4c23cc64c5270d83cd3b07e30eb74ba86a79b3cc
child 670383 7fc37806848f3047db904293ffa4fe936cd1c8b0
push id81613
push userbmo:nchevobbe@mozilla.com
push dateTue, 26 Sep 2017 10:17:11 +0000
reviewersemilio
milestone58.0a1
servo: Merge #17056 - Make dictionaries and unions containing GC values safer (from jdm:heapdict); r=emilio Problems: * the Heap::new constructor is memory-unsafe with any value other than Undefined/Null * this means that moving dictionaries containing Heap values (ie. any/object) is memory-unsafe * unions containing GC pointers are similarly unsafe Solutions: - dictionaries containing GC pointers are now wrapped in RootedTraceableBox (even inside other dictionaries) - instead of using Heap::new, dictionaries containing GC pointers are now initialized to a default value (by deriving Default) and mutated one field at a time - dictionaries containing GC pointers are marked #[must_root] - FromJSVal for dictionaries containing GC pointers now returns RootedTraceableBox<Dictionary> - unions wrap their variants that require rooting in RootedTraceableBox Rather than attempting to derive Default for all dictionaries, we only do so for the dictionaries that require it. Because some dictionaries that require it inherit from dictionaries that do not, we need to write manual implementations for those parent dictionaries. This is a better option than having to figure out a default value for types like `Root<T>`, which would be required for deriving Default for all dictionaries. I would still like to come up with an automated test for this, but I figured I would get eyes on this first. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #16952 - [ ] There are tests for these changes Source-Repo: https://github.com/servo/servo Source-Revision: 532dee36c10b7dd5d33e560b55cf65c7243ef1d3
servo/components/script/dom/bindings/codegen/CodegenRust.py
servo/components/script/dom/bindings/conversions.rs
servo/components/script/dom/bindings/iterable.rs
servo/components/script/dom/bindings/js.rs
servo/components/script/dom/bindings/num.rs
servo/components/script/dom/bindings/str.rs
servo/components/script/dom/bindings/trace.rs
servo/components/script/dom/event.rs
servo/components/script/dom/extendableevent.rs
servo/components/script/dom/filereader.rs
servo/components/script/dom/testbinding.rs
--- a/servo/components/script/dom/bindings/codegen/CodegenRust.py
+++ b/servo/components/script/dom/bindings/codegen/CodegenRust.py
@@ -718,19 +718,16 @@ def getJSToNativeConversionInfo(type, de
 
         return handleOptional(templateBody, declType, handleDefaultNull("None"))
 
     if type.isUnion():
         declType = CGGeneric(union_native_type(type))
         if type.nullable():
             declType = CGWrapper(declType, pre="Option<", post=" >")
 
-        if isMember != "Dictionary" and type_needs_tracing(type):
-            declType = CGTemplatedType("RootedTraceableBox", declType)
-
         templateBody = ("match FromJSValConvertible::from_jsval(cx, ${val}, ()) {\n"
                         "    Ok(ConversionResult::Success(value)) => value,\n"
                         "    Ok(ConversionResult::Failure(error)) => {\n"
                         "%s\n"
                         "    }\n"
                         "    _ => { %s },\n"
                         "}" % (indent(failOrPropagate, 8), exceptionCode))
 
@@ -1043,22 +1040,22 @@ def getJSToNativeConversionInfo(type, de
         if isMember == "Dictionary":
             # TODO: Need to properly root dictionaries
             # https://github.com/servo/servo/issues/6381
             declType = CGGeneric("Heap<JSVal>")
 
             if defaultValue is None:
                 default = None
             elif isinstance(defaultValue, IDLNullValue):
-                default = "Heap::new(NullValue())"
+                default = "NullValue()"
             elif isinstance(defaultValue, IDLUndefinedValue):
-                default = "Heap::new(UndefinedValue())"
+                default = "UndefinedValue()"
             else:
                 raise TypeError("Can't handle non-null, non-undefined default value here")
-            return handleOptional("Heap::new(${val}.get())", declType, default)
+            return handleOptional("${val}.get()", declType, default)
 
         declType = CGGeneric("HandleValue")
 
         if defaultValue is None:
             default = None
         elif isinstance(defaultValue, IDLNullValue):
             default = "HandleValue::null()"
         elif isinstance(defaultValue, IDLUndefinedValue):
@@ -1075,18 +1072,16 @@ def getJSToNativeConversionInfo(type, de
         # https://github.com/servo/servo/issues/6382
         default = "ptr::null_mut()"
         templateBody = wrapObjectTemplate("${val}.get().to_object()",
                                           default,
                                           isDefinitelyObject, type, failureCode)
 
         if isMember in ("Dictionary", "Union"):
             declType = CGGeneric("Heap<*mut JSObject>")
-            templateBody = "Heap::new(%s)" % templateBody
-            default = "Heap::new(%s)" % default
         else:
             # TODO: Need to root somehow
             # https://github.com/servo/servo/issues/6382
             declType = CGGeneric("*mut JSObject")
 
         return handleOptional(templateBody, declType,
                               handleDefaultNull(default))
 
@@ -1094,19 +1089,18 @@ def getJSToNativeConversionInfo(type, de
         # There are no nullable dictionaries
         assert not type.nullable()
 
         typeName = "%s::%s" % (CGDictionary.makeModuleName(type.inner),
                                CGDictionary.makeDictionaryName(type.inner))
         declType = CGGeneric(typeName)
         empty = "%s::empty(cx)" % typeName
 
-        if isMember != "Dictionary" and type_needs_tracing(type):
+        if type_needs_tracing(type):
             declType = CGTemplatedType("RootedTraceableBox", declType)
-            empty = "RootedTraceableBox::new(%s)" % empty
 
         template = ("match FromJSValConvertible::from_jsval(cx, ${val}, ()) {\n"
                     "    Ok(ConversionResult::Success(dictionary)) => dictionary,\n"
                     "    Ok(ConversionResult::Failure(error)) => {\n"
                     "%s\n"
                     "    }\n"
                     "    _ => { %s },\n"
                     "}" % (indent(failOrPropagate, 8), exceptionCode))
@@ -1422,16 +1416,18 @@ def getRetvalDeclarationForType(returnTy
         result = wrapInNativeContainerType(returnType, result)
         if returnType.nullable():
             result = CGWrapper(result, pre="Option<", post=">")
         return result
     if returnType.isDictionary():
         nullable = returnType.nullable()
         dictName = returnType.inner.name if nullable else returnType.name
         result = CGGeneric(dictName)
+        if type_needs_tracing(returnType):
+            result = CGWrapper(result, pre="RootedTraceableBox<", post=">")
         if nullable:
             result = CGWrapper(result, pre="Option<", post=">")
         return result
 
     raise TypeError("Don't know how to declare return value for %s" %
                     returnType)
 
 
@@ -2257,16 +2253,17 @@ def UnionTypes(descriptors, dictionaries
         'dom::bindings::conversions::StringificationBehavior',
         'dom::bindings::conversions::root_from_handlevalue',
         'dom::bindings::error::throw_not_in_union',
         'dom::bindings::js::Root',
         'dom::bindings::mozmap::MozMap',
         'dom::bindings::str::ByteString',
         'dom::bindings::str::DOMString',
         'dom::bindings::str::USVString',
+        'dom::bindings::trace::RootedTraceableBox',
         'dom::types::*',
         'js::error::throw_type_error',
         'js::jsapi::HandleValue',
         'js::jsapi::Heap',
         'js::jsapi::JSContext',
         'js::jsapi::JSObject',
         'js::jsapi::MutableHandleValue',
         'js::jsval::JSVal',
@@ -4009,37 +4006,46 @@ class CGEnum(CGThing):
 #[derive(JSTraceable, PartialEq, Copy, Clone, HeapSizeOf, Debug)]
 pub enum %s {
     %s
 }
 """ % (ident, ",\n    ".join(map(getEnumValueName, enum.values())))
 
         pairs = ",\n    ".join(['("%s", super::%s::%s)' % (val, ident, getEnumValueName(val)) for val in enum.values()])
 
-        inner = """\
+        inner = string.Template("""\
 use dom::bindings::conversions::ToJSValConvertible;
 use js::jsapi::{JSContext, MutableHandleValue};
 use js::jsval::JSVal;
 
-pub const pairs: &'static [(&'static str, super::%s)] = &[
-    %s,
+pub const pairs: &'static [(&'static str, super::${ident})] = &[
+    ${pairs},
 ];
 
-impl super::%s {
+impl super::${ident} {
     pub fn as_str(&self) -> &'static str {
         pairs[*self as usize].0
     }
 }
 
-impl ToJSValConvertible for super::%s {
+impl Default for super::${ident} {
+    fn default() -> super::${ident} {
+        pairs[0].1
+    }
+}
+
+impl ToJSValConvertible for super::${ident} {
     unsafe fn to_jsval(&self, cx: *mut JSContext, rval: MutableHandleValue) {
         pairs[*self as usize].0.to_jsval(cx, rval);
     }
 }
-    """ % (ident, pairs, ident, ident)
+    """).substitute({
+            'ident': ident,
+            'pairs': pairs
+        })
         self.cgRoot = CGList([
             CGGeneric(decl),
             CGNamespace.build([ident + "Values"],
                               CGIndenter(CGGeneric(inner)), public=True),
         ])
 
     def define(self):
         return self.cgRoot.define()
@@ -4127,25 +4133,33 @@ class CGUnionStruct(CGThing):
     def __init__(self, type, descriptorProvider):
         assert not type.nullable()
         assert not type.hasNullableType
 
         CGThing.__init__(self)
         self.type = type
         self.descriptorProvider = descriptorProvider
 
+    def membersNeedTracing(self):
+        for t in self.type.flatMemberTypes:
+            if type_needs_tracing(t):
+                return True
+        return False
+
     def define(self):
-        templateVars = map(lambda t: getUnionTypeTemplateVars(t, self.descriptorProvider),
+        templateVars = map(lambda t: (getUnionTypeTemplateVars(t, self.descriptorProvider),
+                                      type_needs_tracing(t)),
                            self.type.flatMemberTypes)
         enumValues = [
-            "    %s(%s)," % (v["name"], v["typeName"]) for v in templateVars
+            "    %s(%s)," % (v["name"], "RootedTraceableBox<%s>" % v["typeName"] if trace else v["typeName"])
+            for (v, trace) in templateVars
         ]
         enumConversions = [
             "            %s::%s(ref inner) => inner.to_jsval(cx, rval),"
-            % (self.type, v["name"]) for v in templateVars
+            % (self.type, v["name"]) for (v, _) in templateVars
         ]
         return ("""\
 #[derive(JSTraceable)]
 pub enum %s {
 %s
 }
 
 impl ToJSValConvertible for %s {
@@ -4162,16 +4176,22 @@ class CGUnionConversionStruct(CGThing):
     def __init__(self, type, descriptorProvider):
         assert not type.nullable()
         assert not type.hasNullableType
 
         CGThing.__init__(self)
         self.type = type
         self.descriptorProvider = descriptorProvider
 
+    def membersNeedTracing(self):
+        for t in self.type.flatMemberTypes:
+            if type_needs_tracing(t):
+                return True
+        return False
+
     def from_jsval(self):
         memberTypes = self.type.flatMemberTypes
         names = []
         conversions = []
 
         def get_name(memberType):
             if self.type.isGeckoInterface():
                 return memberType.inner.identifier.name
@@ -4305,23 +4325,30 @@ class CGUnionConversionStruct(CGThing):
                 CGGeneric("type Config = ();"),
                 method,
             ], "\n")),
             pre="impl FromJSValConvertible for %s {\n" % self.type,
             post="\n}")
 
     def try_method(self, t):
         templateVars = getUnionTypeTemplateVars(t, self.descriptorProvider)
-        returnType = "Result<Option<%s>, ()>" % templateVars["typeName"]
+        actualType = templateVars["typeName"]
+        if type_needs_tracing(t):
+            actualType = "RootedTraceableBox<%s>" % actualType
+        returnType = "Result<Option<%s>, ()>" % actualType
         jsConversion = templateVars["jsConversion"]
 
+        # Any code to convert to Object is unused, since we're already converting
+        # from an Object value.
+        if t.name == 'Object':
+            return CGGeneric('')
+
         return CGWrapper(
             CGIndenter(jsConversion, 4),
-            # TryConvertToObject is unused, but not generating it while generating others is tricky.
-            pre="#[allow(dead_code)] unsafe fn TryConvertTo%s(cx: *mut JSContext, value: HandleValue) -> %s {\n"
+            pre="unsafe fn TryConvertTo%s(cx: *mut JSContext, value: HandleValue) -> %s {\n"
                 % (t.name, returnType),
             post="\n}")
 
     def define(self):
         from_jsval = self.from_jsval()
         methods = CGIndenter(CGList([
             self.try_method(t) for t in self.type.flatMemberTypes
         ], "\n\n"))
@@ -5677,16 +5704,17 @@ def generate_imports(config, cgthings, d
         'dom::bindings::interface::define_guarded_properties',
         'dom::bindings::interface::html_constructor',
         'dom::bindings::interface::is_exposed_in',
         'dom::bindings::interface::pop_current_element_queue',
         'dom::bindings::interface::push_new_element_queue',
         'dom::bindings::iterable::Iterable',
         'dom::bindings::iterable::IteratorType',
         'dom::bindings::js::JS',
+        'dom::bindings::js::OptionalHeapSetter',
         'dom::bindings::js::Root',
         'dom::bindings::js::RootedReference',
         'dom::bindings::namespace::NamespaceObjectClass',
         'dom::bindings::namespace::create_namespace_object',
         'dom::bindings::reflector::MutDomObject',
         'dom::bindings::reflector::DomObject',
         'dom::bindings::utils::AsVoidPtr',
         'dom::bindings::utils::DOMClass',
@@ -6013,45 +6041,58 @@ class CGDictionary(CGThing):
             inheritance = "    pub parent: %s::%s,\n" % (self.makeModuleName(d.parent),
                                                          self.makeClassName(d.parent))
         else:
             inheritance = ""
         memberDecls = ["    pub %s: %s," %
                        (self.makeMemberName(m[0].identifier.name), self.getMemberType(m))
                        for m in self.memberInfo]
 
+        derive = ["JSTraceable"]
+        mustRoot = ""
+        if self.membersNeedTracing():
+            mustRoot = "#[must_root]\n"
+            derive += ["Default"]
+
         return (string.Template(
-                "#[derive(JSTraceable)]\n"
+                "#[derive(${derive})]\n"
+                "${mustRoot}" +
                 "pub struct ${selfName} {\n" +
                 "${inheritance}" +
                 "\n".join(memberDecls) + "\n" +
                 "}").substitute({"selfName": self.makeClassName(d),
-                                 "inheritance": inheritance}))
+                                 "inheritance": inheritance,
+                                 "mustRoot": mustRoot,
+                                 "derive": ', '.join(derive)}))
 
     def impl(self):
         d = self.dictionary
         if d.parent:
-            initParent = ("parent: {\n"
+            initParent = ("{\n"
                           "    match try!(%s::%s::new(cx, val)) {\n"
                           "        ConversionResult::Success(v) => v,\n"
                           "        ConversionResult::Failure(error) => {\n"
                           "            throw_type_error(cx, &error);\n"
                           "            return Err(());\n"
                           "        }\n"
                           "    }\n"
-                          "},\n" % (self.makeModuleName(d.parent),
-                                    self.makeClassName(d.parent)))
+                          "}" % (self.makeModuleName(d.parent),
+                                 self.makeClassName(d.parent)))
         else:
             initParent = ""
 
-        def memberInit(memberInfo):
+        def memberInit(memberInfo, isInitial):
             member, _ = memberInfo
             name = self.makeMemberName(member.identifier.name)
             conversion = self.getMemberConversion(memberInfo, member.type)
-            return CGGeneric("%s: %s,\n" % (name, conversion.define()))
+            if isInitial:
+                return CGGeneric("%s: %s,\n" % (name, conversion.define()))
+            if member.type.isAny() or member.type.isObject():
+                return CGGeneric("dictionary.%s.set(%s);\n" % (name, conversion.define()))
+            return CGGeneric("dictionary.%s = %s;\n" % (name, conversion.define()))
 
         def varInsert(varName, dictionaryName):
             insertion = ("rooted!(in(cx) let mut %s_js = UndefinedValue());\n"
                          "%s.to_jsval(cx, %s_js.handle_mut());\n"
                          "set_dictionary_property(cx, obj.handle(), \"%s\", %s_js.handle()).unwrap();"
                          % (varName, varName, varName, dictionaryName, varName))
             return CGGeneric(insertion)
 
@@ -6061,66 +6102,89 @@ class CGDictionary(CGThing):
             if member.optional and not member.defaultValue:
                 insertion = CGIfWrapper("let Some(ref %s) = self.%s" % (name, name),
                                         varInsert(name, member.identifier.name))
             else:
                 insertion = CGGeneric("let %s = &self.%s;\n%s" %
                                       (name, name, varInsert(name, member.identifier.name).define()))
             return CGGeneric("%s\n" % insertion.define())
 
-        memberInits = CGList([memberInit(m) for m in self.memberInfo])
         memberInserts = CGList([memberInsert(m) for m in self.memberInfo])
 
+        selfName = self.makeClassName(d)
+        if self.membersNeedTracing():
+            actualType = "RootedTraceableBox<%s>" % selfName
+            preInitial = "let mut dictionary = RootedTraceableBox::new(%s::default());\n" % selfName
+            initParent = initParent = ("dictionary.parent = %s;\n" % initParent) if initParent else ""
+            memberInits = CGList([memberInit(m, False) for m in self.memberInfo])
+            postInitial = ""
+        else:
+            actualType = selfName
+            preInitial = "let dictionary = %s {\n" % selfName
+            postInitial = "};\n"
+            initParent = ("parent: %s,\n" % initParent) if initParent else ""
+            memberInits = CGList([memberInit(m, True) for m in self.memberInfo])
+
         return string.Template(
             "impl ${selfName} {\n"
-            "    pub unsafe fn empty(cx: *mut JSContext) -> ${selfName} {\n"
+            "    pub unsafe fn empty(cx: *mut JSContext) -> ${actualType} {\n"
             "        match ${selfName}::new(cx, HandleValue::null()) {\n"
             "            Ok(ConversionResult::Success(v)) => v,\n"
             "            _ => unreachable!(),\n"
             "        }\n"
             "    }\n"
             "    pub unsafe fn new(cx: *mut JSContext, val: HandleValue) \n"
-            "                      -> Result<ConversionResult<${selfName}>, ()> {\n"
+            "                      -> Result<ConversionResult<${actualType}>, ()> {\n"
             "        let object = if val.get().is_null_or_undefined() {\n"
             "            ptr::null_mut()\n"
             "        } else if val.get().is_object() {\n"
             "            val.get().to_object()\n"
             "        } else {\n"
             "            throw_type_error(cx, \"Value not an object.\");\n"
             "            return Err(());\n"
             "        };\n"
             "        rooted!(in(cx) let object = object);\n"
-            "        Ok(ConversionResult::Success(${selfName} {\n"
+            "${preInitial}"
             "${initParent}"
             "${initMembers}"
-            "        }))\n"
+            "${postInitial}"
+            "        Ok(ConversionResult::Success(dictionary))\n"
             "    }\n"
             "}\n"
             "\n"
-            "impl FromJSValConvertible for ${selfName} {\n"
+            "impl FromJSValConvertible for ${actualType} {\n"
             "    type Config = ();\n"
             "    unsafe fn from_jsval(cx: *mut JSContext, value: HandleValue, _option: ())\n"
-            "                         -> Result<ConversionResult<${selfName}>, ()> {\n"
+            "                         -> Result<ConversionResult<${actualType}>, ()> {\n"
             "        ${selfName}::new(cx, value)\n"
             "    }\n"
             "}\n"
             "\n"
             "impl ToJSValConvertible for ${selfName} {\n"
             "    unsafe fn to_jsval(&self, cx: *mut JSContext, rval: MutableHandleValue) {\n"
             "        rooted!(in(cx) let obj = JS_NewObject(cx, ptr::null()));\n"
             "${insertMembers}"
             "        rval.set(ObjectOrNullValue(obj.get()))\n"
             "    }\n"
             "}\n").substitute({
-                "selfName": self.makeClassName(d),
+                "selfName": selfName,
+                "actualType": actualType,
                 "initParent": CGIndenter(CGGeneric(initParent), indentLevel=12).define(),
                 "initMembers": CGIndenter(memberInits, indentLevel=12).define(),
                 "insertMembers": CGIndenter(memberInserts, indentLevel=8).define(),
+                "preInitial": CGIndenter(CGGeneric(preInitial), indentLevel=12).define(),
+                "postInitial": CGIndenter(CGGeneric(postInitial), indentLevel=12).define(),
             })
 
+    def membersNeedTracing(self):
+        for member, _ in self.memberInfo:
+            if type_needs_tracing(member.type):
+                return True
+        return False
+
     @staticmethod
     def makeDictionaryName(dictionary):
         return dictionary.identifier.name
 
     def makeClassName(self, dictionary):
         return self.makeDictionaryName(dictionary)
 
     @staticmethod
@@ -6633,17 +6697,17 @@ class CallbackMember(CGNativeMember):
             "convertArgs": self.getArgConversions(),
             "doCall": self.getCall(),
             "setupCall": self.getCallSetup(),
         }
         if self.argCount > 0:
             replacements["argCount"] = self.argCountStr
             replacements["argvDecl"] = string.Template(
                 "rooted_vec!(let mut argv);\n"
-                "argv.extend((0..${argCount}).map(|_| Heap::new(UndefinedValue())));\n"
+                "argv.extend((0..${argCount}).map(|_| Heap::default()));\n"
             ).substitute(replacements)
         else:
             # Avoid weird 0-sized arrays
             replacements["argvDecl"] = ""
 
         # Newlines and semicolons are in the values
         pre = string.Template(
             "${setupCall}"
@@ -6708,33 +6772,37 @@ class CallbackMember(CGNativeMember):
             jsvalIndex = "%d + idx" % i
         else:
             jsvalIndex = "%d" % i
             if arg.optional and not arg.defaultValue:
                 argval += ".clone().unwrap()"
 
         conversion = wrapForType(
             "argv_root.handle_mut()", result=argval,
-            successCode="argv[%s] = Heap::new(argv_root.get());" % jsvalIndex,
+            successCode=("{\n" +
+                         "let arg = &mut argv[%s];\n" +
+                         "*arg = Heap::default();\n" +
+                         "arg.set(argv_root.get());\n" +
+                         "}") % jsvalIndex,
             pre="rooted!(in(cx) let mut argv_root = UndefinedValue());")
         if arg.variadic:
             conversion = string.Template(
                 "for idx in 0..${arg}.len() {\n" +
                 CGIndenter(CGGeneric(conversion)).define() + "\n"
                 "}"
             ).substitute({"arg": arg.identifier.name})
         elif arg.optional and not arg.defaultValue:
             conversion = (
                 CGIfWrapper("%s.is_some()" % arg.identifier.name,
                             CGGeneric(conversion)).define() +
                 " else if argc == %d {\n"
                 "    // This is our current trailing argument; reduce argc\n"
                 "    argc -= 1;\n"
                 "} else {\n"
-                "    argv[%d] = Heap::new(UndefinedValue());\n"
+                "    argv[%d] = Heap::default();\n"
                 "}" % (i + 1, i))
         return conversion
 
     def getArgs(self, returnType, argList):
         args = CGNativeMember.getArgs(self, returnType, argList)
         if not self.needThisHandling:
             # Since we don't need this handling, we're the actual method that
             # will be called, so we need an aRethrowExceptions argument.
--- a/servo/components/script/dom/bindings/conversions.rs
+++ b/servo/components/script/dom/bindings/conversions.rs
@@ -111,29 +111,21 @@ impl <T: DomObject + IDLInterface> FromJ
                          -> Result<ConversionResult<Root<T>>, ()> {
         Ok(match root_from_handlevalue(value) {
             Ok(result) => ConversionResult::Success(result),
             Err(()) => ConversionResult::Failure("value is not an object".into()),
         })
     }
 }
 
-impl <T: FromJSValConvertible + JSTraceable> FromJSValConvertible for RootedTraceableBox<T> {
-    type Config = T::Config;
-
-    unsafe fn from_jsval(cx: *mut JSContext,
-                         value: HandleValue,
-                         config: Self::Config)
-                         -> Result<ConversionResult<Self>, ()> {
-        T::from_jsval(cx, value, config).map(|result| {
-            match result {
-                ConversionResult::Success(v) => ConversionResult::Success(RootedTraceableBox::new(v)),
-                ConversionResult::Failure(e) => ConversionResult::Failure(e),
-            }
-        })
+impl<T: ToJSValConvertible + JSTraceable> ToJSValConvertible for RootedTraceableBox<T> {
+    #[inline]
+    unsafe fn to_jsval(&self, cx: *mut JSContext, rval: MutableHandleValue) {
+        let value = &**self;
+        value.to_jsval(cx, rval);
     }
 }
 
 /// Convert `id` to a `DOMString`. Returns `None` if `id` is not a string or
 /// integer.
 ///
 /// Handling of invalid UTF-16 in strings depends on the relevant option.
 pub unsafe fn jsid_to_string(cx: *mut JSContext, id: HandleId) -> Option<DOMString> {
--- a/servo/components/script/dom/bindings/iterable.rs
+++ b/servo/components/script/dom/bindings/iterable.rs
@@ -127,16 +127,19 @@ fn dict_return(cx: *mut JSContext,
 }
 
 fn key_and_value_return(cx: *mut JSContext,
                         result: MutableHandleObject,
                         key: HandleValue,
                         value: HandleValue) -> Fallible<()> {
     let mut dict = unsafe { IterableKeyAndValueResult::empty(cx) };
     dict.done = false;
-    dict.value = Some(vec![Heap::new(key.get()), Heap::new(value.get())]);
+    let values = vec![Heap::default(), Heap::default()];
+    values[0].set(key.get());
+    values[1].set(value.get());
+    dict.value = Some(values);
     rooted!(in(cx) let mut dict_value = UndefinedValue());
     unsafe {
         dict.to_jsval(cx, dict_value.handle_mut());
     }
     result.set(dict_value.to_object());
     Ok(())
 }
--- a/servo/components/script/dom/bindings/js.rs
+++ b/servo/components/script/dom/bindings/js.rs
@@ -26,17 +26,18 @@
 use core::nonzero::NonZero;
 use dom::bindings::conversions::DerivedFrom;
 use dom::bindings::inheritance::Castable;
 use dom::bindings::reflector::{DomObject, Reflector};
 use dom::bindings::trace::JSTraceable;
 use dom::bindings::trace::trace_reflector;
 use dom::node::Node;
 use heapsize::HeapSizeOf;
-use js::jsapi::{JSObject, JSTracer};
+use js::jsapi::{JSObject, JSTracer, Heap};
+use js::rust::GCMethods;
 use mitochondria::OnceCell;
 use script_layout_interface::TrustedNodeAddress;
 use script_thread::STACK_ROOTS;
 use std::cell::UnsafeCell;
 use std::default::Default;
 use std::hash::{Hash, Hasher};
 #[cfg(debug_assertions)]
 use std::intrinsics::type_name;
@@ -649,8 +650,34 @@ impl<T: DomObject> Drop for Root<T> {
     }
 }
 
 unsafe impl<T: DomObject> JSTraceable for Root<T> {
     unsafe fn trace(&self, _: *mut JSTracer) {
         // Already traced.
     }
 }
+
+/// Helper trait for safer manipulations of Option<Heap<T>> values.
+pub trait OptionalHeapSetter {
+    type Value;
+    /// Update this optional heap value with a new value.
+    fn set(&mut self, v: Option<Self::Value>);
+}
+
+impl<T: GCMethods + Copy> OptionalHeapSetter for Option<Heap<T>> where Heap<T>: Default {
+    type Value = T;
+    fn set(&mut self, v: Option<T>) {
+        let v = match v {
+            None => {
+                *self = None;
+                return;
+            }
+            Some(v) => v,
+        };
+
+        if self.is_none() {
+            *self = Some(Heap::default());
+        }
+
+        self.as_ref().unwrap().set(v);
+    }
+}
--- a/servo/components/script/dom/bindings/num.rs
+++ b/servo/components/script/dom/bindings/num.rs
@@ -1,16 +1,17 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 //! The `Finite<T>` struct.
 
 use heapsize::HeapSizeOf;
 use num_traits::Float;
+use std::default::Default;
 use std::ops::Deref;
 
 /// Encapsulates the IDL restricted float type.
 #[derive(Clone, Copy, Eq, JSTraceable, PartialEq)]
 pub struct Finite<T: Float>(T);
 
 impl<T: Float> Finite<T> {
     /// Create a new `Finite<T: Float>` safely.
@@ -40,8 +41,14 @@ impl<T: Float> Deref for Finite<T> {
     }
 }
 
 impl<T: Float + HeapSizeOf> HeapSizeOf for Finite<T> {
     fn heap_size_of_children(&self) -> usize {
         (**self).heap_size_of_children()
     }
 }
+
+impl<T: Float + Default> Default for Finite<T> {
+    fn default() -> Finite<T> {
+        Finite::wrap(T::default())
+    }
+}
--- a/servo/components/script/dom/bindings/str.rs
+++ b/servo/components/script/dom/bindings/str.rs
@@ -4,26 +4,27 @@
 
 //! The `ByteString` struct.
 
 use cssparser::CowRcStr;
 use html5ever::{LocalName, Namespace};
 use servo_atoms::Atom;
 use std::ascii::AsciiExt;
 use std::borrow::{Borrow, Cow, ToOwned};
+use std::default::Default;
 use std::fmt;
 use std::hash::{Hash, Hasher};
 use std::marker::PhantomData;
 use std::ops;
 use std::ops::{Deref, DerefMut};
 use std::str;
 use std::str::{Bytes, FromStr};
 
 /// Encapsulates the IDL `ByteString` type.
-#[derive(Clone, Debug, Eq, HeapSizeOf, JSTraceable, PartialEq)]
+#[derive(Clone, Debug, Default, Eq, HeapSizeOf, JSTraceable, PartialEq)]
 pub struct ByteString(Vec<u8>);
 
 impl ByteString {
     /// Creates a new `ByteString`.
     pub fn new(value: Vec<u8>) -> ByteString {
         ByteString(value)
     }
 
@@ -72,17 +73,17 @@ impl ops::Deref for ByteString {
     type Target = [u8];
     fn deref(&self) -> &[u8] {
         &self.0
     }
 }
 
 /// A string that is constructed from a UCS-2 buffer by replacing invalid code
 /// points with the replacement character.
-#[derive(Clone, HeapSizeOf)]
+#[derive(Clone, Default, HeapSizeOf)]
 pub struct USVString(pub String);
 
 
 /// Returns whether `s` is a `token`, as defined by
 /// [RFC 2616](http://tools.ietf.org/html/rfc2616#page-17).
 pub fn is_token(s: &[u8]) -> bool {
     if s.is_empty() {
         return false; // A token must be at least a single character
--- a/servo/components/script/dom/bindings/trace.rs
+++ b/servo/components/script/dom/bindings/trace.rs
@@ -740,16 +740,17 @@ impl<'a, T: JSTraceable + 'static> Drop 
 }
 
 /// Roots any JSTraceable thing
 ///
 /// If you have a valid DomObject, use Root.
 /// If you have GC things like *mut JSObject or JSVal, use rooted!.
 /// If you have an arbitrary number of DomObjects to root, use rooted_vec!.
 /// If you know what you're doing, use this.
+#[allow_unrooted_interior]
 pub struct RootedTraceableBox<T: 'static + JSTraceable> {
     ptr: *mut T,
 }
 
 unsafe impl<T: JSTraceable + 'static> JSTraceable for RootedTraceableBox<T> {
     unsafe fn trace(&self, tracer: *mut JSTracer) {
         (*self.ptr).trace(tracer);
     }
@@ -763,16 +764,22 @@ impl<T: JSTraceable + 'static> RootedTra
             RootedTraceableSet::add(traceable);
         }
         RootedTraceableBox {
             ptr: traceable,
         }
     }
 }
 
+impl<T: JSTraceable + Default> Default for RootedTraceableBox<T> {
+    fn default() -> RootedTraceableBox<T> {
+        RootedTraceableBox::new(T::default())
+    }
+}
+
 impl<T: JSTraceable> Deref for RootedTraceableBox<T> {
     type Target = T;
     fn deref(&self) -> &T {
         unsafe {
             &*self.ptr
         }
     }
 }
--- a/servo/components/script/dom/event.rs
+++ b/servo/components/script/dom/event.rs
@@ -517,8 +517,17 @@ fn inner_invoke(window: Option<&Window>,
         }
 
         // TODO: step 2.7.
     }
 
     // Step 3.
     found
 }
+
+impl Default for EventBinding::EventInit {
+    fn default() -> EventBinding::EventInit {
+        EventBinding::EventInit {
+            bubbles: false,
+            cancelable: false,
+        }
+    }
+}
--- a/servo/components/script/dom/extendableevent.rs
+++ b/servo/components/script/dom/extendableevent.rs
@@ -1,13 +1,13 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use dom::bindings::codegen::Bindings::EventBinding::EventMethods;
+use dom::bindings::codegen::Bindings::EventBinding::{self, EventMethods};
 use dom::bindings::codegen::Bindings::ExtendableEventBinding;
 use dom::bindings::error::{Error, ErrorResult, Fallible};
 use dom::bindings::inheritance::Castable;
 use dom::bindings::js::Root;
 use dom::bindings::reflector::reflect_dom_object;
 use dom::bindings::str::DOMString;
 use dom::event::Event;
 use dom::serviceworkerglobalscope::ServiceWorkerGlobalScope;
@@ -62,8 +62,16 @@ impl ExtendableEvent {
         Ok(())
     }
 
     // https://dom.spec.whatwg.org/#dom-event-istrusted
     pub fn IsTrusted(&self) -> bool {
         self.event.IsTrusted()
     }
 }
+
+impl Default for ExtendableEventBinding::ExtendableEventInit {
+    fn default() -> ExtendableEventBinding::ExtendableEventInit {
+        ExtendableEventBinding::ExtendableEventInit {
+            parent: EventBinding::EventInit::default(),
+        }
+    }
+}
--- a/servo/components/script/dom/filereader.rs
+++ b/servo/components/script/dom/filereader.rs
@@ -8,16 +8,17 @@ use dom::bindings::codegen::Bindings::Bl
 use dom::bindings::codegen::Bindings::FileReaderBinding::{self, FileReaderConstants, FileReaderMethods};
 use dom::bindings::codegen::UnionTypes::StringOrObject;
 use dom::bindings::error::{Error, ErrorResult, Fallible};
 use dom::bindings::inheritance::Castable;
 use dom::bindings::js::{MutNullableJS, Root};
 use dom::bindings::refcounted::Trusted;
 use dom::bindings::reflector::{DomObject, reflect_dom_object};
 use dom::bindings::str::DOMString;
+use dom::bindings::trace::RootedTraceableBox;
 use dom::blob::Blob;
 use dom::domexception::{DOMErrorName, DOMException};
 use dom::event::{Event, EventBubbles, EventCancelable};
 use dom::eventtarget::EventTarget;
 use dom::globalscope::GlobalScope;
 use dom::progressevent::ProgressEvent;
 use dom_struct::dom_struct;
 use encoding::all::UTF_8;
@@ -333,17 +334,19 @@ impl FileReaderMethods for FileReader {
 
     #[allow(unsafe_code)]
     // https://w3c.github.io/FileAPI/#dfn-result
     unsafe fn GetResult(&self, _: *mut JSContext) -> Option<StringOrObject> {
         self.result.borrow().as_ref().map(|r| match *r {
             FileReaderResult::String(ref string) =>
                 StringOrObject::String(string.clone()),
             FileReaderResult::ArrayBuffer(ref arr_buffer) => {
-                StringOrObject::Object(Heap::new((*arr_buffer.ptr.get()).to_object()))
+                let result = RootedTraceableBox::new(Heap::default());
+                result.set((*arr_buffer.ptr.get()).to_object());
+                StringOrObject::Object(result)
             }
         })
     }
 
     // https://w3c.github.io/FileAPI/#dfn-readyState
     fn ReadyState(&self) -> u16 {
         self.ready_state.get() as u16
     }
--- a/servo/components/script/dom/testbinding.rs
+++ b/servo/components/script/dom/testbinding.rs
@@ -333,40 +333,40 @@ impl TestBindingMethods for TestBinding 
     }
     fn ReceiveNullableUnion5(&self) -> Option<UnsignedLongOrBoolean> {
         Some(UnsignedLongOrBoolean::UnsignedLong(0u32))
     }
     fn ReceiveNullableUnion6(&self) -> Option<ByteStringOrLong> {
         Some(ByteStringOrLong::ByteString(ByteString::new(vec!())))
     }
     fn ReceiveNullableSequence(&self) -> Option<Vec<i32>> { Some(vec![1]) }
-    fn ReceiveTestDictionaryWithSuccessOnKeyword(&self) -> TestDictionary {
-        TestDictionary {
-            anyValue: Heap::new(NullValue()),
+    fn ReceiveTestDictionaryWithSuccessOnKeyword(&self) -> RootedTraceableBox<TestDictionary> {
+        RootedTraceableBox::new(TestDictionary {
+            anyValue: Heap::default(),
             booleanValue: None,
             byteValue: None,
-            dict: TestDictionaryDefaults {
+            dict: RootedTraceableBox::new(TestDictionaryDefaults {
                 UnrestrictedDoubleValue: 0.0,
-                anyValue: Heap::new(NullValue()),
+                anyValue: Heap::default(),
                 booleanValue: false,
                 bytestringValue: ByteString::new(vec![]),
                 byteValue: 0,
                 doubleValue: Finite::new(1.0).unwrap(),
                 enumValue: TestEnum::Foo,
                 floatValue: Finite::new(1.0).unwrap(),
                 longLongValue: 54,
                 longValue: 12,
                 nullableBooleanValue: None,
                 nullableBytestringValue: None,
                 nullableByteValue: None,
                 nullableDoubleValue: None,
                 nullableFloatValue: None,
                 nullableLongLongValue: None,
                 nullableLongValue: None,
-                nullableObjectValue: Heap::new(ptr::null_mut()),
+                nullableObjectValue: Heap::default(),
                 nullableOctetValue: None,
                 nullableShortValue: None,
                 nullableStringValue: None,
                 nullableUnrestrictedDoubleValue: None,
                 nullableUnrestrictedFloatValue: None,
                 nullableUnsignedLongLongValue: None,
                 nullableUnsignedLongValue: None,
                 nullableUnsignedShortValue: None,
@@ -374,17 +374,17 @@ impl TestBindingMethods for TestBinding 
                 octetValue: 0,
                 shortValue: 0,
                 stringValue: DOMString::new(),
                 unrestrictedFloatValue: 0.0,
                 unsignedLongLongValue: 0,
                 unsignedLongValue: 0,
                 unsignedShortValue: 0,
                 usvstringValue: USVString("".to_owned()),
-            },
+            }),
             doubleValue: None,
             enumValue: None,
             floatValue: None,
             interfaceValue: None,
             longLongValue: None,
             longValue: None,
             objectValue: None,
             octetValue: None,
@@ -396,17 +396,17 @@ impl TestBindingMethods for TestBinding 
             unrestrictedDoubleValue: None,
             unrestrictedFloatValue: None,
             unsignedLongLongValue: None,
             unsignedLongValue: None,
             unsignedShortValue: None,
             usvstringValue: None,
             nonRequiredNullable: None,
             nonRequiredNullable2: Some(None), // null
-        }
+        })
     }
 
     fn DictMatchesPassedValues(&self, arg: RootedTraceableBox<TestDictionary>) -> bool {
         arg.type_.as_ref().map(|s| s == "success").unwrap_or(false) &&
             arg.nonRequiredNullable.is_none() &&
             arg.nonRequiredNullable2 == Some(None)
     }
 
@@ -431,19 +431,19 @@ impl TestBindingMethods for TestBinding 
     fn PassUnion(&self, _: HTMLElementOrLong) {}
     fn PassUnion2(&self, _: EventOrString) {}
     fn PassUnion3(&self, _: BlobOrString) {}
     fn PassUnion4(&self, _: StringOrStringSequence) {}
     fn PassUnion5(&self, _: StringOrBoolean) {}
     fn PassUnion6(&self, _: UnsignedLongOrBoolean) {}
     fn PassUnion7(&self, _: StringSequenceOrUnsignedLong) {}
     fn PassUnion8(&self, _: ByteStringSequenceOrLong) {}
-    fn PassUnion9(&self, _: RootedTraceableBox<UnionTypes::TestDictionaryOrLong>) {}
+    fn PassUnion9(&self, _: UnionTypes::TestDictionaryOrLong) {}
     #[allow(unsafe_code)]
-    unsafe fn PassUnion10(&self, _: *mut JSContext, _: RootedTraceableBox<UnionTypes::StringOrObject>) {}
+    unsafe fn PassUnion10(&self, _: *mut JSContext, _: UnionTypes::StringOrObject) {}
     fn PassUnionWithTypedef(&self, _: DocumentOrTestTypedef) {}
     fn PassUnionWithTypedef2(&self, _: LongSequenceOrTestTypedef) {}
     #[allow(unsafe_code)]
     unsafe fn PassAny(&self, _: *mut JSContext, _: HandleValue) {}
     #[allow(unsafe_code)]
     unsafe fn PassObject(&self, _: *mut JSContext, _: *mut JSObject) {}
     fn PassCallbackFunction(&self, _: Rc<Function>) {}
     fn PassCallbackInterface(&self, _: Rc<EventListener>) {}