Prevent memory leak of PerformanceObserver

[gterzian]

#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 #24072 (comment)

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

servo/components/script/dom/performanceobserver.rs

Line 131 in 6297fa5

fn Disconnect(&self) {

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

servo/components/script/dom/performance.rs

Line 244 in 39bd455

if add_to_performance_entries_buffer {

And this happens for instance each time script does a fetch:

servo/components/script/network_listener.rs

Line 65 in 7490dd6

.queue_entry(performance_entry.upcast::<PerformanceEntry>(), true);

used for example by the stylesheet loader(used in the debug script that was leaking docs):

servo/components/script/stylesheet_loader.rs

Line 233 in 799490a

network_listener::submit_timing(self)

Itself called by fetch at

servo/components/net_traits/lib.rs

Line 260 in 9bba14c

listener.submit_resource_timing();

[gterzian]

Ok so this #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 #24074 (comment), 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.