Bug 1654851 - Correctly handle transplanting objects with private fields r=jorendorff,extension-reviewers,zombie

As part of DOM node adoption they are transplanted, and their expando chains are copied over.

This copying uses JS_CopyPropertiesFrom, which until this patch, cannot see
private fields as they are excluded from property iteration. This patch adds
property iteration for private fields, and renames JS_CopyPropertiesFrom to
JS_CopyOwnPropertiesAndPrivateFields which is more accurate.

The users of this method are all doing object manipulation in ways where
preserving the copied private field is the better default.

In addition to testing DOM nodes explicitly, this patch also adds a jit-test
which uses transplantableObject to test similar things with FakeDOMObjects.

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

dom/bindings/BindingUtils.cpp

@@ -2207,7 +2207,7 @@ void UpdateReflectorGlobal(JSContext* aCx, JS::Handle<JSObject*> aObjArg,
       return;
     }
 
-    if (!JS_CopyPropertiesFrom(aCx, propertyHolder, copyFrom)) {
+    if (!JS_CopyOwnPropertiesAndPrivateFields(aCx, propertyHolder, copyFrom)) {
       aError.StealExceptionFromJSContext(aCx);
       return;
     }
@@ -2220,7 +2220,8 @@ void UpdateReflectorGlobal(JSContext* aCx, JS::Handle<JSObject*> aObjArg,
   //
   // NB: It's important to do this _after_ copying the properties to
   // propertyHolder. Otherwise, an object with |foo.x === foo| will
-  // crash when JS_CopyPropertiesFrom tries to call wrap() on foo.x.
+  // crash when JS_CopyOwnPropertiesAndPrivateFields tries to call wrap() on
+  // foo.x.
   js::SetReservedSlot(newobj, DOM_OBJECT_SLOT,
                       js::GetReservedSlot(aObj, DOM_OBJECT_SLOT));
   js::SetReservedSlot(aObj, DOM_OBJECT_SLOT, JS::PrivateValue(nullptr));
@@ -2242,7 +2243,8 @@ void UpdateReflectorGlobal(JSContext* aCx, JS::Handle<JSObject*> aObjArg,
       copyTo = aObj;
     }
 
-    if (!copyTo || !JS_CopyPropertiesFrom(aCx, copyTo, propertyHolder)) {
+    if (!copyTo ||
+        !JS_CopyOwnPropertiesAndPrivateFields(aCx, copyTo, propertyHolder)) {
       MOZ_CRASH();
     }
   }

js/rust/build.rs

@@ -282,7 +282,7 @@ const WHITELIST_FUNCTIONS: &'static [&'static str] = &[
     "JS::CompileFunctionUtf8",
     "JS::Construct",
     "JS::ContextOptionsRef",
-    "JS_CopyPropertiesFrom",
+    "JS_CopyOwnPropertiesAndPrivateFields",
     "JS::CurrentGlobalOrNull",
     "JS_DeletePropertyById",
     "js::detail::IsWindowSlow",

js/src/jit-test/tests/fields/transplant.js

@@ -0,0 +1,51 @@
+// |jit-test| --enable-private-fields;
+
+
+class Base {
+  constructor(o) {
+    return o;
+  }
+}
+
+class A extends Base {
+  #x = 10;
+  static gx(o) {
+    return o.#x
+  }
+  static sx(o, v) {
+    o.#x = v;
+  }
+}
+
+function transplantTest(transplantOptions, global) {
+  var {object, transplant} = transplantableObject(transplantOptions);
+
+  new A(object);
+  assertEq(A.gx(object), 10);
+  A.sx(object, 15);
+  assertEq(A.gx(object), 15);
+
+  transplant(global);
+
+  assertEq(A.gx(object), 15);
+  A.sx(object, 29);
+  assertEq(A.gx(object), 29);
+}
+
+// Structure helpfully provided by bug1403679.js
+const thisGlobal = this;
+const otherGlobalSameCompartment = newGlobal({sameCompartmentAs: thisGlobal});
+const otherGlobalNewCompartment = newGlobal({newCompartment: true});
+
+const globals =
+    [thisGlobal, otherGlobalSameCompartment, otherGlobalNewCompartment];
+
+function testWithOptions(fn) {
+  for (let global of globals) {
+    for (let options of [{}, {proxy: true}, {object: new FakeDOMObject()}, ]) {
+      fn(options, global);
+    }
+  }
+}
+
+testWithOptions(transplantTest)

js/src/jsfriendapi.h

@@ -212,15 +212,15 @@ extern JS_FRIEND_API bool GetIsSecureContext(JS::Realm* realm);
 }  // namespace JS
 
 /**
- * Copies all own properties from |obj| to |target|. Both |obj| and |target|
- * must not be cross-compartment wrappers because we have to enter their realms.
+ * Copies all own properties and private fields from |obj| to |target|. Both
+ * |obj| and |target| must not be cross-compartment wrappers because we have to
+ * enter their realms.
  *
  * This function immediately enters a realm, and does not impose any
  * restrictions on the realm of |cx|.
  */
-extern JS_FRIEND_API bool JS_CopyPropertiesFrom(JSContext* cx,
-                                                JS::HandleObject target,
-                                                JS::HandleObject obj);
+extern JS_FRIEND_API bool JS_CopyOwnPropertiesAndPrivateFields(
+    JSContext* cx, JS::HandleObject target, JS::HandleObject obj);
 
 /*
  * Single-property version of the above. This function asserts that an |own|
@@ -808,7 +808,7 @@ JS_FRIEND_API bool IsObjectInContextCompartment(JSObject* obj,
  */
 /* 0x1 is no longer used */
 /* 0x2 is no longer used */
-/* 0x4 is no longer used */
+#define JSITER_PRIVATE 0x4      /* Include private names in iteration */
 #define JSITER_OWNONLY 0x8      /* iterate over obj's own properties only */
 #define JSITER_HIDDEN 0x10      /* also enumerate non-enumerable properties */
 #define JSITER_SYMBOLS 0x20     /* also include symbol property keys */

js/src/shell/js.cpp

@@ -8326,7 +8326,7 @@ static bool TransplantObject(JSContext* cx, unsigned argc, Value* vp) {
     return false;
   }
 
-  if (!JS_CopyPropertiesFrom(cx, propertyHolder, copyFrom)) {
+  if (!JS_CopyOwnPropertiesAndPrivateFields(cx, propertyHolder, copyFrom)) {
     return false;
   }
 
@@ -8348,7 +8348,7 @@ static bool TransplantObject(JSContext* cx, unsigned argc, Value* vp) {
   } else {
     copyTo = source;
   }
-  if (!JS_CopyPropertiesFrom(cx, copyTo, propertyHolder)) {
+  if (!JS_CopyOwnPropertiesAndPrivateFields(cx, copyTo, propertyHolder)) {
     return false;
   }
 

js/src/vm/Iteration.cpp

@@ -126,9 +126,12 @@ static inline bool Enumerate(JSContext* cx, HandleObject pobj, jsid id,
   // Symbol-keyed properties and nonenumerable properties are skipped unless
   // the caller specifically asks for them. A caller can also filter out
   // non-symbols by asking for JSITER_SYMBOLSONLY. PrivateName symbols are
-  // always skipped.
+  // skipped unless JSITER_PRIVATE is passed.
   if (JSID_IS_SYMBOL(id)) {
-    if (!(flags & JSITER_SYMBOLS) || JSID_TO_SYMBOL(id)->isPrivateName()) {
+    if (!(flags & JSITER_SYMBOLS)) {
+      return true;
+    }
+    if (!(flags & JSITER_PRIVATE) && JSID_TO_SYMBOL(id)->isPrivateName()) {
       return true;
     }
   } else {
@@ -401,8 +404,6 @@ struct SortComparatorIds {
     RootedString astr(cx), bstr(cx);
     if (JSID_IS_SYMBOL(a)) {
       MOZ_ASSERT(JSID_IS_SYMBOL(b));
-      MOZ_ASSERT(!JSID_TO_SYMBOL(a)->isPrivateName());
-      MOZ_ASSERT(!JSID_TO_SYMBOL(b)->isPrivateName());
       JS::SymbolCode ca = JSID_TO_SYMBOL(a)->code();
       JS::SymbolCode cb = JSID_TO_SYMBOL(b)->code();
       if (ca != cb) {
@@ -552,7 +553,7 @@ JS_FRIEND_API bool js::GetPropertyKeys(JSContext* cx, HandleObject obj,
                                        MutableHandleIdVector props) {
   return Snapshot(cx, obj,
                   flags & (JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS |
-                           JSITER_SYMBOLSONLY),
+                           JSITER_SYMBOLSONLY | JSITER_PRIVATE),
                   props);
 }
 

js/src/vm/JSObject.cpp

@@ -1140,8 +1140,9 @@ JS_FRIEND_API bool JS_CopyPropertyFrom(JSContext* cx, HandleId id,
   return DefineProperty(cx, target, wrappedId, desc);
 }
 
-JS_FRIEND_API bool JS_CopyPropertiesFrom(JSContext* cx, HandleObject target,
-                                         HandleObject obj) {
+JS_FRIEND_API bool JS_CopyOwnPropertiesAndPrivateFields(JSContext* cx,
+                                                        HandleObject target,
+                                                        HandleObject obj) {
   // Both |obj| and |target| must not be CCWs because we need to enter their
   // realms below and CCWs are not associated with a single realm.
   MOZ_ASSERT(!IsCrossCompartmentWrapper(obj));
@@ -1150,8 +1151,10 @@ JS_FRIEND_API bool JS_CopyPropertiesFrom(JSContext* cx, HandleObject target,
   JSAutoRealm ar(cx, obj);
 
   RootedIdVector props(cx);
-  if (!GetPropertyKeys(cx, obj, JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS,
-                       &props)) {
+  if (!GetPropertyKeys(
+          cx, obj,
+          JSITER_PRIVATE | JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS,
+          &props)) {
     return false;
   }
 

js/xpconnect/wrappers/XrayWrapper.cpp

@@ -1443,7 +1443,7 @@ bool XrayTraits::cloneExpandoChain(JSContext* cx, HandleObject dst,
 
     if (movingIntoXrayCompartment) {
       // Just copy properties directly onto dst.
-      if (!JS_CopyPropertiesFrom(cx, dst, oldHead)) {
+      if (!JS_CopyOwnPropertiesAndPrivateFields(cx, dst, oldHead)) {
         return false;
       }
     } else {
@@ -1453,7 +1453,7 @@ bool XrayTraits::cloneExpandoChain(JSContext* cx, HandleObject dst,
           cx,
           attachExpandoObject(cx, dst, exclusiveWrapper, exclusiveWrapperGlobal,
                               GetExpandoObjectPrincipal(oldHead)));
-      if (!JS_CopyPropertiesFrom(cx, newHead, oldHead)) {
+      if (!JS_CopyOwnPropertiesAndPrivateFields(cx, newHead, oldHead)) {
         return false;
       }
     }

toolkit/components/extensions/test/xpcshell/test_ext_adoption_with_private_field_xrays.js

@@ -0,0 +1,131 @@
+'use strict';
+
+// ExtensionContent.jsm needs to know when it's running from xpcshell,
+// to use the right timeout for content scripts executed at document_idle.
+ExtensionTestUtils.mockAppInfo();
+const server = createHttpServer();
+server.registerDirectory('/data/', do_get_file('data'));
+
+const BASE_URL = `http://localhost:${server.identity.primaryPort}/data`;
+
+add_task(async function test_contentscript_private_field_xrays() {
+  async function contentScript() {
+    let node = window.document.createElement('div');
+
+    class Base {
+      constructor(o) {
+        return o;
+      }
+    }
+
+    class A extends Base {
+      #x = 5;
+      static gx(o) {
+        return o.#x;
+      }
+      static sx(o, v) {
+        o.#x = v;
+      }
+    }
+
+    browser.test.log(A.toString());
+
+    // Stamp node with A's private field.
+    new A(node);
+
+    browser.test.log('stamped');
+
+    browser.test.assertEq(
+        A.gx(node), 5, 'We should be able to see our expando private field');
+    browser.test.log('Read');
+    browser.test.assertThrows(
+        () => A.gx(node.wrappedJSObject), /Trying to read undeclared field/,
+        'Underlying object should not have our private field');
+
+    browser.test.log('threw');
+    window.frames[0].document.adoptNode(node);
+    browser.test.log('adopted');
+    browser.test.assertEq(
+        A.gx(node), 5, 'Adoption should not change expando private field');
+    browser.test.log('read');
+    browser.test.assertThrows(
+        () => A.gx(node.wrappedJSObject), /Trying to read undeclared field/,
+        'Adoption should really not change expandos private fields');
+    browser.test.log('threw2');
+
+    // Repeat but now with an object that has a reference from the
+    // window it's being cloned into.
+    node = window.document.createElement('div');
+    // Stamp node with A's private field.
+    new A(node);
+    A.sx(node, 6);
+
+    browser.test.assertEq(
+        A.gx(node), 6, 'We should be able to see our expando (2)');
+    browser.test.assertThrows(
+        () => A.gx(node.wrappedJSObject), /Trying to read undeclared field/,
+        'Underlying object should not have exxpando. (2)');
+
+    window.frames[0].wrappedJSObject.incoming = node.wrappedJSObject;
+    window.frames[0].document.adoptNode(node);
+
+    browser.test.assertEq(
+        A.gx(node), 6, 'We should be able to see our expando (3)');
+    browser.test.assertThrows(
+        () => A.gx(node.wrappedJSObject), /Trying to read undeclared field/,
+        'Underlying object should not have exxpando. (3)');
+
+    // Repeat once more, now with an expando that refers to the object itself
+    node = window.document.createElement('div');
+    new A(node);
+    A.sx(node, node);
+
+    browser.test.assertEq(
+        A.gx(node), node,
+        'We should be able to see our self-referential expando (4)');
+    browser.test.assertThrows(
+        () => A.gx(node.wrappedJSObject), /Trying to read undeclared field/,
+        'Underlying object should not have exxpando. (4)');
+
+    window.frames[0].document.adoptNode(node);
+
+    browser.test.assertEq(
+        A.gx(node), node,
+        'Adoption should not change our self-referential expando (4)');
+    browser.test.assertThrows(
+        () => A.gx(node.wrappedJSObject), /Trying to read undeclared field/,
+        'Adoption should not change underlying object. (4)');
+
+    // And test what happens if we now set document.domain and cause
+    // wrapper remapping.
+    let doc = window.frames[0].document;
+    // eslint-disable-next-line no-self-assign
+    doc.domain = doc.domain;
+
+    browser.test.notifyPass('privateFieldXRayAdoption');
+  }
+
+  let extension = ExtensionTestUtils.loadExtension({
+    manifest: {
+      content_scripts: [
+        {
+          matches: ['http://*/*/file_toplevel.html'],
+          js: ['content_script.js'],
+        },
+      ],
+    },
+
+    files: {
+      'content_script.js': contentScript,
+    },
+  });
+
+  await extension.startup();
+  let contentPage = await ExtensionTestUtils.loadContentPage(
+      `${BASE_URL}/file_toplevel.html`);
+
+  await extension.awaitFinish('privateFieldXRayAdoption');
+
+  await contentPage.close();
+  await extension.unload();
+});

toolkit/components/extensions/test/xpcshell/xpcshell-content.ini

@@ -1,3 +1,7 @@
+[DEFAULT]
+prefs =
+  javascript.options.experimental.private_fields=true
+
 [test_ext_i18n.js]
 skip-if = os == "android" || (os == "win" && debug) || (os == "linux")
 [test_ext_i18n_css.js]
@@ -11,6 +15,7 @@ skip-if = (os == "android" && debug) || (os == "win" && debug) || tsan || socket
 [test_ext_contentScripts_register.js]
 [test_ext_contexts_gc.js]
 [test_ext_adoption_with_xrays.js]
+[test_ext_adoption_with_private_field_xrays.js]
 [test_ext_shadowdom.js]
 skip-if = ccov && os == 'linux' # bug 1607581
 [test_ext_web_accessible_resources.js]