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.

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.

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?

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.

(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.

Let me try to get back to this one next week...

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.

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.

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?

Probably not, but on a testcase like the one above each call keeps alive megs of memory indirectly so...

Jan, do you know who might know about console.log handling on devtools side and whether we could keep fewer objects alive?

Nicolas, could you please help here?

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.

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) {

(For the above. Probably should be done in a separate bug)

Using ownerdoc makes sense to me, and is in some sense even more correct than element.

Emilio, is there anything I could help with on DevTools side?

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.