From df2bc8c4f6ad427c18a3e6c857f4a983b6496e9e Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 4 Oct 2018 18:16:49 -0400 Subject: [PATCH 1/5] Wrap Object.{freeze,seal,...} to tag objects for Map compatibility. I believe using non-extensible objects as keys in a Map should work, because there's no excuse for a deviation from the spec unless it's somehow impossible to follow the spec. Also, I have real use cases to back up this belief, if anyone is interested. Most other Map polyfills avoid this edge case by using a linear-time lookup for object keys, but the constant-time lookup provided by the tagging approach is an important observable property of Map behavior. I would like to stop recommending my customers find a replacement for the React Native Map polyfill, since the replacements all have their drawbacks. Hence this commit. As suggested in the previous comments, wrapping Object.freeze, Object.seal, and Object.preventExtensions is a good way to make sure objects get tagged before they become non-extensible. More specifically, the wrappers for these methods simply insert the object into a Map and then immediately remove it, so the Map implementation has a chance to do whatever it likes with the object before it gets frozen. --- .../Core/__tests__/MapAndSetPolyfills-test.js | 104 ++++++++++++++++++ Libraries/Core/polyfillES6Collections.js | 2 + Libraries/vendor/core/Map.js | 21 ++-- .../core/_wrapObjectFreezeAndFriends.js | 35 ++++++ 4 files changed, 154 insertions(+), 8 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..d9ce89bd277be4 100644 --- a/Libraries/vendor/core/Map.js +++ b/Libraries/vendor/core/Map.js @@ -26,6 +26,9 @@ module.exports = (function(global, undefined) { return global.Map; } + // In case this module has not already been evaluated, import it now. + require('./_wrapObjectFreezeAndFriends'); + /** * == ES6 Map Collection == * @@ -38,15 +41,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. diff --git a/Libraries/vendor/core/_wrapObjectFreezeAndFriends.js b/Libraries/vendor/core/_wrapObjectFreezeAndFriends.js new file mode 100644 index 00000000000000..f7dbe2c4182725 --- /dev/null +++ b/Libraries/vendor/core/_wrapObjectFreezeAndFriends.js @@ -0,0 +1,35 @@ +/** + * 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) + */ + +'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[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); + } + }; + } +}); From 3ce6db8fda42642befff0334b5451878e1b3a575 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 4 Oct 2018 16:57:39 -0400 Subject: [PATCH 2/5] Use hasOwnProperty to prevent accidental hashProperty inheritance. This was a fairly serious undocumented spec deviation: before this change, if an object was ever used as a Map key, any other objects with that object in their prototype chains would appear to have the same hash, because the hashProperty key would be inherited. In my case, I was storing Object.getPrototypeOf(someObject) in a Map as part of a strategy for determining if I'd ever seen a structurally similar object before. Imagine the chaos when that prototype object happened to be none other than... Object.prototype! :skull::jack_o_lantern::ghost: --- Libraries/vendor/core/Map.js | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/Libraries/vendor/core/Map.js b/Libraries/vendor/core/Map.js index d9ce89bd277be4..cbcfa1b662f92b 100644 --- a/Libraries/vendor/core/Map.js +++ b/Libraries/vendor/core/Map.js @@ -29,6 +29,8 @@ module.exports = (function(global, undefined) { // In case this module has not already been evaluated, import it now. require('./_wrapObjectFreezeAndFriends'); + const hasOwn = Object.prototype.hasOwnProperty; + /** * == ES6 Map Collection == * @@ -450,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, { @@ -536,19 +538,17 @@ 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) { From de09adf4fd6f62ea8a75e44cbc2a94bfeb248cdd Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Thu, 4 Oct 2018 17:07:35 -0400 Subject: [PATCH 3/5] Store non-extensible Map keys in an Array, as other polyfills do. Now that Object.{freeze,seal,preventExtensions} are wrapped to make sure their argument objects get tagged before they become non-extensible, this case should be exercised only for object keys that were somehow frozen before the wrapping happened. Since that's pretty unlikely, the length of these arrays should be small in practice, so the performance penalty of the linear-time lookup should be negligible. --- Libraries/vendor/core/Map.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/Libraries/vendor/core/Map.js b/Libraries/vendor/core/Map.js index cbcfa1b662f92b..16393f52d66957 100644 --- a/Libraries/vendor/core/Map.js +++ b/Libraries/vendor/core/Map.js @@ -531,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. * @@ -572,7 +575,15 @@ module.exports = (function(global, undefined) { } 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]; } }; })(); From 139b2239983e52a1f36c6b993bb526f8cf2637ec Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Sat, 26 Jan 2019 11:50:27 -0500 Subject: [PATCH 4/5] Fall through to nonExtensibleObjects.indexOf when all else fails. Thanks to @pvdz for this suggestion: https://github.com/facebook/react-native/pull/21492/files#r224349711 --- Libraries/vendor/core/Map.js | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/Libraries/vendor/core/Map.js b/Libraries/vendor/core/Map.js index 16393f52d66957..8643cebb9e6fce 100644 --- a/Libraries/vendor/core/Map.js +++ b/Libraries/vendor/core/Map.js @@ -553,15 +553,17 @@ module.exports = (function(global, undefined) { } 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 @@ -569,22 +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 { - // 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]; } + + // 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]; }; })(); From 9dbbd1801892d1a52f237130e38eadcf53c1f268 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Sat, 26 Jan 2019 11:51:17 -0500 Subject: [PATCH 5/5] Add `@format` and `@flow` directives to _wrapObjectFreezeAndFriends.js. The `@format` directive was requested by @yungsters: https://github.com/facebook/react-native/pull/21492/files#r223949980 And the `@flow` directive by @analysis-bot: https://github.com/facebook/react-native/pull/21492#discussion_r251205921 --- Libraries/vendor/core/_wrapObjectFreezeAndFriends.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Libraries/vendor/core/_wrapObjectFreezeAndFriends.js b/Libraries/vendor/core/_wrapObjectFreezeAndFriends.js index f7dbe2c4182725..c3ab8a08556fd5 100644 --- a/Libraries/vendor/core/_wrapObjectFreezeAndFriends.js +++ b/Libraries/vendor/core/_wrapObjectFreezeAndFriends.js @@ -5,6 +5,8 @@ * LICENSE file in the root directory of this source tree. * * @author Ben Newman (@benjamn) + * @flow + * @format */ 'use strict'; @@ -20,7 +22,7 @@ function getTestMap() { ["freeze", "seal", "preventExtensions"].forEach(name => { const method = Object[name]; if (typeof method === "function") { - Object[name] = function (obj) { + (Object: any)[name] = function (obj) { try { // If .set succeeds, also call .delete to avoid leaking memory. getTestMap().set(obj, obj).delete(obj);