From f903da0a7bba2806e3a200eee8414a629d27ffa8 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Thu, 25 Jan 2018 11:25:23 +0100 Subject: [PATCH 1/3] Make callbacks' new methods unsafe They take raw pointers to contexts and objects. --- .../dom/bindings/codegen/CodegenRust.py | 4 +- .../script/dom/customelementregistry.rs | 18 ++--- components/script/dom/eventtarget.rs | 66 +++++++++++++------ 3 files changed, 58 insertions(+), 30 deletions(-) diff --git a/components/script/dom/bindings/codegen/CodegenRust.py b/components/script/dom/bindings/codegen/CodegenRust.py index 92f6b3a71b6..cc9a37a5ecc 100644 --- a/components/script/dom/bindings/codegen/CodegenRust.py +++ b/components/script/dom/bindings/codegen/CodegenRust.py @@ -4549,7 +4549,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 +4564,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, diff --git a/components/script/dom/customelementregistry.rs b/components/script/dom/customelementregistry.rs index 99b38717ac2..a208f99def6 100644 --- a/components/script/dom/customelementregistry.rs +++ b/components/script/dom/customelementregistry.rs @@ -121,7 +121,8 @@ impl CustomElementRegistry { /// /// Steps 10.3, 10.4 - fn get_callbacks(&self, prototype: HandleObject) -> Fallible { + #[allow(unsafe_code)] + unsafe fn get_callbacks(&self, prototype: HandleObject) -> Fallible { let cx = self.window.get_cx(); // Step 4 @@ -164,20 +165,21 @@ impl CustomElementRegistry { /// /// Step 10.4 #[allow(unsafe_code)] -fn get_callback(cx: *mut JSContext, prototype: HandleObject, name: &[u8]) -> Fallible>> { +unsafe fn get_callback( + cx: *mut JSContext, + prototype: HandleObject, + name: &[u8], +) -> Fallible>> { rooted!(in(cx) let mut callback = UndefinedValue()); // Step 10.4.1 - if unsafe { !JS_GetProperty(cx, - prototype, - name.as_ptr() as *const _, - callback.handle_mut()) } { + if !JS_GetProperty(cx, 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); diff --git a/components/script/dom/eventtarget.rs b/components/script/dom/eventtarget.rs index 426752e42d5..7418efce422 100644 --- a/components/script/dom/eventtarget.rs +++ b/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( - &self, ty: &str, listener: Option>) + &self, + ty: &str, + listener: Option>, + ) + where + T: CallbackContainer, { let cx = self.global().get_cx(); - let event_listener = listener.map(|listener| - InlineEventListener::Compiled( - CommonEventHandler::EventHandler( - EventHandlerNonNull::new(cx, listener.callback())))); + let event_listener = listener.map(|listener| { + InlineEventListener::Compiled(CommonEventHandler::EventHandler( + unsafe { EventHandlerNonNull::new(cx, listener.callback()) }, + )) + }); self.set_inline_event_listener(Atom::from(ty), event_listener); } + #[allow(unsafe_code)] pub fn set_error_event_handler( - &self, ty: &str, listener: Option>) + &self, + ty: &str, + listener: Option>, + ) + where + T: CallbackContainer, { let cx = self.global().get_cx(); - let event_listener = listener.map(|listener| - InlineEventListener::Compiled( - CommonEventHandler::ErrorEventHandler( - OnErrorEventHandlerNonNull::new(cx, listener.callback())))); + let event_listener = listener.map(|listener| { + InlineEventListener::Compiled(CommonEventHandler::ErrorEventHandler( + unsafe { OnErrorEventHandlerNonNull::new(cx, listener.callback()) } + )) + }); self.set_inline_event_listener(Atom::from(ty), event_listener); } - pub fn set_beforeunload_event_handler(&self, ty: &str, - listener: Option>) { + #[allow(unsafe_code)] + pub fn set_beforeunload_event_handler( + &self, + ty: &str, + listener: Option>, + ) + where + T: CallbackContainer, + { let cx = self.global().get_cx(); - let event_listener = listener.map(|listener| - InlineEventListener::Compiled( - CommonEventHandler::BeforeUnloadEventHandler( - OnBeforeUnloadEventHandlerNonNull::new(cx, listener.callback()))) - ); + let event_listener = listener.map(|listener| { + InlineEventListener::Compiled(CommonEventHandler::BeforeUnloadEventHandler( + unsafe { OnBeforeUnloadEventHandlerNonNull::new(cx, listener.callback()) } + )) + }); self.set_inline_event_listener(Atom::from(ty), event_listener); } From 74dabbdc62b49862c115eccb38fb9e010041cf28 Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Thu, 25 Jan 2018 11:37:24 +0100 Subject: [PATCH 2/3] Kill dead callback codegen code --- .../dom/bindings/codegen/CodegenRust.py | 65 ++----------------- 1 file changed, 4 insertions(+), 61 deletions(-) diff --git a/components/script/dom/bindings/codegen/CodegenRust.py b/components/script/dom/bindings/codegen/CodegenRust.py index cc9a37a5ecc..00ba2ce2148 100644 --- a/components/script/dom/bindings/codegen/CodegenRust.py +++ b/components/script/dom/bindings/codegen/CodegenRust.py @@ -6529,8 +6529,7 @@ class CGNativeMember(ClassMethod): class CGCallback(CGClass): - def __init__(self, idlObject, descriptorProvider, baseName, methods, - getters=[], setters=[]): + def __init__(self, idlObject, descriptorProvider, baseName, methods): self.baseName = baseName self._deps = idlObject.getDeps() name = idlObject.identifier.name @@ -6549,7 +6548,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): @@ -6672,16 +6671,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] - setters = [CallbackSetter(a, descriptor) for a in attrs - if not a.readonly] + assert not attrs 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", - methods, getters=getters, setters=setters) + CGCallback.__init__(self, iface, descriptor, "CallbackInterface", methods) class FakeMember(): @@ -6998,59 +6994,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 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 From de426baeccfecb5a02e5db876d9a127ee9379c2e Mon Sep 17 00:00:00 2001 From: Anthony Ramine Date: Thu, 25 Jan 2018 12:07:13 +0100 Subject: [PATCH 3/3] Make the private callback methods taking a raw this pointer unsafe --- .../dom/bindings/codegen/CodegenRust.py | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/components/script/dom/bindings/codegen/CodegenRust.py b/components/script/dom/bindings/codegen/CodegenRust.py index 00ba2ce2148..d2d090f8350 100644 --- a/components/script/dom/bindings/codegen/CodegenRust.py +++ b/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): @@ -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): @@ -6598,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::());\n" - "return ${methodName}(${callArgs});").substitute({ + "unsafe { ${methodName}(${callArgs}) }").substitute({ "callArgs": ", ".join(argnamesWithoutThis), "methodName": 'self.' + method.name, }) @@ -6728,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. @@ -6761,10 +6766,7 @@ class CallbackMember(CGNativeMember): "${convertArgs}" "${doCall}" "${returnResult}").substitute(replacements) - return CGWrapper(CGIndenter(CGList([ - CGGeneric(pre), - CGGeneric(body), - ], "\n"), 4), pre="unsafe {\n", post="\n}").define() + return pre + "\n" + body def getResultConversion(self): replacements = {