free transient observers at end of microtask #512
Conversation
|
@arv mind taking a look? |
| // Run after all other end-of-microtask things, like observers, have | ||
| // had their chance to run. Use `setTimeout` to keep the test isolated | ||
| // from details of the MutationObserver system (setEndOfMicrotask | ||
| // internally relies on a native MutationObserver). |
There was a problem hiding this comment.
This is not the case in IE10 and Firefox has no support for microtasks. Maybe increase the timeout a to 4+ ms to ensure it gets scheduled later than the emulated MO callbacks?
There was a problem hiding this comment.
Ah good catch! I wonder if it's just my comment that's badly worded ... since the test is passing on IE10/Firefox. Maybe because setTimeout still gets added to the end of the pending callback queue? Or I'm getting lucky? :)
Here's what happened:
My first thought was to use setEndOfMicrotask. It works because internally MutationObservers.js uses the same setEndOfMicrotask, so its callback is scheduled before the one we schedule here (in the test). I liked that because it was a tight bound on the timing.
What stopped me from using it: the test would fail if you ran on native Shadow DOM+setEndOfMicrotask. Since setEndOfMicrotask uses a native mutation observer, and it only creates one observer very early on, its observer has a lower birth order than ours. That reverses the order of callbacks and the test fails. (if setEndOfMicrotask allocated a new MutationObserver each time, then the test would pass). I know we don't normally run these tests against native ShadowDOM, but I wanted to double check and it scared me that the test (seemed) to fail on native in my original setEndOfMicrotask version.
But based on your comment, I'm wondering if the setEndOfMicrotask version would be better?
Or maybe I could clarify this comment somehow?
|
LGTM |
jmesserly
force-pushed
the
fixTransientLeak
branch
from
44ff8da
to
f280dd5
Compare
|
in the meantime I like your suggestion of setTimeout(4), updated! |
free transient observers at end of microtask
Since we try to be lazy when scheduling observers, we need to make sure to schedule ones that have transient observers. Otherwise, we never free the transient observer, and it is stuck observing a disconnected tree, keeping the tree alive in memory.
(before #505, this issue was much less likely to manifest because if a change record matched any observer, all transient observers would be cleaned up. After that change, only those observers who matched the record would get their transients cleaned up.)
Thanks to @donny-dont for helping track down these leaks. I'm not to the bottom of all of them yet, but this seems like a step in the right direction.