Bug 1729602 - Forward T constructor arguments through `Rooted<T> foo(cx, ...args...);` r=tcampbell

Differential Revision: https://phabricator.services.mozilla.com/D124869

js/public/RootingAPI.h

@@ -884,7 +884,11 @@ struct RootedTraceable final : public VirtualTraceable {
 
   T ptr;
 
-  template <typename U>
+  template <typename... CtorArgs>
+  explicit RootedTraceable(std::in_place_t, CtorArgs... args)
+      : ptr(std::forward<CtorArgs>(args)...) {}
+
+  template <typename U, typename = typename std::is_constructible<T, U>::type>
   MOZ_IMPLICIT RootedTraceable(U&& initial) : ptr(std::forward<U>(initial)) {}
 
   operator T&() { return ptr; }
@@ -1078,15 +1082,17 @@ class MOZ_RAII JS_PUBLIC_API CustomAutoRooter : private AutoGCRooter {
 
 namespace detail {
 
+template <typename T>
+constexpr bool IsTraceable_v =
+    MapTypeToRootKind<T>::kind == JS::RootKind::Traceable;
+
 template <typename T>
 using RootedPtr =
-    std::conditional_t<MapTypeToRootKind<T>::kind == JS::RootKind::Traceable,
+    std::conditional_t<IsTraceable_v<T>, js::RootedTraceable<T>, T>;
-                       js::RootedTraceable<T>, T>;
 
 template <typename T>
 using RootedPtrTraits =
-    std::conditional_t<MapTypeToRootKind<T>::kind == JS::RootKind::Traceable,
+    std::conditional_t<IsTraceable_v<T>, js::RootedTraceableTraits<T>,
-                       js::RootedTraceableTraits<T>,
                        js::RootedGCThingTraits<T>>;
 
 } /* namespace detail */
@@ -1121,20 +1127,45 @@ class MOZ_RAII Rooted : public js::RootedBase<T, Rooted<T>> {
   using ElementType = T;
 
   // Construct an empty Rooted holding a safely initialized but empty T.
-  template <typename RootingContext>
+  // Requires T to have a copy constructor in order to copy the safely
-  explicit Rooted(const RootingContext& cx)
+  // initialized value.
-      : ptr(SafelyInitialized<T>()) {
+  //
+  // Note that for SFINAE to reject this method, the 2nd template parameter must
+  // depend on RootingContext somehow even though we really only care about T.
+  template <typename RootingContext,
+            typename =
+                std::enable_if_t<std::is_copy_constructible_v<T>, RootingContext>>
+  explicit Rooted(const RootingContext& cx) : ptr(SafelyInitialized<T>()) {
     registerWithRootLists(rootLists(cx));
   }
 
-  // Provide an initial value.
+  // Provide an initial value. Requires T to be constructible from the given
-  template <typename RootingContext, typename S>
+  // argument.
+  template <typename RootingContext, typename S,
+            typename = typename std::is_constructible<T, S>::type>
   Rooted(const RootingContext& cx, S&& initial)
       : ptr(std::forward<S>(initial)) {
     MOZ_ASSERT(GCPolicy<T>::isValid(ptr));
     registerWithRootLists(rootLists(cx));
   }
 
+  // (Traceables only) Construct the contained value from the given arguments.
+  // Constructs in-place, so T does not need to be copyable or movable.
+  //
+  // Note that a copyable Traceable passed only a RootingContext will
+  // choose the above SafelyInitialized<T> constructor, because otherwise
+  // identical functions with parameter packs are considered less specialized.
+  //
+  // The SFINAE type must again depend on an inferred template parameter.
+  template <
+      typename RootingContext, typename... CtorArgs,
+      typename = std::enable_if_t<detail::IsTraceable_v<T>, RootingContext>>
+  explicit Rooted(const RootingContext& cx, CtorArgs... args)
+      : ptr(std::in_place, std::forward<CtorArgs>(args)...) {
+    MOZ_ASSERT(GCPolicy<T>::isValid(ptr));
+    registerWithRootLists(rootLists(cx));
+  }
+
   ~Rooted() {
     MOZ_ASSERT(*stack ==
                reinterpret_cast<Rooted<detail::RootListEntry*>*>(this));

js/src/jsapi-tests/testGCExactRooting.cpp

@@ -47,13 +47,49 @@ BEGIN_TEST(testGCSuppressions) {
 END_TEST(testGCSuppressions)
 
 struct MyContainer {
+  int whichConstructor;
   HeapPtr<JSObject*> obj;
   HeapPtr<JSString*> str;
 
-  MyContainer() : obj(nullptr), str(nullptr) {}
+  MyContainer() : whichConstructor(1), obj(nullptr), str(nullptr) {}
+  explicit MyContainer(double) : MyContainer() { whichConstructor = 2; }
+  explicit MyContainer(JSContext* cx) : MyContainer() { whichConstructor = 3; }
+  MyContainer(JSContext* cx, JSContext* cx2, JSContext* cx3) : MyContainer() {
+    whichConstructor = 4;
+  }
+  MyContainer(const MyContainer& rhs)
+      : whichConstructor(100 + rhs.whichConstructor),
+        obj(rhs.obj),
+        str(rhs.str) {}
   void trace(JSTracer* trc) {
-    js::TraceNullableEdge(trc, &obj, "test container");
+    js::TraceNullableEdge(trc, &obj, "test container obj");
-    js::TraceNullableEdge(trc, &str, "test container");
+    js::TraceNullableEdge(trc, &str, "test container str");
+  }
+};
+
+struct MyNonCopyableContainer {
+  int whichConstructor;
+  HeapPtr<JSObject*> obj;
+  HeapPtr<JSString*> str;
+
+  MyNonCopyableContainer() : whichConstructor(1), obj(nullptr), str(nullptr) {}
+  explicit MyNonCopyableContainer(double) : MyNonCopyableContainer() {
+    whichConstructor = 2;
+  }
+  explicit MyNonCopyableContainer(JSContext* cx) : MyNonCopyableContainer() {
+    whichConstructor = 3;
+  }
+  explicit MyNonCopyableContainer(JSContext* cx, JSContext* cx2, JSContext* cx3)
+      : MyNonCopyableContainer() {
+    whichConstructor = 4;
+  }
+
+  MyNonCopyableContainer(const MyNonCopyableContainer&) = delete;
+  MyNonCopyableContainer& operator=(const MyNonCopyableContainer&) = delete;
+
+  void trace(JSTracer* trc) {
+    js::TraceNullableEdge(trc, &obj, "test container obj");
+    js::TraceNullableEdge(trc, &str, "test container str");
   }
 };
 
@@ -62,10 +98,50 @@ template <typename Wrapper>
 struct MutableWrappedPtrOperations<MyContainer, Wrapper> {
   HeapPtr<JSObject*>& obj() { return static_cast<Wrapper*>(this)->get().obj; }
   HeapPtr<JSString*>& str() { return static_cast<Wrapper*>(this)->get().str; }
+  int constructor() {
+    return static_cast<Wrapper*>(this)->get().whichConstructor;
+  }
+};
+
+template <typename Wrapper>
+struct MutableWrappedPtrOperations<MyNonCopyableContainer, Wrapper> {
+  HeapPtr<JSObject*>& obj() { return static_cast<Wrapper*>(this)->get().obj; }
+  HeapPtr<JSString*>& str() { return static_cast<Wrapper*>(this)->get().str; }
+  int constructor() {
+    return static_cast<Wrapper*>(this)->get().whichConstructor;
+  }
 };
 }  // namespace js
 
 BEGIN_TEST(testGCRootedStaticStructInternalStackStorageAugmented) {
+  // Test Rooted constructors for a copyable type.
+  JS::Rooted<MyContainer> r1(cx);
+  JS::Rooted<MyContainer> r2(cx, 3.4);
+  JS::Rooted<MyContainer> r3(cx, MyContainer(cx));
+  JS::Rooted<MyContainer> r4(cx, cx);
+  JS::Rooted<MyContainer> r5(cx, cx, cx, cx);
+
+  JS::Rooted<Value> rv(cx);
+
+  CHECK(r1.constructor() == 101);  // copy of SafelyInitialized<T>
+  CHECK(r2.constructor() == 2);    // direct MyContainer(3.4)
+  CHECK(r3.constructor() == 103);  // copy of MyContainer(cx)
+  CHECK(r4.constructor() == 3);    // direct MyContainer(cx)
+  CHECK(r5.constructor() == 4);    // direct MyContainer(cx, cx, cx)
+
+  // Test Rooted constructor forwarding for a non-copyable type.
+  JS::Rooted<MyNonCopyableContainer> nc1(cx);
+  JS::Rooted<MyNonCopyableContainer> nc2(cx, 3.4);
+  // Compile error: cannot copy
+  // JS::Rooted<MyNonCopyableContainer> nc3(cx, MyNonCopyableContainer(cx));
+  JS::Rooted<MyNonCopyableContainer> nc4(cx, cx);
+  JS::Rooted<MyNonCopyableContainer> nc5(cx, cx, cx, cx);
+
+  CHECK(nc1.constructor() == 1);  // direct MyNonCopyableContainer()
+  CHECK(nc2.constructor() == 2);  // direct MyNonCopyableContainer(3.4)
+  CHECK(nc4.constructor() == 3);  // direct MyNonCopyableContainer(cx)
+  CHECK(nc5.constructor() == 4);  // direct MyNonCopyableContainer(cx, cx, cx)
+
   JS::Rooted<MyContainer> container(cx);
   container.obj() = JS_NewObject(cx, nullptr);
   container.str() = JS_NewStringCopyZ(cx, "Hello");