-
Notifications
You must be signed in to change notification settings - Fork 24.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Critical improvements for Map and Set polyfills. #21492
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Generated by 🚫 dangerJS |
Were the release notes supposed to be in some other file, or should I simply have formatted them differently in the PR description? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very clever! I like it.
I only have minor comments inline, but otherwise this looks great. Thanks for adding tests, too!
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
eslint
found some issues.eslint
found some issues.
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
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).
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).
@yungsters Ok, I think I've addressed your feedback. Moving the Still interested in your thoughts on #21492 (comment), though I'm fine with it (since it works) if you are! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Libraries/vendor/core/Map.js
Outdated
let index = nonExtensibleObjects.indexOf(o); | ||
if (index < 0) { | ||
index = nonExtensibleObjects.length; | ||
nonExtensibleObjects[index] = o; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be convinced that changing hashes doesn't matter in this case, but it worries me.
} 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
What's the status of this pull request? |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
flow
found some issues.
@@ -18,8 +18,10 @@ const {polyfillGlobal} = require('PolyfillFunctions'); | |||
*/ | |||
const _shouldPolyfillCollection = require('_shouldPolyfillES6Collection'); | |||
if (_shouldPolyfillCollection('Map')) { | |||
require('_wrapObjectFreezeAndFriends'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
polyfillGlobal('Map', () => require('Map')); | ||
} | ||
if (_shouldPolyfillCollection('Set')) { | ||
require('_wrapObjectFreezeAndFriends'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
flow
found some issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code analysis results:
flow
found some issues.
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)
Rebased and addressed feedback from @pvdz and @yungsters. It's still possible that the Apologies to @Format for the unintentional @-mention-spam. |
Looks like test_detox_end_to_end is failing on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
Summary: Pull Request resolved: facebook#21492 Differential Revision: D10288094 Pulled By: cpojer fbshipit-source-id: b1ca7344dda3043553be6945614b439a0f42a46a
Test Plan:
See included tests.
Release Notes:
Map
andSet
polyfills no longer reject non-extensible object keys, through a combination of tagging objects before they become non-extensible, and falling back to a linearArray#indexOf
lookup when there is no other option.Map
andSet
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
andSet
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 thereact-native
override field inpackage.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!