Servo: Prevent memory leak of PerformanceObserver

[gterzian]

https://github.com/servo/servo/pull/24072 fixes the leak, but the underlying reason is still not clear.

It could be that if we didn't clear the performance list, then it ended up getting entries added to it but never removed, so if you quickly navigated away from the page it just sat there being a memory leak?

_Originally posted by @asajeffrey in https://github.com/servo/servo/pull/24072#issuecomment-525514036_

See also other comments in that PR.

[gterzian]

The theory of the Performance takes being kept in the task queue seems plausible.

cc @asajeffrey @jdm I think it is plausible, and also that it's not what happened in this specific case, and that instead we're leaking PerformanceObserver since they are never removed from Performance(which itself wasn't set to None either), unless script calls https://github.com/servo/servo/blob/6297fa582ac4c870e4a8d16cd261da6ede14191d/components/script/dom/performanceobserver.rs#L131

We could look into changing the structure of how observers are stored, so that they are only kept alive by their corresponding PerformanceObserverCallback in script(so they would drop if script unsets the callback, I think).

I need to look more into it.

It could look something like: we could separate the PerformanceObserver DOM object, from a PerformanceObserverImpl, with the latter keeping track of things like DOMPerformanceEntryList and other "data", whereas the DOM object would just be a wrapper around the callback.

Then the global could keep a strong ref to the Impl, and a Weak<PerformanceObserver> that would drop if the script callback is set to None.

Then the global could run a periodic checkpoint like is done for messageports at https://github.com/servo/servo/pull/23637/files#diff-59d233642d0ce6d687484bdd009e1017R484

[gterzian]

On the other hand, I'm not actually sure the code at https://gist.github.com/kanaka/119f5ed9841e23e35d07e8944cca6aa7 creates any observers, which could invalidate the above logic(at least with regards to the leaking perceived when running that script). Will require looking into more closely.

[gterzian]

Ah ok, even if there are no observers set, we still often buffer the entry at https://github.com/servo/servo/blob/39bd45529d6ef3eea8e0eeef77d0294e0db1b02c/components/script/dom/performance.rs#L244

And this happens for instance each time script does a fetch: https://github.com/servo/servo/blob/7490dd6f0deafc8192126a88a8a6f75c100e5d71/components/script/network_listener.rs#L65

used for example by the stylesheet loader(used in the debug script that was leaking docs): https://github.com/servo/servo/blob/799490a02e9bea575bf34c39f045ef0883539f05/components/script/stylesheet_loader.rs#L233

Itself called by fetch at https://github.com/servo/servo/blob/9bba14cb438cf5ac08945bb8fbb5a0f31d43fcbe/components/net_traits/lib.rs#L260

[gterzian]

Ok so this https://github.com/servo/servo/pull/24109 fixes the actual cause of the leak, and removes the previous changes that set Performance as a whole to None.

The cause of the perceived leak was indeed the buffer, as no performance observers were created.

Leaving this one open, since I think we should still fix the issue with the performance observers, which leak unless script calls Disconnect on them. See https://github.com/servo/servo/issues/24074#issuecomment-526026684, the fix for that would be finding a way to store the observers while allowing them to drop when script isn't keeping a reference to their callback.