Critical improvements for Map and Set polyfills.

Summary: Pull Request resolved: #21492

Differential Revision: D10288094

Pulled By: cpojer

fbshipit-source-id: b1ca7344dda3043553be6945614b439a0f42a46a

Libraries/Core/__tests__/MapAndSetPolyfills-test.js

@@ -0,0 +1,104 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @format
+ * @emails oncall+react_native
+ */
+'use strict';
+
+// Save these methods so that we can restore them afterward.
+const {freeze, seal, preventExtensions} = Object;
+
+function setup() {
+  jest.setMock('../../vendor/core/_shouldPolyfillES6Collection', () => true);
+  jest.unmock('_wrapObjectFreezeAndFriends');
+  require('_wrapObjectFreezeAndFriends');
+}
+
+function cleanup() {
+  Object.assign(Object, {freeze, seal, preventExtensions});
+}
+
+describe('Map polyfill', () => {
+  setup();
+
+  const Map = require('Map');
+
+  it('is not native', () => {
+    const getCode = Function.prototype.toString.call(Map.prototype.get);
+    expect(getCode).not.toContain('[native code]');
+    expect(getCode).toContain('getIndex');
+  });
+
+  it('should tolerate non-extensible object keys', () => {
+    const map = new Map();
+    const key = Object.create(null);
+    Object.freeze(key);
+    map.set(key, key);
+    expect(map.size).toBe(1);
+    expect(map.has(key)).toBe(true);
+    map.delete(key);
+    expect(map.size).toBe(0);
+    expect(map.has(key)).toBe(false);
+  });
+
+  it('should not get confused by prototypal inheritance', () => {
+    const map = new Map();
+    const proto = Object.create(null);
+    const base = Object.create(proto);
+    map.set(proto, proto);
+    expect(map.size).toBe(1);
+    expect(map.has(proto)).toBe(true);
+    expect(map.has(base)).toBe(false);
+    map.set(base, base);
+    expect(map.size).toBe(2);
+    expect(map.get(proto)).toBe(proto);
+    expect(map.get(base)).toBe(base);
+  });
+
+  afterAll(cleanup);
+});
+
+describe('Set polyfill', () => {
+  setup();
+
+  const Set = require('Set');
+
+  it('is not native', () => {
+    const addCode = Function.prototype.toString.call(Set.prototype.add);
+    expect(addCode).not.toContain('[native code]');
+  });
+
+  it('should tolerate non-extensible object elements', () => {
+    const set = new Set();
+    const elem = Object.create(null);
+    Object.freeze(elem);
+    set.add(elem);
+    expect(set.size).toBe(1);
+    expect(set.has(elem)).toBe(true);
+    set.add(elem);
+    expect(set.size).toBe(1);
+    set.delete(elem);
+    expect(set.size).toBe(0);
+    expect(set.has(elem)).toBe(false);
+  });
+
+  it('should not get confused by prototypal inheritance', () => {
+    const set = new Set();
+    const proto = Object.create(null);
+    const base = Object.create(proto);
+    set.add(proto);
+    expect(set.size).toBe(1);
+    expect(set.has(proto)).toBe(true);
+    expect(set.has(base)).toBe(false);
+    set.add(base);
+    expect(set.size).toBe(2);
+    expect(set.has(proto)).toBe(true);
+    expect(set.has(base)).toBe(true);
+  });
+
+  afterAll(cleanup);
+});

Libraries/Core/polyfillES6Collections.js

@@ -18,8 +18,10 @@ const {polyfillGlobal} = require('PolyfillFunctions');
  */
 const _shouldPolyfillCollection = require('_shouldPolyfillES6Collection');
 if (_shouldPolyfillCollection('Map')) {
+  require('_wrapObjectFreezeAndFriends');
   polyfillGlobal('Map', () => require('Map'));
 }
 if (_shouldPolyfillCollection('Set')) {
+  require('_wrapObjectFreezeAndFriends');
   polyfillGlobal('Set', () => require('Set'));
 }

Libraries/vendor/core/Map.js

@@ -26,6 +26,11 @@ module.exports = (function(global, undefined) {
     return global.Map;
   }
 
+  // In case this module has not already been evaluated, import it now.
+  require('./_wrapObjectFreezeAndFriends');
+
+  const hasOwn = Object.prototype.hasOwnProperty;
+
   /**
    * == ES6 Map Collection ==
    *
@@ -38,15 +43,17 @@ module.exports = (function(global, undefined) {
    *
    * https://people.mozilla.org/~jorendorff/es6-draft.html#sec-map-objects
    *
-   * There only two -- rather small -- diviations from the spec:
+   * There only two -- rather small -- deviations from the spec:
    *
-   * 1. The use of frozen objects as keys.
-   *    We decided not to allow and simply throw an error. The reason being is
-   *    we store a "hash" on the object for fast access to it's place in the
-   *    internal map entries.
-   *    If this turns out to be a popular use case it's possible to implement by
-   *    overiding `Object.freeze` to store a "hash" property on the object
-   *    for later use with the map.
+   * 1. The use of untagged frozen objects as keys.
+   *    We decided not to allow and simply throw an error, because this
+   *    implementation of Map works by tagging objects used as Map keys
+   *    with a secret hash property for fast access to the object's place
+   *    in the internal _mapData array. However, to limit the impact of
+   *    this spec deviation, Libraries/Core/InitializeCore.js also wraps
+   *    Object.freeze, Object.seal, and Object.preventExtensions so that
+   *    they tag objects before making them non-extensible, by inserting
+   *    each object into a Map and then immediately removing it.
    *
    * 2. The `size` property on a map object is a regular property and not a
    *    computed property on the prototype as described by the spec.
@@ -445,7 +452,7 @@ module.exports = (function(global, undefined) {
         // If the `SECRET_SIZE_PROP` property is already defined then we're not
         // in the first call to `initMap` (e.g. coming from `map.clear()`) so
         // all we need to do is reset the size without defining the properties.
-        if (map.hasOwnProperty(SECRET_SIZE_PROP)) {
+        if (hasOwn.call(map, SECRET_SIZE_PROP)) {
           map[SECRET_SIZE_PROP] = 0;
         } else {
           Object.defineProperty(map, SECRET_SIZE_PROP, {
@@ -524,51 +531,59 @@ module.exports = (function(global, undefined) {
     const hashProperty = '__MAP_POLYFILL_INTERNAL_HASH__';
     let hashCounter = 0;
 
+    const nonExtensibleObjects = [];
+    const nonExtensibleHashes = [];
+
     /**
      * Get the "hash" associated with an object.
      *
      * @param {object|array|function|regexp} o
      * @return {number}
      */
     return function getHash(o) {
-      // eslint-disable-line no-shadow
-      if (o[hashProperty]) {
-        return o[hashProperty];
-      } else if (
-        !isES5 &&
-        o.propertyIsEnumerable &&
-        o.propertyIsEnumerable[hashProperty]
-      ) {
-        return o.propertyIsEnumerable[hashProperty];
-      } else if (!isES5 && o[hashProperty]) {
+      if (hasOwn.call(o, hashProperty)) {
         return o[hashProperty];
       }
 
+      if (!isES5) {
+        if (hasOwn.call(o, "propertyIsEnumerable") &&
+            hasOwn.call(o.propertyIsEnumerable, hashProperty)) {
+          return o.propertyIsEnumerable[hashProperty];
+        }
+      }
+
       if (isExtensible(o)) {
-        hashCounter += 1;
         if (isES5) {
           Object.defineProperty(o, hashProperty, {
             enumerable: false,
             writable: false,
             configurable: false,
-            value: hashCounter,
+            value: ++hashCounter,
           });
-        } else if (o.propertyIsEnumerable) {
+          return hashCounter;
+        }
+
+        if (o.propertyIsEnumerable) {
           // Since we can't define a non-enumerable property on the object
           // we'll hijack one of the less-used non-enumerable properties to
           // save our hash on it. Additionally, since this is a function it
           // will not show up in `JSON.stringify` which is what we want.
           o.propertyIsEnumerable = function() {
             return propIsEnumerable.apply(this, arguments);
           };
-          o.propertyIsEnumerable[hashProperty] = hashCounter;
-        } else {
-          throw new Error('Unable to set a non-enumerable property on object.');
+          return o.propertyIsEnumerable[hashProperty] = ++hashCounter;
         }
-        return hashCounter;
-      } else {
-        throw new Error('Non-extensible objects are not allowed as keys.');
       }
+
+      // If the object is not extensible, fall back to storing it in an
+      // array and using Array.prototype.indexOf to find it.
+      let index = nonExtensibleObjects.indexOf(o);
+      if (index < 0) {
+        index = nonExtensibleObjects.length;
+        nonExtensibleObjects[index] = o;
+        nonExtensibleHashes[index] = ++hashCounter;
+      }
+      return nonExtensibleHashes[index];
     };
   })();
 

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js

@@ -0,0 +1,37 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @author Ben Newman (@benjamn) <ben@benjamn.com>
+ * @flow
+ * @format
+ */
+
+'use strict';
+
+let testMap; // Initialized lazily.
+function getTestMap() {
+  return testMap || (testMap = new (require('./Map'))());
+}
+
+// Wrap Object.{freeze,seal,preventExtensions} so each function adds its
+// argument to a Map first, which gives our ./Map.js polyfill a chance to
+// tag the object before it becomes non-extensible.
+["freeze", "seal", "preventExtensions"].forEach(name => {
+  const method = Object[name];
+  if (typeof method === "function") {
+    (Object: any)[name] = function (obj) {
+      try {
+        // If .set succeeds, also call .delete to avoid leaking memory.
+        getTestMap().set(obj, obj).delete(obj);
+      } finally {
+        // If .set fails, the exception will be silently swallowed
+        // by this return-from-finally statement, and the method will
+        // behave exactly as it did before it was wrapped.
+        return method.call(Object, obj);
+      }
+    };
+  }
+});