Auto merge of #19864 - nox:callbacks, r=Manishearth

Make some callback-related code unsafe

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

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

@@ -4417,7 +4417,7 @@ class ClassMethod(ClassItem):
     def __init__(self, name, returnType, args, inline=False, static=False,
                  virtual=False, const=False, bodyInHeader=False,
                  templateArgs=None, visibility='public', body=None,
-                 breakAfterReturnDecl="\n",
+                 breakAfterReturnDecl="\n", unsafe=False,
                  breakAfterSelf="\n", override=False):
         """
         override indicates whether to flag the method as MOZ_OVERRIDE
@@ -4436,6 +4436,7 @@ class ClassMethod(ClassItem):
         self.breakAfterReturnDecl = breakAfterReturnDecl
         self.breakAfterSelf = breakAfterSelf
         self.override = override
+        self.unsafe = unsafe
         ClassItem.__init__(self, name, visibility)
 
     def getDecorators(self, declaring):
@@ -4468,7 +4469,7 @@ class ClassMethod(ClassItem):
 
         return string.Template(
             "${decorators}%s"
-            "${visibility}fn ${name}${templateClause}(${args})${returnType}${const}${override}${body}%s" %
+            "${visibility}${unsafe}fn ${name}${templateClause}(${args})${returnType}${const}${override}${body}%s" %
             (self.breakAfterReturnDecl, self.breakAfterSelf)
         ).substitute({
             'templateClause': templateClause,
@@ -4479,7 +4480,8 @@ class ClassMethod(ClassItem):
             'override': ' MOZ_OVERRIDE' if self.override else '',
             'args': args,
             'body': body,
-            'visibility': self.visibility + ' ' if self.visibility != 'priv' else ''
+            'visibility': self.visibility + ' ' if self.visibility != 'priv' else '',
+            'unsafe': "unsafe " if self.unsafe else "",
         })
 
     def define(self, cgClass):
@@ -4549,7 +4551,7 @@ class ClassConstructor(ClassItem):
                 "});\n"
                 "// Note: callback cannot be moved after calling init.\n"
                 "match Rc::get_mut(&mut ret) {\n"
-                "    Some(ref mut callback) => unsafe { callback.parent.init(%s, %s) },\n"
+                "    Some(ref mut callback) => callback.parent.init(%s, %s),\n"
                 "    None => unreachable!(),\n"
                 "};\n"
                 "ret") % (cgClass.name, '\n'.join(initializers),
@@ -4564,7 +4566,7 @@ class ClassConstructor(ClassItem):
         body = ' {\n' + body + '}'
 
         return string.Template("""\
-pub fn ${decorators}new(${args}) -> Rc<${className}>${body}
+pub unsafe fn ${decorators}new(${args}) -> Rc<${className}>${body}
 """).substitute({'decorators': self.getDecorators(True),
                  'className': cgClass.getNameString(),
                  'args': args,
@@ -6495,7 +6497,8 @@ def return_type(descriptorProvider, rettype, infallible):
 
 class CGNativeMember(ClassMethod):
     def __init__(self, descriptorProvider, member, name, signature, extendedAttrs,
-                 breakAfter=True, passJSBitsAsNeeded=True, visibility="public"):
+                 breakAfter=True, passJSBitsAsNeeded=True, visibility="public",
+                 unsafe=False):
         """
         If passJSBitsAsNeeded is false, we don't automatically pass in a
         JSContext* or a JSObject* based on the return and argument types.
@@ -6514,6 +6517,7 @@ class CGNativeMember(ClassMethod):
                              const=(not member.isStatic() and member.isAttr() and
                                     not signature[0].isVoid()),
                              breakAfterSelf=breakAfterSelf,
+                             unsafe=unsafe,
                              visibility=visibility)
 
     def getReturnType(self, type):
@@ -6529,8 +6533,7 @@ class CGNativeMember(ClassMethod):
 
 
 class CGCallback(CGClass):
-    def __init__(self, idlObject, descriptorProvider, baseName, methods,
+    def __init__(self, idlObject, descriptorProvider, baseName, methods):
-                 getters=[], setters=[]):
         self.baseName = baseName
         self._deps = idlObject.getDeps()
         name = idlObject.identifier.name
@@ -6549,7 +6552,7 @@ class CGCallback(CGClass):
         CGClass.__init__(self, name,
                          bases=[ClassBase(baseName)],
                          constructors=self.getConstructors(),
-                         methods=realMethods + getters + setters,
+                         methods=realMethods,
                          decorators="#[derive(JSTraceable, PartialEq)]\n#[allow_unrooted_interior]")
 
     def getConstructors(self):
@@ -6599,14 +6602,14 @@ class CGCallback(CGClass):
             "if thisObjJS.is_null() {\n"
             "    return Err(JSFailed);\n"
             "}\n"
-            "return ${methodName}(${callArgs});").substitute({
+            "unsafe { ${methodName}(${callArgs}) }").substitute({
                 "callArgs": ", ".join(argnamesWithThis),
                 "methodName": 'self.' + method.name,
             })
         bodyWithoutThis = string.Template(
             setupCall +
             "rooted!(in(s.get_context()) let thisObjJS = ptr::null_mut::<JSObject>());\n"
-            "return ${methodName}(${callArgs});").substitute({
+            "unsafe { ${methodName}(${callArgs}) }").substitute({
                 "callArgs": ", ".join(argnamesWithoutThis),
                 "methodName": 'self.' + method.name,
             })
@@ -6672,16 +6675,13 @@ class CGCallbackInterface(CGCallback):
     def __init__(self, descriptor):
         iface = descriptor.interface
         attrs = [m for m in iface.members if m.isAttr() and not m.isStatic()]
-        getters = [CallbackGetter(a, descriptor) for a in attrs]
+        assert not attrs
-        setters = [CallbackSetter(a, descriptor) for a in attrs
-                   if not a.readonly]
         methods = [m for m in iface.members
                    if m.isMethod() and not m.isStatic() and not m.isIdentifierLess()]
         methods = [CallbackOperation(m, sig, descriptor) for m in methods
                    for sig in m.signatures()]
         assert not iface.isJSImplemented() or not iface.ctor()
-        CGCallback.__init__(self, iface, descriptor, "CallbackInterface",
+        CGCallback.__init__(self, iface, descriptor, "CallbackInterface", methods)
-                            methods, getters=getters, setters=setters)
 
 
 class FakeMember():
@@ -6732,6 +6732,7 @@ class CallbackMember(CGNativeMember):
                                 name, (self.retvalType, args),
                                 extendedAttrs={},
                                 passJSBitsAsNeeded=False,
+                                unsafe=needThisHandling,
                                 visibility=visibility)
         # We have to do all the generation of our body now, because
         # the caller relies on us throwing if we can't manage it.
@@ -6765,10 +6766,7 @@ class CallbackMember(CGNativeMember):
             "${convertArgs}"
             "${doCall}"
             "${returnResult}").substitute(replacements)
-        return CGWrapper(CGIndenter(CGList([
+        return pre + "\n" + body
-            CGGeneric(pre),
-            CGGeneric(body),
-        ], "\n"), 4), pre="unsafe {\n", post="\n}").define()
 
     def getResultConversion(self):
         replacements = {
@@ -6998,59 +6996,6 @@ class CallbackOperation(CallbackOperationBase):
                                        descriptor, descriptor.interface.isSingleOperationInterface())
 
 
-class CallbackGetter(CallbackMember):
-    def __init__(self, attr, descriptor):
-        self.ensureASCIIName(attr)
-        self.attrName = attr.identifier.name
-        CallbackMember.__init__(self,
-                                (attr.type, []),
-                                callbackGetterName(attr),
-                                descriptor,
-                                needThisHandling=False)
-
-    def getRvalDecl(self):
-        return "Dom::Rooted<Dom::Value> rval(cx, JS::UndefinedValue());\n"
-
-    def getCall(self):
-        replacements = {
-            "attrName": self.attrName
-        }
-        return string.Template(
-            'if (!JS_GetProperty(cx, mCallback, "${attrName}", &rval)) {\n'
-            '    return Err(JSFailed);\n'
-            '}\n').substitute(replacements)
-
-
-class CallbackSetter(CallbackMember):
-    def __init__(self, attr, descriptor):
-        self.ensureASCIIName(attr)
-        self.attrName = attr.identifier.name
-        CallbackMember.__init__(self,
-                                (BuiltinTypes[IDLBuiltinType.Types.void],
-                                 [FakeArgument(attr.type, attr)]),
-                                callbackSetterName(attr),
-                                descriptor,
-                                needThisHandling=False)
-
-    def getRvalDecl(self):
-        # We don't need an rval
-        return ""
-
-    def getCall(self):
-        replacements = {
-            "attrName": self.attrName,
-            "argv": "argv.handleAt(0)",
-        }
-        return string.Template(
-            'MOZ_ASSERT(argv.length() == 1);\n'
-            'if (!JS_SetProperty(cx, mCallback, "${attrName}", ${argv})) {\n'
-            '    return Err(JSFailed);\n'
-            '}\n').substitute(replacements)
-
-    def getArgcDecl(self):
-        return None
-
-
 class CGIterableMethodGenerator(CGGeneric):
     """
     Creates methods for iterable interfaces. Unwrapping/wrapping

components/script/dom/customelementregistry.rs

@@ -121,7 +121,8 @@ impl CustomElementRegistry {
 
     /// <https://html.spec.whatwg.org/multipage/#dom-customelementregistry-define>
     /// Steps 10.3, 10.4
-    fn get_callbacks(&self, prototype: HandleObject) -> Fallible<LifecycleCallbacks> {
+    #[allow(unsafe_code)]
+    unsafe fn get_callbacks(&self, prototype: HandleObject) -> Fallible<LifecycleCallbacks> {
         let cx = self.window.get_cx();
 
         // Step 4
@@ -164,20 +165,21 @@ impl CustomElementRegistry {
 /// <https://html.spec.whatwg.org/multipage/#dom-customelementregistry-define>
 /// Step 10.4
 #[allow(unsafe_code)]
-fn get_callback(cx: *mut JSContext, prototype: HandleObject, name: &[u8]) -> Fallible<Option<Rc<Function>>> {
+unsafe fn get_callback(
+    cx: *mut JSContext,
+    prototype: HandleObject,
+    name: &[u8],
+) -> Fallible<Option<Rc<Function>>> {
     rooted!(in(cx) let mut callback = UndefinedValue());
 
     // Step 10.4.1
-    if unsafe { !JS_GetProperty(cx,
+    if !JS_GetProperty(cx, prototype, name.as_ptr() as *const _, callback.handle_mut()) {
-                                prototype,
-                                name.as_ptr() as *const _,
-                                callback.handle_mut()) } {
         return Err(Error::JSFailed);
     }
 
     // Step 10.4.2
     if !callback.is_undefined() {
-        if !callback.is_object() || unsafe { !IsCallable(callback.to_object()) } {
+        if !callback.is_object() || !IsCallable(callback.to_object()) {
             return Err(Error::Type("Lifecycle callback is not callable".to_owned()));
         }
         Ok(Some(Function::new(cx, callback.to_object())))
@@ -265,7 +267,7 @@ impl CustomElementRegistryMethods for CustomElementRegistry {
         rooted!(in(cx) let proto_object = prototype.to_object());
         let callbacks = {
             let _ac = JSAutoCompartment::new(cx, proto_object.get());
-            match self.get_callbacks(proto_object.handle()) {
+            match unsafe { self.get_callbacks(proto_object.handle()) } {
                 Ok(callbacks) => callbacks,
                 Err(error) => {
                     self.element_definition_is_running.set(false);

components/script/dom/eventtarget.rs

@@ -471,50 +471,76 @@ impl EventTarget {
         assert!(!funobj.is_null());
         // Step 1.14
         if is_error {
-            Some(CommonEventHandler::ErrorEventHandler(OnErrorEventHandlerNonNull::new(cx, funobj)))
+            Some(CommonEventHandler::ErrorEventHandler(
+                unsafe { OnErrorEventHandlerNonNull::new(cx, funobj) },
+            ))
         } else {
             if ty == &atom!("beforeunload") {
                 Some(CommonEventHandler::BeforeUnloadEventHandler(
-                        OnBeforeUnloadEventHandlerNonNull::new(cx, funobj)))
+                    unsafe { OnBeforeUnloadEventHandlerNonNull::new(cx, funobj) },
+                ))
             } else {
-                Some(CommonEventHandler::EventHandler(EventHandlerNonNull::new(cx, funobj)))
+                Some(CommonEventHandler::EventHandler(
+                    unsafe { EventHandlerNonNull::new(cx, funobj) },
+                ))
             }
         }
     }
 
+    #[allow(unsafe_code)]
     pub fn set_event_handler_common<T: CallbackContainer>(
-        &self, ty: &str, listener: Option<Rc<T>>)
+        &self,
+        ty: &str,
+        listener: Option<Rc<T>>,
+    )
+    where
+        T: CallbackContainer,
     {
         let cx = self.global().get_cx();
 
-        let event_listener = listener.map(|listener|
+        let event_listener = listener.map(|listener| {
-                                          InlineEventListener::Compiled(
+            InlineEventListener::Compiled(CommonEventHandler::EventHandler(
-                                              CommonEventHandler::EventHandler(
+                unsafe { EventHandlerNonNull::new(cx, listener.callback()) },
-                                                  EventHandlerNonNull::new(cx, listener.callback()))));
+            ))
+        });
         self.set_inline_event_listener(Atom::from(ty), event_listener);
     }
 
+    #[allow(unsafe_code)]
     pub fn set_error_event_handler<T: CallbackContainer>(
-        &self, ty: &str, listener: Option<Rc<T>>)
+        &self,
+        ty: &str,
+        listener: Option<Rc<T>>,
+    )
+    where
+        T: CallbackContainer,
     {
         let cx = self.global().get_cx();
 
-        let event_listener = listener.map(|listener|
+        let event_listener = listener.map(|listener| {
-                                          InlineEventListener::Compiled(
+            InlineEventListener::Compiled(CommonEventHandler::ErrorEventHandler(
-                                              CommonEventHandler::ErrorEventHandler(
+                unsafe { OnErrorEventHandlerNonNull::new(cx, listener.callback()) }
-                                                  OnErrorEventHandlerNonNull::new(cx, listener.callback()))));
+            ))
+        });
         self.set_inline_event_listener(Atom::from(ty), event_listener);
     }
 
-    pub fn set_beforeunload_event_handler<T: CallbackContainer>(&self, ty: &str,
+    #[allow(unsafe_code)]
-                                                                listener: Option<Rc<T>>) {
+    pub fn set_beforeunload_event_handler<T: CallbackContainer>(
+        &self,
+        ty: &str,
+        listener: Option<Rc<T>>,
+    )
+    where
+        T: CallbackContainer,
+    {
         let cx = self.global().get_cx();
 
-        let event_listener = listener.map(|listener|
+        let event_listener = listener.map(|listener| {
-            InlineEventListener::Compiled(
+            InlineEventListener::Compiled(CommonEventHandler::BeforeUnloadEventHandler(
-                CommonEventHandler::BeforeUnloadEventHandler(
+                unsafe { OnBeforeUnloadEventHandlerNonNull::new(cx, listener.callback()) }
-                    OnBeforeUnloadEventHandlerNonNull::new(cx, listener.callback())))
+            ))
-        );
+        });
         self.set_inline_event_listener(Atom::from(ty), event_listener);
     }