7128 - React document listener memory leak fix. #7209
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
ghost
added
the
Done
The big mem leak test - unfortunately cant be unit tested, needs a human to use something like Chrome dev tools -> timeline and profile to take snapshots, add and remove a react UI and verify memory stays constant. Ive tried to add tests for the basic changes.
Ive documented the added non private funcs.
pass
My changes didnt introduce any new complaints. There are failures but they previously existed and are on files i have not touched.
Done. |
|
--My changes are very limited but i have gone thru and added some comments where i am unsure about style, supported browsers etc to help make things right.-- I am now using the map that tracks ids to components to know when the last component is removed. |
| if (remover && remover.remove) { | ||
| listenerRemovers.push(remover.remove); | ||
| } | ||
| }; |
There was a problem hiding this comment.
not sure if the names $trapBubbledEvent and $trapCapturedEvent should be different so theres no confusion about them being refs to the ones that live in ReactEventListener.
mP1
force-pushed
the
7128-unmount-last-component-document-reset
branch
from
89ce402
to
6c76f2f
Compare
|
I no longer use document.querySelectAll( the attribute that is used to mark react root nodes), and am instead using the map that tracks ids to components in ReactMount, to know when the last component was unmounted. I added tests for new apis for |
mP1
force-pushed
the
7128-unmount-last-component-document-reset
branch
from
6c76f2f
to
93d7c21
Compare
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| @@ -328,6 +369,14 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, { | |||
| isListening[dependency] = true; | |||
| } | |||
| } | |||
|
|
|||
| return function() { | |||
There was a problem hiding this comment.
Quickly reading this I'm having a little trouble understanding the exact flow, if I understand this correctly, this seems like a wasteful way of doing it. Allocating a new callback which holds an array of callbacks, instead of keeping a single global array of callbacks? Or am I missing something.
There was a problem hiding this comment.
This function can accept documents and other types of targets. In both these cases it returns a remover that removes all registered listeners. To give a consistent interface it must therefore always return a remover for all types of target.
In this case, the caller knows if its adding to a document, and it does the right thing with the returned remover.
Its simply trying to match the style that whoever registers a listener also takes care of removing them at the right time.
Regarding the wastefulness of creating an array for each call, it hardly matters, as this isnt called that frequently, actually it only happens once if your "root" is the document, or once for the other "targets", which again wont be that many.
|
Considering some listeners have side-effects, might it be a good idea to clean up listeners immediately whenever they are no longer used? (question for the team) |
|
NB: |
mP1
force-pushed
the
7128-unmount-last-component-document-reset
branch
2 times, most recently
from
49af1c4
to
e9495c5
Compare
|
Since React uses Jest for tests, you may actually be able to test for the presence of the global event listeners through a JSDOM escape hatch or test utility. |
|
Thanks for the PR! Sorry this didn't get proper attention. My impression is that while it could be nice to fix, it's a somewhat uncommon request, and adds quite a bit of complexity. I think the proper fix to this would be to do #2043 which we want eventually anyway. So I'll close this. |
This captures global (document) added event listeners, and saves them. When the last component is removed these "saved" listeners are also removed, and the object on document that keeps track of document state(mostly tracks which event listeners have already been added) is also "reset".