servo: Merge #19644 - Root sequence<any> params using CustomAutoRooter (from Xanewok:root-seq-any); r=jdm
authorIgor Matuszewski <Xanewok@gmail.com>
Fri, 05 Jan 2018 14:02:53 -0600
changeset 449795 cdc2693ba485669c0fbd4e63de6a394b6cfca8a0
parent 449794 a10c3fef37f5e84dbdabde2475aab10cfdb8fa3c
child 449796 58d3a80ef7f78610a56a1dd3766e208513196cf5
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
servo: Merge #19644 - Root sequence<any> params using CustomAutoRooter (from Xanewok:root-seq-any); r=jdm <!-- Please describe your changes on the following line: --> Attempt at https://github.com/servo/servo/issues/16678. At the moment this pulls an external [remove-handle-conv](https://github.com/Xanewok/rust-mozjs/tree/remove-handle-conv) branch for rust-mozjs (removing `FromJSValConvertible` implementation for `HandleValue` and adding one for `*mut JSObject`) In short, this roots the `sequence<any>` and `sequence<object>` function arguments using `CustomAutoRooter` (introduced in https://github.com/servo/rust-mozjs/pull/382). As per https://github.com/servo/servo/issues/16678#issuecomment-302224110 the underlying vector contains raw gc thing pointers instead of handles. To do that, this introduces new possible `isMember = "AutoRoot"` value in CodegenRust.py. The rooted argument is passed to a function wrapped in a `CustomAutoRooterGuard` [RAII root guard](https://github.com/servo/rust-mozjs/blob/ed5e37a288b5738d9b571b8100b4a22a2c00f075/src/rust.rs#L586-L588) (e.g. as `CustomAutoRooterGuard<Vec<JSVal>>`) I felt quite out of my depth when trying to adapt CodegenRust.py code, so I'd appreciate any help with how can be done better/in a more clean manner. Also the name `CustomAutoRooterGuard` is quite lengthy, so I'm also open to changing it (`AutoRoot`? `AutoRootGuard`?) --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #16678 (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: 989d2fd53267d063212ef3b7b7f13f2402943c4a
servo/Cargo.lock
servo/components/script/Cargo.toml
servo/components/script/dom/bindings/codegen/CodegenRust.py
servo/components/script/dom/testbinding.rs
servo/components/script/dom/webidls/TestBinding.webidl
--- a/servo/Cargo.lock
+++ b/servo/Cargo.lock
@@ -1687,17 +1687,17 @@ dependencies = [
 [[package]]
 name = "malloc_size_of"
 version = "0.0.1"
 dependencies = [
  "app_units 0.6.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "cssparser 0.23.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "euclid 0.16.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "hashglobe 0.1.0",
- "mozjs 0.1.9 (registry+https://github.com/rust-lang/crates.io-index)",
+ "mozjs 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)",
  "servo_arc 0.0.1",
  "smallbitvec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)",
  "smallvec 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "string_cache 0.7.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "url 1.6.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "webrender_api 0.56.1 (git+https://github.com/servo/webrender)",
  "xml5ever 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
@@ -1857,17 +1857,17 @@ dependencies = [
 
 [[package]]
 name = "mitochondria"
 version = "1.1.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 
 [[package]]
 name = "mozjs"
-version = "0.1.9"
+version = "0.1.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "cmake 0.1.29 (registry+https://github.com/rust-lang/crates.io-index)",
  "lazy_static 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "libc 0.2.33 (registry+https://github.com/rust-lang/crates.io-index)",
  "log 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)",
  "mozjs_sys 0.50.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "num-traits 0.1.37 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -2581,17 +2581,17 @@ dependencies = [
  "libc 0.2.33 (registry+https://github.com/rust-lang/crates.io-index)",
  "log 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)",
  "malloc_size_of 0.0.1",
  "malloc_size_of_derive 0.0.1",
  "metrics 0.0.1",
  "mime 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "mime_guess 1.8.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "mitochondria 1.1.2 (registry+https://github.com/rust-lang/crates.io-index)",
- "mozjs 0.1.9 (registry+https://github.com/rust-lang/crates.io-index)",
+ "mozjs 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)",
  "msg 0.0.1",
  "net_traits 0.0.1",
  "nonzero 0.0.1",
  "num-traits 0.1.37 (registry+https://github.com/rust-lang/crates.io-index)",
  "offscreen_gl_context 0.14.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "open 1.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "parking_lot 0.4.8 (registry+https://github.com/rust-lang/crates.io-index)",
  "phf 0.7.21 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -3891,17 +3891,17 @@ dependencies = [
 "checksum metadeps 1.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = "73b122901b3a675fac8cecf68dcb2f0d3036193bc861d1ac0e1c337f7d5254c2"
 "checksum mime 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)" = "9d69889cdc6336ed56b174514ce876c4c3dc564cc23dd872e7bca589bb2a36c8"
 "checksum mime 0.3.5 (registry+https://github.com/rust-lang/crates.io-index)" = "e2e00e17be181010a91dbfefb01660b17311059dc8c7f48b9017677721e732bd"
 "checksum mime_guess 1.8.1 (registry+https://github.com/rust-lang/crates.io-index)" = "76da6df85047af8c0edfa53f48eb1073012ce1cc95c8fedc0a374f659a89dd65"
 "checksum miniz-sys 0.1.9 (registry+https://github.com/rust-lang/crates.io-index)" = "28eaee17666671fa872e567547e8428e83308ebe5808cdf6a0e28397dbe2c726"
 "checksum mio 0.6.9 (registry+https://github.com/rust-lang/crates.io-index)" = "9e965267d4d58496fc4f740e9861118367f13570cadf66316ed2c3f2f14d87c7"
 "checksum miow 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "8c1f2f3b1cf331de6896aabf6e9d55dca90356cc9960cca7eaaf408a355ae919"
 "checksum mitochondria 1.1.2 (registry+https://github.com/rust-lang/crates.io-index)" = "9de3eca27871df31c33b807f834b94ef7d000956f57aa25c5aed9c5f0aae8f6f"
-"checksum mozjs 0.1.9 (registry+https://github.com/rust-lang/crates.io-index)" = "c9935bada5ea5d70291f21cce7b1bbad507edea12590ebe9cfb6b665fb716480"
+"checksum mozjs 0.1.10 (registry+https://github.com/rust-lang/crates.io-index)" = "2cd8370617e9a151ed9e7b49f38092075d0ae80bdf9f1dcd807a60cc9c3b7151"
 "checksum mozjs_sys 0.50.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ef1e24df9f76502cd4459919098ec1ac3af75ce694ec5b8837aa91f69f2ad0eb"
 "checksum mp3-metadata 0.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "4ab5f1d2693586420208d1200ce5a51cd44726f055b635176188137aff42c7de"
 "checksum mp4parse 0.9.1 (registry+https://github.com/rust-lang/crates.io-index)" = "f821e3799bc0fd16d9b861fb02fa7ee1b5fba29f45ad591dade105c48ca9a1a0"
 "checksum mp4parse_capi 0.9.1 (registry+https://github.com/rust-lang/crates.io-index)" = "395247952fbdb68f933dc008927c635d3c87386af3bccd6fb7ae555137028449"
 "checksum net2 0.2.29 (registry+https://github.com/rust-lang/crates.io-index)" = "bc01404e7568680f1259aa5729539f221cb1e6d047a0d9053cab4be8a73b5d67"
 "checksum nodrop 0.1.12 (registry+https://github.com/rust-lang/crates.io-index)" = "9a2228dca57108069a5262f2ed8bd2e82496d2e074a06d1ccc7ce1687b6ae0a2"
 "checksum nom 1.2.4 (registry+https://github.com/rust-lang/crates.io-index)" = "a5b8c256fd9471521bcb84c3cdba98921497f1a331cbc15b8030fc63b82050ce"
 "checksum num 0.1.37 (registry+https://github.com/rust-lang/crates.io-index)" = "98b15ba84e910ea7a1973bccd3df7b31ae282bf9d8bd2897779950c9b8303d40"
--- a/servo/components/script/Cargo.toml
+++ b/servo/components/script/Cargo.toml
@@ -60,17 +60,17 @@ lazy_static = "1"
 libc = "0.2"
 log = "0.3.5"
 malloc_size_of = { path = "../malloc_size_of" }
 malloc_size_of_derive = { path = "../malloc_size_of_derive" }
 metrics = {path = "../metrics"}
 mitochondria = "1.1.2"
 mime = "0.2.1"
 mime_guess = "1.8.0"
-mozjs = { version = "0.1.9", features = ["promises"]}
+mozjs = { version = "0.1.10", features = ["promises"]}
 msg = {path = "../msg"}
 net_traits = {path = "../net_traits"}
 nonzero = {path = "../nonzero"}
 num-traits = "0.1.32"
 offscreen_gl_context = { version = "0.14", features = ["serde"] }
 open = "1.1.1"
 parking_lot = "0.4"
 phf = "0.7.18"
--- a/servo/components/script/dom/bindings/codegen/CodegenRust.py
+++ b/servo/components/script/dom/bindings/codegen/CodegenRust.py
@@ -557,16 +557,17 @@ class JSToNativeConversionInfo():
         self.default = default
         self.declType = declType
 
 
 def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None,
                                 isDefinitelyObject=False,
                                 isMember=False,
                                 isArgument=False,
+                                isAutoRooted=False,
                                 invalidEnumValueFatal=True,
                                 defaultValue=None,
                                 treatNullAs="Default",
                                 isEnforceRange=False,
                                 isClamp=False,
                                 exceptionCode=None,
                                 allowTreatNonObjectAsNull=False,
                                 isCallbackReturnValue=False,
@@ -697,17 +698,18 @@ def getJSToNativeConversionInfo(type, de
                 "}")
         return templateBody
 
     assert not (isEnforceRange and isClamp)  # These are mutually exclusive
 
     if type.isSequence() or type.isRecord():
         innerInfo = getJSToNativeConversionInfo(innerContainerType(type),
                                                 descriptorProvider,
-                                                isMember=isMember)
+                                                isMember=isMember,
+                                                isAutoRooted=isAutoRooted)
         declType = wrapInNativeContainerType(type, innerInfo.declType)
         config = getConversionConfigForType(type, isEnforceRange, isClamp, treatNullAs)
 
         if type.nullable():
             declType = CGWrapper(declType, pre="Option<", post=" >")
 
         templateBody = ("match FromJSValConvertible::from_jsval(cx, ${val}, %s) {\n"
                         "    Ok(ConversionResult::Success(value)) => value,\n"
@@ -1033,20 +1035,24 @@ def getJSToNativeConversionInfo(type, de
             default = None
 
         return JSToNativeConversionInfo(template, default, finalDeclType)
 
     if type.isAny():
         assert not isEnforceRange and not isClamp
         assert isMember != "Union"
 
-        if isMember == "Dictionary":
+        if isMember == "Dictionary" or isAutoRooted:
             # TODO: Need to properly root dictionaries
             # https://github.com/servo/servo/issues/6381
-            declType = CGGeneric("Heap<JSVal>")
+            if isMember == "Dictionary":
+                declType = CGGeneric("Heap<JSVal>")
+            # AutoRooter can trace properly inner raw GC thing pointers
+            else:
+                declType = CGGeneric("JSVal")
 
             if defaultValue is None:
                 default = None
             elif isinstance(defaultValue, IDLNullValue):
                 default = "NullValue()"
             elif isinstance(defaultValue, IDLUndefinedValue):
                 default = "UndefinedValue()"
             else:
@@ -1233,16 +1239,17 @@ class CGArgumentConverter(CGThing):
             argument.type,
             descriptorProvider,
             invalidEnumValueFatal=invalidEnumValueFatal,
             defaultValue=argument.defaultValue,
             treatNullAs=argument.treatNullAs,
             isEnforceRange=argument.enforceRange,
             isClamp=argument.clamp,
             isMember="Variadic" if argument.variadic else False,
+            isAutoRooted=type_needs_auto_root(argument.type),
             allowTreatNonObjectAsNull=argument.allowTreatNonCallableAsNull())
         template = info.template
         default = info.default
         declType = info.declType
 
         if not argument.variadic:
             if argument.optional:
                 condition = "{args}.get({index}).is_undefined()".format(**replacer)
@@ -1255,18 +1262,25 @@ class CGArgumentConverter(CGThing):
                     assert not default
                     declType = CGWrapper(declType, pre="Option<", post=">")
                     template = CGIfElseWrapper(condition,
                                                CGGeneric("None"),
                                                CGGeneric("Some(%s)" % template)).define()
             else:
                 assert not default
 
+            arg = "arg%d" % index
+
             self.converter = instantiateJSToNativeConversionTemplate(
-                template, replacementVariables, declType, "arg%d" % index)
+                template, replacementVariables, declType, arg)
+
+            # The auto rooting is done only after the conversion is performed
+            if type_needs_auto_root(argument.type):
+                self.converter.append(CGGeneric("auto_root!(in(cx) let %s = %s);" % (arg, arg)))
+
         else:
             assert argument.optional
             variadicConversion = {
                 "val": string.Template("${args}.get(variadicArg)").substitute(replacer),
             }
             innerConverter = [instantiateJSToNativeConversionTemplate(
                 template, variadicConversion, declType, "slot")]
 
@@ -5693,16 +5707,17 @@ def generate_imports(config, cgthings, d
         'js::glue::RUST_JSID_IS_INT',
         'js::glue::RUST_JSID_IS_STRING',
         'js::glue::RUST_SYMBOL_TO_JSID',
         'js::glue::int_to_jsid',
         'js::glue::UnwrapObject',
         'js::panic::maybe_resume_unwind',
         'js::panic::wrap_panic',
         'js::rust::GCMethods',
+        'js::rust::CustomAutoRooterGuard',
         'js::rust::define_methods',
         'js::rust::define_properties',
         'js::rust::get_object_class',
         'dom',
         'dom::bindings',
         'dom::bindings::codegen::InterfaceObjectMap',
         'dom::bindings::constant::ConstantSpec',
         'dom::bindings::constant::ConstantVal',
@@ -6416,32 +6431,50 @@ def type_needs_tracing(t):
         return False
 
     if t.isEnum():
         return False
 
     assert False, (t, type(t))
 
 
+def type_needs_auto_root(t):
+    """
+    Certain IDL types, such as `sequence<any>` or `sequence<object>` need to be
+    traced and wrapped via (Custom)AutoRooter
+    """
+    assert isinstance(t, IDLObject), (t, type(t))
+
+    if t.isType():
+        if t.isSequence() and (t.inner.isAny() or t.inner.isObject()):
+            return True
+
+    return False
+
+
 def argument_type(descriptorProvider, ty, optional=False, defaultValue=None, variadic=False):
     info = getJSToNativeConversionInfo(
-        ty, descriptorProvider, isArgument=True)
+        ty, descriptorProvider, isArgument=True,
+        isAutoRooted=type_needs_auto_root(ty))
     declType = info.declType
 
     if variadic:
         if ty.isGeckoInterface():
             declType = CGWrapper(declType, pre="&[", post="]")
         else:
             declType = CGWrapper(declType, pre="Vec<", post=">")
     elif optional and not defaultValue:
         declType = CGWrapper(declType, pre="Option<", post=">")
 
     if ty.isDictionary() and not type_needs_tracing(ty):
         declType = CGWrapper(declType, pre="&")
 
+    if type_needs_auto_root(ty):
+        declType = CGTemplatedType("CustomAutoRooterGuard", declType)
+
     return declType.define()
 
 
 def method_arguments(descriptorProvider, returnType, arguments, passJSBits=True, trailing=None):
     if needCx(returnType, arguments, passJSBits):
         yield "cx", "*mut JSContext"
 
     for argument in arguments:
--- a/servo/components/script/dom/testbinding.rs
+++ b/servo/components/script/dom/testbinding.rs
@@ -33,16 +33,17 @@ use dom::blob::{Blob, BlobImpl};
 use dom::globalscope::GlobalScope;
 use dom::promise::Promise;
 use dom::promisenativehandler::{PromiseNativeHandler, Callback};
 use dom::url::URL;
 use dom_struct::dom_struct;
 use js::jsapi::{HandleObject, HandleValue, Heap, JSContext, JSObject};
 use js::jsapi::{JS_NewPlainObject, JS_NewUint8ClampedArray};
 use js::jsval::{JSVal, NullValue};
+use js::rust::CustomAutoRooterGuard;
 use script_traits::MsDuration;
 use servo_config::prefs::PREFS;
 use std::borrow::ToOwned;
 use std::ptr;
 use std::rc::Rc;
 use timers::OneshotTimerCallback;
 
 #[dom_struct]
@@ -444,16 +445,24 @@ impl TestBindingMethods for TestBinding 
     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>) {}
     fn PassSequence(&self, _: Vec<i32>) {}
+    #[allow(unsafe_code)]
+    unsafe fn PassAnySequence(&self, _: *mut JSContext, _: CustomAutoRooterGuard<Vec<JSVal>>) {}
+    #[allow(unsafe_code)]
+    unsafe fn AnySequencePassthrough(&self, _: *mut JSContext, seq: CustomAutoRooterGuard<Vec<JSVal>>) -> Vec<JSVal> {
+        (*seq).clone()
+    }
+    #[allow(unsafe_code)]
+    unsafe fn PassObjectSequence(&self, _: *mut JSContext, _: CustomAutoRooterGuard<Vec<*mut JSObject>>) {}
     fn PassStringSequence(&self, _: Vec<DOMString>) {}
     fn PassInterfaceSequence(&self, _: Vec<DomRoot<Blob>>) {}
 
     fn PassNullableBoolean(&self, _: Option<bool>) {}
     fn PassNullableByte(&self, _: Option<i8>) {}
     fn PassNullableOctet(&self, _: Option<u8>) {}
     fn PassNullableShort(&self, _: Option<i16>) {}
     fn PassNullableUnsignedShort(&self, _: Option<u16>) {}
--- a/servo/components/script/dom/webidls/TestBinding.webidl
+++ b/servo/components/script/dom/webidls/TestBinding.webidl
@@ -258,16 +258,19 @@ interface TestBinding {
   void passUnion10((DOMString or object) arg);
   void passUnionWithTypedef((Document or TestTypedef) arg);
   void passUnionWithTypedef2((sequence<long> or TestTypedef) arg);
   void passAny(any arg);
   void passObject(object arg);
   void passCallbackFunction(Function fun);
   void passCallbackInterface(EventListener listener);
   void passSequence(sequence<long> seq);
+  void passAnySequence(sequence<any> seq);
+  sequence<any> anySequencePassthrough(sequence<any> seq);
+  void passObjectSequence(sequence<object> seq);
   void passStringSequence(sequence<DOMString> seq);
   void passInterfaceSequence(sequence<Blob> seq);
 
   void passNullableBoolean(boolean? arg);
   void passNullableByte(byte? arg);
   void passNullableOctet(octet? arg);
   void passNullableShort(short? arg);
   void passNullableUnsignedShort(unsigned short? arg);