1596992 - ResizeObserver Memory Leak

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Matthew Dapena-Tretter]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/78.0.3904.97 Safari/537.36

Steps to reproduce:

The code explains it best:

class LeakyThing {
  _memoryWaster = new ArrayBuffer(10000000);
  constructor() {
    new ResizeObserver(this._handleResize);
  }
  _handleResize = () => {
    this;
  };
}

for (let i = 0; i < 50; i++) {
  new LeakyThing();
}

https://codepen.io/matthewwithanm/pen/jOOQZRj

Actual results:

It seems like cyclical references stemming from ResizeObservers aren't cleaned up. If you force a gc (via about:memory) and then take a snapshot, you'll see the ArrayBuffers are all retained.

Expected results:

They should all be garbage collected.

Comment 1

[Matthew Dapena-Tretter]

I submitted the bug via Chrome (sorry 😝) but I've reproed on FF 70.0.1 ("Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:70.0) Gecko/20100101 Firefox/70.0").

Comment 2

[Matthew Dapena-Tretter]

Sorry, doesn't have anything to do with cyclical references. ResizeObservers in general just aren't gc'd apparently.

for (let i = 0; i < 50; i++) {
  const ab = new ArrayBuffer(10000000);
  const ro = new ResizeObserver(() => {});
  ro.x = ab;
}

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I can reproduce.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Well, all live ResizeObservers are kept alive for the lifetime of the document in ResizeObserverController: https://searchfox.org/mozilla-central/rev/131338e5017bc0283d86fb73844407b9a2155c98/dom/base/ResizeObserverController.cpp#85

So it's not a leak per se, but we should probably not keep them around for so long, somehow.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Well, that array is append-only, that's silly.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Taking a look...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

This is mostly per spec, see https://github.com/w3c/csswg-drafts/issues/4518. However it seems other browsers don't follow the spec, so I'll align Firefox with WebKit for now, I think.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1596992 - Avoid leaking ResizeObserver entries without active observations for the lifetime of the document. r=smaug

By adding / removing to the observer list on observe() / unobserve()
respectively, rather than only adding on construction per spec.

See https://github.com/w3c/csswg-drafts/issues/4518 for the relevant spec issue.

Comment 9

[Andrew McCreight [:mccr8]]

Thanks for fixing this, Emilio. It would also be nice to have the memory used by the resize observer controller reported along with the document.

Comment 10

[Aleks Totic]

ResizeObservers do not have to be kept alive as long as the document.

The answer to "how long should the ResizeObserver be kept alive?" is surprisingly convoluted.

ResizeObserver must stay alive as long as either:
A) there is a js reference to observer.
B) there is an active observation. Specifically, there is an element, reachable from JS, that is being observed.

A) is easy, ResizeObserver gets kept alive by standard Javascript object lifetime
B) is bit more complex. RO is being kept alive by a reference from an Element.

In Chrome, this is implemented by Element having a pointer to all ResizeObservers observing it. These pointers keep ResizeObserver alive.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Aleks Totic from comment #10)

ResizeObservers do not have to be kept alive as long as the document.

Well, not necessarily, but per spec you need to preserve order of construction, which means that you need an array of all the observers that are still created and alive.

That array can be an array of weak pointers of sorts, but it is still an append-only array...

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d36b54709a8
Avoid leaking ResizeObserver entries without active observations for the lifetime of the document. r=smaug

Comment 13

[Aleks Totic]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

(In reply to Aleks Totic from comment #10)

ResizeObservers do not have to be kept alive as long as the document.

Well, not necessarily, but per spec you need to preserve order of construction, which means that you need an array of all the observers that are still created and alive.

That array can be an array of weak pointers of sorts, but it is still an append-only array...

I do not understand the append-only requirement. Observers get removed if:

• there are no more live Elements being observed.
• there are no references to the observer from JS.

Comment 14

[Matthew Dapena-Tretter]

I do not understand the append-only requirement.

I think the point is that, if you remove them, you lose track of the order of construction.

Comment 15

[Aleks Totic]

I think the point is that, if you remove them, you lose track of the order of construction.

If observer is removed, it will never deliver any observations again.
Order of construction is no longer relevant.

Comment 16

[Matthew Dapena-Tretter]

If observer is removed, it will never deliver any observations again.
Order of construction is no longer relevant.

I may be misunderstanding something, but you can always call observe() on it again, causing it to deliver observations again.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Aleks Totic from comment #13)

I do not understand the append-only requirement. Observers get removed if:

• there are no more live Elements being observed.
• there are no references to the observer from JS.

Yeah, Aleks is right in the sense that if you keep the list as an array of weak pointers, you can clear the entry when the element is destroyed or something.

In terms of blink code, what I mean is that there are only insert() calls into this hash set, nothing ever gets removed from there.

You could do it from the ResizeObserver destructor, I guess... But it's unclear to me how Blink guarantees any sort of sane ordering with a HashSet.

Comment 18

[Aleks Totic]

there are only insert() calls into this hash set, nothing ever gets removed from there.

Blink has garbage collection, so weak members of that set get removed automatically by the GC.

t's unclear to me how Blink guarantees any sort of sane ordering with a HashSet

Blink's collections are an undocumented zoo. I think (would check docs if there were any) that HashSet is a special type of Map whose keys are kept sorted in insertion order.

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4d36b54709a8

Comment 20

[Emilio Cobos Álvarez (:emilio)]

(In reply to Aleks Totic from comment #18)

Blink's collections are an undocumented zoo. I think (would check docs if there were any) that HashSet is a special type of Map whose keys are kept sorted in insertion order.

That doesn't match what I see:

<!doctype html>
<style>
  div {
    width: 100px;
    height: 100px;
    background: green;
  }
</style>
<div id="observeme"></div>
<script>
  let div = document.querySelector("#observeme");
  for (let i = 0; i < 100; ++i) {
    new ResizeObserver(function() {
      console.log(i);
    }).observe(div);
  }
</script>

Per spec, this test-case should enumerate the numbers from 0 to 99. In Blink the order is rather random.