Hold on to the lastNativeEvent weakly so as not to retain fibers belonging to long-ago unmounted DOM nodes #17582
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e26761e:
|
| return [leave]; | ||
| } | ||
| lastNativeEvent = nativeEvent; | ||
| if (seenNativeEvents instanceof Set) { | ||
| seenNativeEvents.clear(); |
There was a problem hiding this comment.
This replicates the ‘overwrite’ behavior of lastNativeEvent = nativeEvent for clients that don't support WeakSet.
| lastNativeEvent = nativeEvent; | ||
| if (seenNativeEvents instanceof Set) { | ||
| seenNativeEvents.clear(); | ||
| } |
There was a problem hiding this comment.
Maybe:
seenNativeEvents = typeof WeakSet === 'function' ? new WeakSet() : new Set();
There was a problem hiding this comment.
When WeakSet is supported then is there any harm to letting the set ‘grow’ infinitely? Something's presence in the set will be contingent on it being held on to somewhere else in your app, right?
When WeakSet is not supported, I wonder if it's more efficient to call clear() rather than to construct a new Set.
There was a problem hiding this comment.
There's a small one. If you're leaking the thing you're holding a weak reference to, or retaining them on purpose, then this set is growing. If we only care about the last thing it's best to only track the last thing. It's unfortunate you can't simply clear a WeakSet.
…nging to long-ago unmounted DOM nodes. Addresses #17580.
| if (nativeEvent === lastNativeEvent) { | ||
| lastNativeEvent = null; | ||
| if (seenNativeEvents.has(nativeEvent)) { | ||
| if (seenNativeEvents instanceof Set) { |
There was a problem hiding this comment.
Can we get away with not performing the instanceof check each time? We should know at the point seenNativeEvents is defined whether or not we're working with a Set or WeakSet
|
In generally we use expandos instead of weaksets as they've had better perf characteristics and doesn't require fallbacks implementations. E.g. However, in this case, we really shouldn't need to mess with the native events. It's just poorly structured code. One solution would be to just pass a boolean or eventSystemFlags here that signifies that this is the first processed ancestor tree. |
|
@sebmarkbage That's exactly what I did in my follow up PR! I noticed you added the flags to the EventPlugins, which made this fix slightly more obvious. Follow up PR: #17585. Thank you @steveluscher for taking the time to investigate this! |
|
Yay! Thanks @trueadm! |
The event that we store in
lastNativeEventhas a reference to its related DOM nodes which has a reference to someFiberNodeinstances. When a component unmounts we don't want to retain any of that.This PR stores the
lastNativeEventsthat we've seen in aWeakSetso as not to cause them to be retained. WhereWeakSetis not supported, we use a shim that does what the old thing used to do (and leaks like it too).Here's a snapshot of one of these leaks in the Chrome Memory profiler.
tkisFiberNodeandtlislastNativeEvent.This aims to address #17580. Credit for the idea to use
WeakSetgoes to @bgirard!