From 90850cace9991ed0a02605586ea5c32ce099de65 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Mon, 28 Jan 2019 01:49:35 -0800 Subject: [PATCH] Critical improvements for Map and Set polyfills. Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/21492 Differential Revision: D10288094 Pulled By: cpojer fbshipit-source-id: b1ca7344dda3043553be6945614b439a0f42a46a --- .../Core/__tests__/MapAndSetPolyfills-test.js | 104 ++++++++++++++++++ Libraries/Core/polyfillES6Collections.js | 2 + Libraries/vendor/core/Map.js | 71 +++++++----- .../core/_wrapObjectFreezeAndFriends.js | 37 +++++++ 4 files changed, 186 insertions(+), 28 deletions(-) create mode 100644 Libraries/Core/__tests__/MapAndSetPolyfills-test.js create mode 100644 Libraries/vendor/core/_wrapObjectFreezeAndFriends.js diff --git a/Libraries/Core/__tests__/MapAndSetPolyfills-test.js b/Libraries/Core/__tests__/MapAndSetPolyfills-test.js new file mode 100644 index 00000000000000..79a73a968789cc --- /dev/null +++ b/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); +}); diff --git a/Libraries/Core/polyfillES6Collections.js b/Libraries/Core/polyfillES6Collections.js index ca1ee798c73f4c..afbb24ab23ba4e 100644 --- a/Libraries/Core/polyfillES6Collections.js +++ b/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')); } diff --git a/Libraries/vendor/core/Map.js b/Libraries/vendor/core/Map.js index 4251479a64e1be..8643cebb9e6fce 100644 --- a/Libraries/vendor/core/Map.js +++ b/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,6 +531,9 @@ 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. * @@ -531,29 +541,29 @@ module.exports = (function(global, undefined) { * @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 @@ -561,14 +571,19 @@ module.exports = (function(global, undefined) { 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]; }; })(); diff --git a/Libraries/vendor/core/_wrapObjectFreezeAndFriends.js b/Libraries/vendor/core/_wrapObjectFreezeAndFriends.js new file mode 100644 index 00000000000000..c3ab8a08556fd5 --- /dev/null +++ b/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) + * @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); + } + }; + } +});