console.log leaks objects passed to it until we hit memory pressure
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
People
(Reporter: jaffathecake, Unassigned)
References
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (X11; CrOS x86_64 14031.0.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/93.0.4543.0 Safari/537.36
Steps to reproduce:
const weakMap = new WeakMap();
button.onclick = () => {
const el = document.createElement('div');
weakMap.set(el, new Uint8Array(1000 * 1000 * 100));
new ResizeObserver(
() => console.log('resize', el.getBoundingClientRect())
).observe(el);
document.body.append(el);
setTimeout(() => el.remove(), 0);
}
Each click leaks an element. In this case that's 100mb.
Demo: https://static-misc-3.glitch.me/leaky-resize-observer/
The same happens with IntersectionObserver.
This seems unexpected because the element no longer has the means to resize or intersect, as it can no longer be reattached to the document. Also, MutationObserver and event listeners do not cause this sort of leak.
Comment 1•3 years ago
|
||
Hmm, not clear to me how this happens with IntersectionObserver
... The Document
doesn't keep them alive (it uses a weak pointer set here). The observer and the target are kept alive by each other, but they're cycle-collected properly afaict.
For ResizeObserver
however it does make sense, ResizeObserverController
keeps strong references to all observers that haven't been disconnected.
Updated•3 years ago
|
Comment 2•3 years ago
|
||
Ok, so ResizeObserver
is clearly bogus because I think we just keep the objects alive for too long. But for IntersectionObserver
I'm not quite sure what's going on. When I add some CC logging I see stuff like this:
0x7f919979ec00 [DOMIntersectionObserver]
Root 0x7f919979ec00 is a ref counted object with 0 unknown edge(s).
known edges:
0x7f9199603ca0 [FragmentOrElement (xhtml) div (orphan) https://static-misc-3.glitch.me/leaky-resize-observer/] --[]--> 0x7f919979ec00
It is an incremental root, which means it was touched during an incremental CC.
Which seems to me like we figured out we're basically only referenced by an orphan node.
When I check the logs for the element I see:
Parsing cclogs/cc-edges.74472-2.log. Done loading graph.
0x7f919979d1c0 [DOMRectReadOnly]
--[mParent]--> 0x7f9199603ca0 [FragmentOrElement (xhtml) div (orphan) https://static-misc-3.glitch.me/leaky-resize-observer/]
Root 0x7f919979d1c0 is a ref counted object with 1 unknown edge(s).
that DOMRect
is the return value of getBoundingClientRect
from the callback, but it seems we can't figure out the edge that comes from the callback until we run a full GC or something? On the last CC I see instead:
Didn't find a path.
known edges:
0x216e5563d4c0 [JS Object (HTMLDivElement)] --[UnwrapDOMObject(obj)]--> 0x7f9199603ca0
Which is the edge that we were missing before most likely... Do you know why we can't find this edge sooner Andrew? Is it just because we don't do a full GC soon enough? We do free the IntersectionObservers
if I click "Minimize memory usage" in about:memory... Are we incorrectly skipping tracing of some stuff or what not?
Comment 3•3 years ago
|
||
Does it just need a GC between CCs so that js objects stop being black?
But 'Root 0x7f919979d1c0 is a ref counted object with 1 unknown edge(s)' which disappears..
This is a runtime leak, so one could add a break point to the release and see who releases that unknown reference.
Comment 4•3 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)
Which seems to me like we figured out we're basically only referenced by an orphan node.
No, that's not the issue here. The key is the sentence "It is an incremental root, which means that the it [the DOMIntersectionObserver] was touched during an incremental CC." If an object is addrefed or released in the middle of an incremental cycle collection, then we never collect it, because the cycle collector's information about the object might be out-of-date. The fact that running "minimize memory", which runs a non-incremental CC, collects things also points at this.
This could happen if, say, somebody had a weak reference to every intersection observer, and on every frame was taking a strong stack ref to the observers, doing something with it, then releasing the reference. It looks like Document::mIntersectionObservers is a weak reference and Document::UpdateIntersectionObservations() takes a strong reference, so maybe there's an observer for a node that could otherwise be collected.
However, this might not be the whole story, because as you said the div element appears to also be keeping the intersection observer alive, via the DOMRectReadOnly.
Updated•3 years ago
|
Comment 6•3 years ago
|
||
So I think this is working as expected. The main difference between IntersectionObserver / ResizeObserver and e.g. MutationObserver in the test-case is that the callback actually runs, so the DOMRect created by calling getBoundingClientRect
keeps the element alive via its mParent
pointer.
In the click / mutation observer case, the callback doesn't run. The reason why "Minimize memory usage" helps is that the console listens for it and clears the values, allowing them to get collected.
So I think the bug here is that Console
might keep JS objects alive for the whole lifetime of the page.
Updated•3 years ago
|
Comment 7•3 years ago
|
||
Comment 8•3 years ago
•
|
||
I confirmed that without the console.log
(but still capturing the element, etc) both IntersectionObserver
and ResizeObserver
behave as expected, thus the title change above.
Comment 9•3 years ago
|
||
There is https://searchfox.org/mozilla-central/rev/c12a59323ee46b29b90c9917a3a7a70ea714ffec/dom/console/Console.cpp#2413-2416
Do we end up keeping up to 1000 entries alive?
Comment 10•3 years ago
|
||
Probably not, but on a testcase like the one above each call keeps alive megs of memory indirectly so...
Comment 11•3 years ago
|
||
Jan, do you know who might know about console.log handling on devtools side and whether we could keep fewer objects alive?
Comment 12•3 years ago
|
||
Nicolas, could you please help here?
Comment 13•3 years ago
|
||
Karl mentioned that we don't store up to 1000 events (we should clear stuff here), but then we pass that to ProcessCallData
, which ends up calling ConsoleAPIStorage.recordEvent()
, which ends up keeping stuff alive anyways. Fun.
Comment 14•3 years ago
|
||
Arguably other browsers sorta behave the same way. If I change my testcase above to log el
rather than el.getBoundingClientRect()
, then they also leak memory.
So in Chromium and WebKit the rect returned by GetBoundingClientRect
doesn't seem to keep the parent element alive... Olli, maybe we can just do something like this (plus similar bits in the various places that return DOMRect/DOMRectReadOnly/etc?). I think we only use them to find a relevant global so it should be fine given we expose them to JS instantly?
diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
index 9db1f6ec0351b..2e97ad69ec300 100644
--- a/dom/base/Element.cpp
+++ b/dom/base/Element.cpp
@@ -1029,7 +1029,7 @@ nsRect Element::GetClientAreaRect() {
}
already_AddRefed<DOMRect> Element::GetBoundingClientRect() {
- RefPtr<DOMRect> rect = new DOMRect(this);
+ RefPtr<DOMRect> rect = new DOMRect(ToSupports(OwnerDoc()));
nsIFrame* frame = GetPrimaryFrame(FlushType::Layout);
if (!frame) {
Comment 15•3 years ago
|
||
(For the above. Probably should be done in a separate bug)
Comment 16•3 years ago
|
||
Using ownerdoc makes sense to me, and is in some sense even more correct than element.
Comment 17•3 years ago
|
||
Emilio, is there anything I could help with on DevTools side?
Reporter | ||
Comment 18•3 years ago
|
||
Ah, sorry I misinterpreted what I was seeing in Firefox. Looks like the observer implementation was much better than Chrome & Firefox.
Fwiw, here's another test that doesn't log the element https://static-misc-3.glitch.me/leaky-resize-observer/no-el-ref.html. It shows that Firefox correctly GCs.
Description
•