Auto merge of #20314 - Xanewok:remove-heap-constructor, r=jdm

Don't use unsafe Heap::new constructor

<!-- Please describe your changes on the following line: -->
Pulls https://github.com/servo/rust-mozjs/pull/398 and aims to close https://github.com/servo/rust-mozjs/issues/343.

We can't convert from `JSVal` to `Heap<JSVal>` safely (due to GC barriers we can't move Heap value after changing its underlying value to something meaningful, e.g. non-null or non-undefined), so I decided to also wrap the Heap values in a Box (and in dictionaries in RootedTraceableBox, see https://github.com/servo/servo/pull/20265#issuecomment-372838379 and the issue for more details) in dictionaries.

Since we allocate more to be safe, I think it'd be good to also do some sort of a JS perf run, if there is any to see if there's any significant overhead.

r? @jdm

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because checking for not moving Heap after setting a value would require encoding a lot more info in type system (Heap) and I'm not sure how to do that and end up with an ergonomic and consistent API

<!-- 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. -->

<!-- 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/20314)
<!-- Reviewable:end -->

Cargo.lock

@@ -1617,7 +1617,7 @@ dependencies = [
  "cssparser 0.23.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "euclid 0.17.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "hashglobe 0.1.0",
- "mozjs 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "mozjs 0.3.0 (git+https://github.com/Xanewok/rust-mozjs?branch=remove-heap-constructor)",
  "selectors 0.19.0",
  "servo_arc 0.1.1",
  "smallbitvec 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -1792,8 +1792,8 @@ dependencies = [
 
 [[package]]
 name = "mozjs"
-version = "0.2.0"
+version = "0.3.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
+source = "git+https://github.com/Xanewok/rust-mozjs?branch=remove-heap-constructor#d8b6ae7fcff342c2fddca8df5a7556bcc3b9e415"
 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)",
@@ -2467,7 +2467,7 @@ dependencies = [
  "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)",
  "mozangle 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)",
- "mozjs 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
+ "mozjs 0.3.0 (git+https://github.com/Xanewok/rust-mozjs?branch=remove-heap-constructor)",
  "msg 0.0.1",
  "net_traits 0.0.1",
  "num-traits 0.1.37 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -3862,7 +3862,7 @@ dependencies = [
 "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 mozangle 0.1.6 (registry+https://github.com/rust-lang/crates.io-index)" = "1f0583e6792917f498bb3a7440f777a59353102063445ab7f5e9d1dc4ed593aa"
-"checksum mozjs 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "79645a302ffcef7ca2673ff4db4d4273b87671dd0de4d53da889b664dd555e74"
+"checksum mozjs 0.3.0 (git+https://github.com/Xanewok/rust-mozjs?branch=remove-heap-constructor)" = "<none>"
 "checksum mozjs_sys 0.50.1 (registry+https://github.com/rust-lang/crates.io-index)" = "e61a792a125b1364c5ec50255ed8343ce02dc56098f8868dd209d472c8de006a"
 "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"

Cargo.toml

@@ -19,7 +19,7 @@ opt-level = 3
 # lto = false
 
 [patch.crates-io]
-
+mozjs = { git = "https://github.com/Xanewok/rust-mozjs", branch = "remove-heap-constructor" }
 # If you need to temporarily test Servo with a local fork of some upstream
 # crate, add that here. Use the form:
 #

components/malloc_size_of/Cargo.toml

@@ -16,7 +16,7 @@ app_units = "0.6"
 cssparser = "0.23.0"
 euclid = "0.17"
 hashglobe = { path = "../hashglobe" }
-mozjs = { version = "0.2", features = ["promises"], optional = true }
+mozjs = { version = "0.3", features = ["promises"], optional = true }
 selectors = { path = "../selectors" }
 servo_arc = { path = "../servo_arc" }
 smallbitvec = "1.0.3"

components/script/Cargo.toml

@@ -62,7 +62,7 @@ metrics = {path = "../metrics"}
 mitochondria = "1.1.2"
 mime = "0.2.1"
 mime_guess = "1.8.0"
-mozjs = { version = "0.2", features = ["promises"]}
+mozjs = { version = "0.3", features = ["promises"]}
 msg = {path = "../msg"}
 net_traits = {path = "../net_traits"}
 num-traits = "0.1.32"

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

@@ -1084,13 +1084,7 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None,
         assert isMember != "Union"
 
         if isMember == "Dictionary" or isAutoRooted:
-            # TODO: Need to properly root dictionaries
+            templateBody = "${val}.get()"
-            # https://github.com/servo/servo/issues/6381
-            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
@@ -1100,7 +1094,17 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None,
                 default = "UndefinedValue()"
             else:
                 raise TypeError("Can't handle non-null, non-undefined default value here")
-            return handleOptional("${val}.get()", declType, default)
+
+            if isMember == "Dictionary":
+                templateBody = "RootedTraceableBox::from_box(Heap::boxed(%s))" % templateBody
+                if default is not None:
+                    default = "RootedTraceableBox::from_box(Heap::boxed(%s))" % default
+                declType = CGGeneric("RootedTraceableBox<Heap<JSVal>>")
+            # AutoRooter can trace properly inner raw GC thing pointers
+            else:
+                declType = CGGeneric("JSVal")
+
+            return handleOptional(templateBody, declType, default)
 
         declType = CGGeneric("HandleValue")
 
@@ -1121,13 +1125,10 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None,
         templateBody = "${val}.get().to_object()"
         default = "ptr::null_mut()"
 
-        # TODO: Do we need to do the same for dictionaries?
+        if isMember in ("Dictionary", "Union"):
-        if isMember == "Union":
             templateBody = "RootedTraceableBox::from_box(Heap::boxed(%s))" % templateBody
             default = "RootedTraceableBox::new(Heap::default())"
             declType = CGGeneric("RootedTraceableBox<Heap<*mut JSObject>>")
-        elif isMember == "Dictionary":
-            declType = CGGeneric("Heap<*mut JSObject>")
         else:
             # TODO: Need to root somehow
             # https://github.com/servo/servo/issues/6382
@@ -6168,8 +6169,6 @@ class CGDictionary(CGThing):
             conversion = self.getMemberConversion(memberInfo, member.type)
             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):

components/script/dom/bindings/conversions.rs

@@ -48,7 +48,7 @@ use js::error::throw_type_error;
 use js::glue::{GetProxyPrivate, IsWrapper};
 use js::glue::{RUST_JSID_IS_INT, RUST_JSID_TO_INT};
 use js::glue::{RUST_JSID_IS_STRING, RUST_JSID_TO_STRING, UnwrapObject};
-use js::jsapi::{HandleId, HandleObject, HandleValue, JSContext, JSObject, JSString};
+use js::jsapi::{HandleId, HandleObject, HandleValue, Heap, JSContext, JSObject, JSString};
 use js::jsapi::{JS_GetLatin1StringCharsAndLength, JS_GetProperty, JS_GetReservedSlot};
 use js::jsapi::{JS_GetTwoByteStringCharsAndLength, JS_IsArrayObject, JS_IsExceptionPending};
 use js::jsapi::{JS_NewStringCopyN, JS_StringHasLatin1Chars, MutableHandleValue};
@@ -125,6 +125,25 @@ impl<T: ToJSValConvertible + JSTraceable> ToJSValConvertible for RootedTraceable
     }
 }
 
+impl<T> FromJSValConvertible for RootedTraceableBox<Heap<T>>
+    where
+        T: FromJSValConvertible + js::rust::GCMethods + Copy,
+        Heap<T>: JSTraceable + Default
+{
+    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(inner) =>
+                ConversionResult::Success(RootedTraceableBox::from_box(Heap::boxed(inner))),
+            ConversionResult::Failure(msg) => ConversionResult::Failure(msg),
+        })
+    }
+}
+
 /// Convert `id` to a `DOMString`. Returns `None` if `id` is not a string or
 /// integer.
 ///

components/script/dom/bindings/iterable.rs

@@ -11,7 +11,7 @@ use dom::bindings::codegen::Bindings::IterableIteratorBinding::IterableKeyOrValu
 use dom::bindings::error::Fallible;
 use dom::bindings::reflector::{DomObject, Reflector, reflect_dom_object};
 use dom::bindings::root::{Dom, DomRoot};
-use dom::bindings::trace::JSTraceable;
+use dom::bindings::trace::{JSTraceable, RootedTraceableBox};
 use dom::globalscope::GlobalScope;
 use dom_struct::dom_struct;
 use js::conversions::ToJSValConvertible;
@@ -131,10 +131,10 @@ fn key_and_value_return(cx: *mut JSContext,
                         value: HandleValue) -> Fallible<()> {
     let mut dict = unsafe { IterableKeyAndValueResult::empty(cx) };
     dict.done = false;
-    let values = vec![Heap::default(), Heap::default()];
+    dict.value = Some(vec![key, value]
-    values[0].set(key.get());
+        .into_iter()
-    values[1].set(value.get());
+        .map(|handle| RootedTraceableBox::from_box(Heap::boxed(handle.get())))
-    dict.value = Some(values);
+        .collect());
     rooted!(in(cx) let mut dict_value = UndefinedValue());
     unsafe {
         dict.to_jsval(cx, dict_value.handle_mut());

components/script/dom/testbinding.rs

@@ -348,12 +348,12 @@ impl TestBindingMethods for TestBinding {
     fn ReceiveNullableSequence(&self) -> Option<Vec<i32>> { Some(vec![1]) }
     fn ReceiveTestDictionaryWithSuccessOnKeyword(&self) -> RootedTraceableBox<TestDictionary> {
         RootedTraceableBox::new(TestDictionary {
-            anyValue: Heap::default(),
+            anyValue: RootedTraceableBox::new(Heap::default()),
             booleanValue: None,
             byteValue: None,
             dict: RootedTraceableBox::new(TestDictionaryDefaults {
                 UnrestrictedDoubleValue: 0.0,
-                anyValue: Heap::default(),
+                anyValue: RootedTraceableBox::new(Heap::default()),
                 booleanValue: false,
                 bytestringValue: ByteString::new(vec![]),
                 byteValue: 0,
@@ -369,7 +369,7 @@ impl TestBindingMethods for TestBinding {
                 nullableFloatValue: None,
                 nullableLongLongValue: None,
                 nullableLongValue: None,
-                nullableObjectValue: Heap::default(),
+                nullableObjectValue: RootedTraceableBox::new(Heap::default()),
                 nullableOctetValue: None,
                 nullableShortValue: None,
                 nullableStringValue: None,

tests/html/pair_iterable_perf.html

@@ -0,0 +1,44 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Value and pair iterable bindings</title>
+<script>
+  // Requires passing --pref=dom.testbinding.enabled to Servo binary
+  function collect(iter) {
+    var collection = [];
+    for (element of iter) {
+      collection.push(element);
+    }
+    return collection;
+  }
+
+  function measure_time(func) {
+    var start = performance.now();
+    func();
+    var stop = performance.now();
+
+    return (stop - start) / 1000.0;
+  };
+
+  const ENTRY_COUNT = 10000;
+  const RUN_COUNT = 10;
+
+  var benchMe = function() {
+    var t = new TestBindingPairIterable();
+
+    for (var i = 0; i < ENTRY_COUNT; i++) {
+      t.add(i.toString(), i);
+    }
+
+    var result = collect(t.entries());
+  };
+
+  var avg = 0;
+  for (var i = 0; i < RUN_COUNT; i++) {
+    var time = measure_time(benchMe);
+    avg += time;
+  }
+  avg /= RUN_COUNT;
+
+  console.log('Average running time across ' + RUN_COUNT + ' runs: ' + avg);
+
+</script>