Critical improvements for Map and Set polyfills. #21492

benjamn · 2018-10-04T22:42:16Z

Test Plan:

See included tests.

Release Notes:

[POLYFILL] [SPEC] The Map and Set polyfills no longer reject non-extensible object keys, through a combination of tagging objects before they become non-extensible, and falling back to a linear Array#indexOf lookup when there is no other option.
[POLYFILL] [BUGFIX] Fixed a critical bug that caused Map and Set object hashes to collide with the hashes of other objects in their prototype chains.

Details

When I first discovered these bugs in the course of investigating apollographql/react-apollo#2442, I was disappointed and disheartened, and I was tempted to recommend that Apollo developers find a replacement for the React Native Map and Set polyfills.

However, as I dug into the code and investigated alternative polyfills, I realized that this Map polyfill has the potential to be one of the fastest and best polyfills available, since almost every other polyfill avoids tagging object keys, and instead resorts to linear lookup. I hope this PR demonstrates that we can provide constant-time lookup in (almost) all cases, even for frozen object keys.

While I can certainly provide more creative solutions to developers who use my libraries, from monkey-patching Map.prototype to using the react-native override field in package.json, I think the best solution is simply to improve the official polyfills, which is what I've tried to do in this PR.

Please read the commit messages and the comments that I've added to the code for more explanation, and thanks for your consideration!

analysis-bot

Code analysis results:

eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.
eslint found some issues.

Libraries/Core/__tests__/MapAndSetPolyfills-test.js

pull-bot · 2018-10-04T22:43:49Z

	Warnings
⚠️	📋 Changelog - This PR appears to be missing Changelog. Please add a section called "changelog" and format it as explained in the contributing guidelines.

Generated by 🚫 dangerJS

benjamn · 2018-10-04T23:17:28Z

Were the release notes supposed to be in some other file, or should I simply have formatted them differently in the PR description?

yungsters

This is very clever! I like it.

I only have minor comments inline, but otherwise this looks great. Thanks for adding tests, too!

Libraries/Core/InitializeCore.js

yungsters · 2018-10-05T02:51:17Z

Libraries/vendor/core/Map.js

@@ -39,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:


hramos · 2018-10-05T04:07:10Z

re: release notes, you did nothing wrong. The bot as currently implemented will only recognize categories from the list provided by the PULL_REQUEST_TEMPLATE (e.g. CLI, GENERAL, IOS, ...), and POLYFILL is not one of those. We should just relax this check, thanks for pointing it out.

I also noticed it's been too verbose with its eslint nits, that can be fixed. (edit: fix at #21503)

analysis-bot

Code analysis results:

eslint found some issues.
eslint found some issues.

Libraries/Core/__tests__/MapAndSetPolyfills-test.js

Libraries/Core/InitializeCore.js

Summary: Fixes issue where the bot would leave multiple lines in the top review comment if, say, eslint found 30 issues, there would be 30 mentions of eslint having found issues. See #21492 for an example of such a case, where `analysis-bot` left a spammy review at #21492 (review). Pull Request resolved: #21503 Differential Revision: D10219439 Pulled By: hramos fbshipit-source-id: 75d32ef3bfeaa91ab614763a19494659ad1be0dd

@peggyrayzis

This temporarily mitigates the prototype chain bug that will be fixed with this React Native PR that I submitted yesterday: facebook/react-native#21492 I would like to find a way to bundle a different Map polyfill in React Native apps, without increasing bundle sizes for non-RN apps, but that has proven tricky so far. For future reference, this seems to be the way to do it (thanks to @peggyrayzis for the tip): https://facebook.github.io/react-native/docs/platform-specific-code#platform-specific-extensions Until a new version of React Native is released with these changes included, the simplest way to fix the problem is simply to avoid storing any prototype objects in a Map, so that's what I've done in this commit. We are already wrapping Object.freeze and friends in src/fixPolyfills.ts, which should make it possible to use frozen objects as Map keys (the other bug that I addressed in the React Native PR linked above).

@peggyrayzis

Fixes apollographql/react-apollo#2442. This temporarily mitigates the prototype chain bug that will be permanently fixed by this React Native PR that I submitted yesterday: facebook/react-native#21492 I would like to find a way to bundle a different Map polyfill in React Native apps, without increasing bundle sizes for non-RN apps, but that has proven tricky so far. For future reference, this seems to be the way to do it (thanks to @peggyrayzis for the tip): https://facebook.github.io/react-native/docs/platform-specific-code#platform-specific-extensions Until a new version of React Native is released with these changes included, the simplest way to fix the problem is simply to avoid storing any prototype objects in a Map, so that's what I've done in this commit. We are already wrapping Object.freeze and friends in src/fixPolyfills.ts, which should make it possible to use frozen objects as Map keys (the other bug that I addressed in the React Native PR linked above).

benjamn · 2018-10-08T14:21:26Z

@yungsters Ok, I think I've addressed your feedback. Moving the Object.freeze wrapping logic into its own module was a great idea, since it means we can safely import that module in multiple places (first in InitializeCore.js and then in Map.js, just to be sure it gets imported if the polyfill is used).

Still interested in your thoughts on #21492 (comment), though I'm fine with it (since it works) if you are!

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js

facebook-github-bot

yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

pvdz · 2018-10-11T07:50:27Z

Libraries/vendor/core/Map.js

+        let index = nonExtensibleObjects.indexOf(o);
+        if (index < 0) {
+          index = nonExtensibleObjects.length;
+          nonExtensibleObjects[index] = o;


I'm slightly worried about a memory leak here. I think it's fine since the temporary arrays should not be retained once the map is released, but I'm not certain.

I don't think enough objects will ever be added to this array to matter from a memory perspective, and I'm worried that removing and re-adding objects will give them different hashes over time. Since these hashes are just made-up (not based on the memory address of the object), and these objects are non-extensible, there's no way to give them the same hash every time without keeping them in memory somewhere.

I could be convinced that changing hashes doesn't matter in this case, but it worries me.

pvdz · 2018-10-11T07:56:03Z

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js

+      } 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.


... with the potential of leaking memory, unreported, if the error happens in the .delete() portion of things.

Hypothetically; if something wants to call freeze or seal on an object, how likely is it that it actually wouldn't want the __MAP_POLYFILL_INTERNAL_HASH__ expando applied to the object itself, in the first place? Wouldn't we be better and safer off letting it use the (new-in-this-PR) fallback method instead?

Wouldn't we be better and safer off letting it use the (new-in-this-PR) fallback method instead?

No, because the fallback method has linear lookup time, compared to the tagging approach, which has constant lookup time. That's a nice advantage of this polyfill over other (better tested, more widely used) polyfills that settle for linear time in order to avoid tagging objects. If we don't care about the performance benefit of tagging, then we should really replace this whole polyfill with one that wasn't invented here.

I'm confident that .delete(obj) will always succeed if .set(obj, obj) succeeds in putting obj into the map. If that invariant does not hold, there's a bug, and we will fix it. And if .set(obj, obj) fails to put obj into the map, then there won't be any memory leak, whether or not .delete(obj) is called/succeeds.

Libraries/vendor/core/Map.js

yungsters · 2018-11-27T05:55:38Z

What's the status of this pull request?

cpojer · 2018-12-05T02:57:07Z

Also asking, @benjamn mind responding to the latest comments (adding @Format)?

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.

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! 💀🎃👻

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.

@pvdz

Thanks to @pvdz for this suggestion: https://github.com/facebook/react-native/pull/21492/files#r224349711

analysis-bot

Code analysis results:

flow found some issues.

analysis-bot · 2019-01-26T17:15:04Z

Libraries/Core/polyfillES6Collections.js

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


Importing from an untyped module makes it any and is not safe! Did you mean to add // @flow to the top of _wrapObjectFreezeAndFriends? (untyped-import)

analysis-bot · 2019-01-26T17:15:05Z

Libraries/Core/polyfillES6Collections.js

  polyfillGlobal('Map', () => require('Map'));
 }
 if (_shouldPolyfillCollection('Set')) {
+  require('_wrapObjectFreezeAndFriends');


Importing from an untyped module makes it any and is not safe! Did you mean to add // @flow to the top of _wrapObjectFreezeAndFriends? (untyped-import)

analysis-bot

Code analysis results:

flow found some issues.

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js

analysis-bot

Code analysis results:

flow found some issues.

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js

@yungsters

The `@format` directive was requested by @yungsters: https://github.com/facebook/react-native/pull/21492/files#r223949980 And the `@flow` directive by @analysis-bot: #21492 (comment)

benjamn · 2019-01-26T17:31:14Z

Rebased and addressed feedback from @pvdz and @yungsters.

It's still possible that the nonExtensibleObjects array will retain objects that were frozen before _wrapObjectFreezeAndFriends.js was imported, but those objects should be exceedingly rare because the wrapping logic begins very early in the setup of the environment, and the nonExtensibleObjects array will be freed whenever the Map is garbage collected.

Apologies to @Format for the unintentional @-mention-spam.

benjamn · 2019-01-26T17:37:44Z

Looks like test_detox_end_to_end is failing on master currently, in case that wasn't clear.

facebook-github-bot

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cpojer

Thanks for updating the PR. I'll land it but also hope that we'll be able to remove the polyfills entirely in the near future :)

react-native-bot · 2019-01-28T09:55:05Z

@benjamn merged commit 90850ca into facebook:master.

Summary: Pull Request resolved: facebook#21492 Differential Revision: D10288094 Pulled By: cpojer fbshipit-source-id: b1ca7344dda3043553be6945614b439a0f42a46a

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 4, 2018

analysis-bot suggested changes Oct 4, 2018

View reviewed changes

react-native-bot added the Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. label Oct 4, 2018

benjamn mentioned this pull request Oct 4, 2018

GraphQL query throws 'QueryKeyMaker cannot handle circular query structures' with simple React Native setup apollographql/react-apollo#2442

Closed

yungsters requested changes Oct 5, 2018

View reviewed changes

hramos added ✅Release Notes and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Oct 5, 2018

hramos mentioned this pull request Oct 5, 2018

[Internal] [Code Review] Flag code analysis issues once per converter used #21503

Closed

analysis-bot suggested changes Oct 5, 2018

View reviewed changes

Libraries/Core/__tests__/MapAndSetPolyfills-test.js Show resolved Hide resolved

Libraries/Core/__tests__/MapAndSetPolyfills-test.js Show resolved Hide resolved

benjamn commented Oct 5, 2018

View reviewed changes

Libraries/Core/InitializeCore.js Outdated Show resolved Hide resolved

yungsters reviewed Oct 10, 2018

View reviewed changes

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js Show resolved Hide resolved

facebook-github-bot reviewed Oct 10, 2018

View reviewed changes

pvdz reviewed Oct 11, 2018

View reviewed changes

Libraries/vendor/core/Map.js Outdated Show resolved Hide resolved

benjamn added 4 commits January 26, 2019 12:03

Fall through to nonExtensibleObjects.indexOf when all else fails.

139b223

Thanks to @pvdz for this suggestion: https://github.com/facebook/react-native/pull/21492/files#r224349711

analysis-bot suggested changes Jan 26, 2019

View reviewed changes

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js Outdated Show resolved Hide resolved

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js Outdated Show resolved Hide resolved

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js Outdated Show resolved Hide resolved

analysis-bot suggested changes Jan 26, 2019

View reviewed changes

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js Outdated Show resolved Hide resolved

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js Outdated Show resolved Hide resolved

Libraries/vendor/core/_wrapObjectFreezeAndFriends.js Outdated Show resolved Hide resolved

Add @format and @flow directives to _wrapObjectFreezeAndFriends.js.

9dbbd18

The `@format` directive was requested by @yungsters: https://github.com/facebook/react-native/pull/21492/files#r223949980 And the `@flow` directive by @analysis-bot: #21492 (comment)

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 28, 2019

facebook-github-bot approved these changes Jan 28, 2019

View reviewed changes

cpojer approved these changes Jan 28, 2019

View reviewed changes

facebook-github-bot closed this in 90850ca Jan 28, 2019

facebook locked as resolved and limited conversation to collaborators Jan 28, 2019

react-native-bot added the Merged This PR has been merged. label Jan 28, 2019

benjamn deleted the fix-map-and-set-polyfills branch February 4, 2019 19:58

hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Critical improvements for Map and Set polyfills. #21492

Critical improvements for Map and Set polyfills. #21492

benjamn commented Oct 4, 2018

analysis-bot left a comment

pull-bot commented Oct 4, 2018 •

edited

Loading

benjamn commented Oct 4, 2018

yungsters left a comment

yungsters Oct 5, 2018

hramos commented Oct 5, 2018 •

edited

Loading

analysis-bot left a comment

benjamn commented Oct 8, 2018

facebook-github-bot left a comment

pvdz Oct 11, 2018

benjamn Oct 11, 2018

benjamn Oct 11, 2018

pvdz Oct 11, 2018

benjamn Oct 11, 2018

benjamn Oct 11, 2018 •

edited

Loading

yungsters commented Nov 27, 2018

cpojer commented Dec 5, 2018

analysis-bot left a comment

analysis-bot Jan 26, 2019

analysis-bot Jan 26, 2019

analysis-bot left a comment

analysis-bot left a comment

benjamn commented Jan 26, 2019 •

edited

Loading

benjamn commented Jan 26, 2019

facebook-github-bot left a comment

cpojer left a comment

react-native-bot commented Jan 28, 2019

Critical improvements for Map and Set polyfills. #21492

Critical improvements for Map and Set polyfills. #21492

Conversation

benjamn commented Oct 4, 2018

Test Plan:

Release Notes:

Details

analysis-bot left a comment

Choose a reason for hiding this comment

pull-bot commented Oct 4, 2018 • edited Loading

benjamn commented Oct 4, 2018

yungsters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hramos commented Oct 5, 2018 • edited Loading

analysis-bot left a comment

Choose a reason for hiding this comment

benjamn commented Oct 8, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamn Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

yungsters commented Nov 27, 2018

cpojer commented Dec 5, 2018

analysis-bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

analysis-bot left a comment

Choose a reason for hiding this comment

benjamn commented Jan 26, 2019 • edited Loading

benjamn commented Jan 26, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

cpojer left a comment

Choose a reason for hiding this comment

react-native-bot commented Jan 28, 2019

pull-bot commented Oct 4, 2018 •

edited

Loading

hramos commented Oct 5, 2018 •

edited

Loading

benjamn Oct 11, 2018 •

edited

Loading

benjamn commented Jan 26, 2019 •

edited

Loading