Bug 215158 – ResizeObserver appears to not be properly garbage collected

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Clark Pan]

Created attachment 405989 [details]
Test file to reproduce ResizeObserver memory leak

ResizeObserver appears not to be properly garbage collected.

I'm unsure if this is a real problem, or simply a problem within the Web Inspector.

Steps to reproduce:

- Open provided example file in Safari (Tested on version Version 13.1.1 (15609.2.9.1.2))
- Open up Web Inspector -> Timelines -> Javascript Allocations
- Take a snapshot
- Wait a minute
- Take another snapshot
- Compare the two snapshots

Expected
- Only a single count of ResizeObserver and HTMLDivElement should be present

Actual
- None of the used objects seem to be garbage collected.

Notes: I've tried the same test in Chrome and Firefox, and they both behave correctly.
These test makes the assumption that garbage collection is run every time I take a heap snapshot (not true in Firefox).

[Radar WebKit Bug Importer]

<rdar://problem/66608245>

[Simon Fraser (smfr)]

I can reproduce.

[Alexey Shvayka]

This might require a tweak similar to r233053.

[cathiechen]

Thanks! I can reproduce it.
I think I can work on it in this week or next week.

[Clark Pan]

Thanks for looking at this guys. Is there any more movement on it?

[cathiechen]

Sorry for the delay.

It seems the snapshot do destroy JSResizeObserver. See

```
#0	0x00000004174e696c in WebCore::JSResizeObserverOwner::finalize(JSC::Handle<JSC::Unknown>, void*) at /Users/cathiechen/cc/source/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/JSResizeObserver.cpp:334
#1	0x0000000435efb44a in JSC::WeakBlock::finalize(JSC::WeakImpl*) at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/WeakSetInlines.h:53
#2	0x0000000435efb4e4 in JSC::WeakBlock::sweep() at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/WeakBlock.cpp:87
#3	0x0000000435efbfd2 in JSC::WeakSet::sweep() at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/WeakSet.cpp:54
#4	0x0000000435ed134a in JSC::MarkedBlock::Handle::sweep(JSC::FreeList*) at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/MarkedBlock.cpp:391
#5	0x0000000435e39a04 in JSC::BlockDirectory::sweep()::$_7::operator()(unsigned long) const at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/BlockDirectory.cpp:280
#6	0x0000000435e3113c in void WTF::FastBitVectorImpl<JSC::BlockDirectoryBits::BlockDirectoryBitVectorWordView<(JSC::BlockDirectoryBits::Kind)6> >::forEachSetBit<JSC::BlockDirectory::sweep()::$_7>(JSC::BlockDirectory::sweep()::$_7 const&) const at /Users/cathiechen/cc/source/WebKit/WebKitBuild/Debug/usr/local/include/wtf/FastBitVector.h:355
#7	0x0000000435e310b6 in JSC::BlockDirectory::sweep() at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/BlockDirectory.cpp:277
#8	0x0000000435ee32e9 in JSC::MarkedSpace::sweepBlocks()::$_9::operator()(JSC::BlockDirectory&) const at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/MarkedSpace.cpp:222
#9	0x0000000435ed3f0e in void JSC::MarkedSpace::forEachDirectory<JSC::MarkedSpace::sweepBlocks()::$_9>(JSC::MarkedSpace::sweepBlocks()::$_9 const&) at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/MarkedSpace.h:241
#10	0x0000000435ed3ec9 in JSC::MarkedSpace::sweepBlocks() at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/MarkedSpace.cpp:220
#11	0x0000000435e52f38 in JSC::Heap::sweepSynchronously() at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/Heap.cpp:1048
#12	0x0000000435e53494 in JSC::Heap::collectNow(JSC::Synchronousness, JSC::GCRequest) at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/Heap.cpp:1091
#13	0x0000000435e96fec in JSC::HeapSnapshotBuilder::buildSnapshot() at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/heap/HeapSnapshotBuilder.cpp:73
#14	0x000000043600aa0f in Inspector::InspectorHeapAgent::snapshot() at /Users/cathiechen/cc/source/WebKit/Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:105
```

I'll check if the Snapshot data is correct later.

@Clark do you meet other problems besides the number of resizeObserver?

[cathiechen]

OK, snapshot does not sweep JSResizeObserver every time.

[Clark Pan]

The number of ResizeObserver is not the issue per se; the real issue is what is leaked via the functions scopes of the passed in handler. In a single page app environment, any visual component that wants to use ResizeObserver will be leaking its entire DOM tree when the component is removed.

I've worked around this issue in our codebase by providing a proxy implement for ResizeObserver, which nulls out the handler when I remove the component in question. It would be nice to not need this hack though, since it requires developers to know to call `disconnect` when the ResizeObserver is no longer used (which is not strictly needed, since if you have matching observe/unobserve calls, it should be sufficient based on the spec).

[cathiechen]

Created attachment 411961 [details]
Patch

[cathiechen]

(In reply to Clark Pan from comment #8)
> The number of ResizeObserver is not the issue per se; the real issue is what
> is leaked via the functions scopes of the passed in handler. In a single
> page app environment, any visual component that wants to use ResizeObserver
> will be leaking its entire DOM tree when the component is removed.

IIUC, in the test, `handler` refers to `observer`, for `() => {}`.
Because the resize observer callback can not be garbage collected and there's always refers from callback, so observer can not be garbage collected. It will be released until the page is unloaded.

[cathiechen]

(In reply to Clark Pan from comment #8)
> The number of ResizeObserver is not the issue per se; the real issue is what
> is leaked via the functions scopes of the passed in handler. In a single
> page app environment, any visual component that wants to use ResizeObserver
> will be leaking its entire DOM tree when the component is removed.

IIUC, in the test, `handler` refers to `observer`, for `() => {}`.
Because the resize observer callback can not be garbage collected and there's always refers from callback, so observer can not be garbage collected. It will be released until the page is unloaded.

[Clark Pan]

(In reply to cathiechen from comment #11)
> (In reply to Clark Pan from comment #8)
> > The number of ResizeObserver is not the issue per se; the real issue is what
> > is leaked via the functions scopes of the passed in handler. In a single
> > page app environment, any visual component that wants to use ResizeObserver
> > will be leaking its entire DOM tree when the component is removed.
> 
> IIUC, in the test, `handler` refers to `observer`, for `() => {}`.
> Because the resize observer callback can not be garbage collected and
> there's always refers from callback, so observer can not be garbage
> collected. It will be released until the page is unloaded.

Your understanding is correct.

In addition, the point I was trying to make is that any other variables that are referenced by the `handler` is also not garbage collected.

In the example provided, `div` is also used by the `handler`. You can see that, within the `destructor` thats returned by the `create` function, that I'm removing the `div` from the `container`, which should mark it available for garbage collection.

However, because `handler` is not garbage collected, the `div` is still held via `handler`'s function scope. That means the `div` will be leaked as well.

In a live application, this represents a rather large memory leak.

[cathiechen]

(In reply to Clark Pan from comment #12)
> 
> In addition, the point I was trying to make is that any other variables that
> are referenced by the `handler` is also not garbage collected.
> 
> In the example provided, `div` is also used by the `handler`. You can see
> that, within the `destructor` thats returned by the `create` function, that
> I'm removing the `div` from the `container`, which should mark it available
> for garbage collection.
> 
> However, because `handler` is not garbage collected, the `div` is still held
> via `handler`'s function scope. That means the `div` will be leaked as well.
> 
> In a live application, this represents a rather large memory leak.

Yeah, that's right.
Like Comment#3, in the patch the callback is IsWeakCallback, should be able to be gc.

[Frédéric Wang (:fredw)]

Comment on attachment 411961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411961&action=review

> Source/WebCore/ChangeLog:8
> +        If ResizeObserver is referenced inside ResizeObserverCallback, they are not garbage collected properly. To fix this,

If a ResizeObserver ... it is not garbage...

or

ResizeObservers ... they are not

> LayoutTests/resize-observer/resize-observer-callback-leak.html:11
> +function test0() {

can you find a better name than "test0"?

> LayoutTests/resize-observer/resize-observer-callback-leak.html:30
> +        }

I don't understand  this handler variable? Is it duplicating the handler below?

[cathiechen]

Comment on attachment 411961 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411961&action=review

Hi Fred,
Thanks for the review:)

>> Source/WebCore/ChangeLog:8
>> +        If ResizeObserver is referenced inside ResizeObserverCallback, they are not garbage collected properly. To fix this,
> 
> If a ResizeObserver ... it is not garbage...
> 
> or
> 
> ResizeObservers ... they are not

Done, thanks

>> LayoutTests/resize-observer/resize-observer-callback-leak.html:11
>> +function test0() {
> 
> can you find a better name than "test0"?

Done, changed it to accessToObserverInCallback

>> LayoutTests/resize-observer/resize-observer-callback-leak.html:30
>> +        }
> 
> I don't understand  this handler variable? Is it duplicating the handler below?

Ah, yes, I should remove it! Thanks!

[cathiechen]

Created attachment 411999 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 411999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411999&action=review

> Source/WebCore/bindings/js/JSResizeObserverCustom.cpp:36
> +    if (wrapped().callback())
> +        wrapped().callback()->visitJSFunction(visitor);

This is not safe. This function runs concurrently to the main thread.
After calling callback, the callback may be set to nullptr in the main thread before visitJSFunction is called.
Store callback to a raw pointer first.

> Source/WebCore/page/ResizeObserver.h:68
> +    ResizeObserverCallback* callback() { return m_callback.get(); }
> +

Let's call this callbackConcurrently() to signify the fact we can't mutate anything ref count, etc...

> LayoutTests/resize-observer/resize-observer-callback-leak.html:27
> +                        assert_true(observerCount < iterationCount, 'Resize observers should be collected.');

Don't assume that the number of resize observers is 0 at the beginning of the test.
There might be other resize observers created for other tests that ran before this test.
Instead, record the number of resize observes at the beginning of the test, and subtract the current number.

> LayoutTests/resize-observer/resize-observer-callback-leak.html:30
> +                }, 1000);

What is this step_timeout about? Why is 1s sufficient?
On bots, we probably want a longer timeout.

[cathiechen]

Comment on attachment 411999 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=411999&action=review

Hi Ryosuke,
Thanks for the review:)

>> Source/WebCore/bindings/js/JSResizeObserverCustom.cpp:36
>> +        wrapped().callback()->visitJSFunction(visitor);
> 
> This is not safe. This function runs concurrently to the main thread.
> After calling callback, the callback may be set to nullptr in the main thread before visitJSFunction is called.
> Store callback to a raw pointer first.

Done, thanks!

>> Source/WebCore/page/ResizeObserver.h:68
>> +
> 
> Let's call this callbackConcurrently() to signify the fact we can't mutate anything ref count, etc...

Done

>> LayoutTests/resize-observer/resize-observer-callback-leak.html:27
>> +                        assert_true(observerCount < iterationCount, 'Resize observers should be collected.');
> 
> Don't assume that the number of resize observers is 0 at the beginning of the test.
> There might be other resize observers created for other tests that ran before this test.
> Instead, record the number of resize observes at the beginning of the test, and subtract the current number.

Done

>> LayoutTests/resize-observer/resize-observer-callback-leak.html:30
>> +                }, 1000);
> 
> What is this step_timeout about? Why is 1s sufficient?
> On bots, we probably want a longer timeout.

My initial thought is to call gc() in other animation frame, so that it garbage collects all the observers (except the last one).
So 1s would be sufficient.

[cathiechen]

Created attachment 412066 [details]
Patch

[Ryosuke Niwa]

(In reply to cathiechen from comment #18)
> Comment on attachment 411999 [details]
> Patch
>
> >> LayoutTests/resize-observer/resize-observer-callback-leak.html:30
> >> +                }, 1000);
> > 
> > What is this step_timeout about? Why is 1s sufficient?
> > On bots, we probably want a longer timeout.
> 
> My initial thought is to call gc() in other animation frame, so that it
> garbage collects all the observers (except the last one).
> So 1s would be sufficient.

Can't we explicitly wait for that to happen instead?

[cathiechen]

(In reply to Ryosuke Niwa from comment #20)
> (In reply to cathiechen from comment #18)
> > Comment on attachment 411999 [details]
> > Patch
> >
> > >> LayoutTests/resize-observer/resize-observer-callback-leak.html:30
> > >> +                }, 1000);
> > > 
> > > What is this step_timeout about? Why is 1s sufficient?
> > > On bots, we probably want a longer timeout.
> > 
> > My initial thought is to call gc() in other animation frame, so that it
> > garbage collects all the observers (except the last one).
> > So 1s would be sufficient.
> 
> Can't we explicitly wait for that to happen instead?

Ah, yes, we can do that! It becomes more clear. Thanks!

[cathiechen]

Created attachment 412072 [details]
Patch

[EWS]

Committed r268860: <https://trac.webkit.org/changeset/268860>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412072 [details].

[Ryosuke Niwa]

Comment on attachment 412072 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412072&action=review

> Source/WebCore/bindings/js/JSResizeObserverCustom.cpp:36
> +    ResizeObserverCallback* callback = wrapped().callbackConcurrently();
> +    if (callback)

Should have declared callback inside if.