Bug 233025 – Defining a custom element should not cause global object's structure transitions

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

[Alexey Shvayka]

JSCustomElementInterface should keep strong references to constructor / callbacks

[Alexey Shvayka]

Created attachment 444026 [details]
Patch

[Geoffrey Garen]

Comment on attachment 444026 [details]
Patch

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

> Source/WebCore/bindings/js/JSCustomElementInterface.h:104
> +    JSC::Strong<JSC::JSObject> m_constructor;
> +    JSC::Strong<JSC::JSObject> m_connectedCallback;
> +    JSC::Strong<JSC::JSObject> m_disconnectedCallback;
> +    JSC::Strong<JSC::JSObject> m_adoptedCallback;
> +    JSC::Strong<JSC::JSObject> m_attributeChangedCallback;

Unfortunately, it can't be this simple because this would create a retain cycle that can't be broken by GC.

I think you could achieve something similarly elegant if you used JSValueInWrappedObject, and had the global object or the document visit this registry and these values during GC, in visitAdditionalChildren.

[Darin Adler]

Comment on attachment 444026 [details]
Patch

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

>> Source/WebCore/bindings/js/JSCustomElementInterface.h:104
>> +    JSC::Strong<JSC::JSObject> m_attributeChangedCallback;
> 
> Unfortunately, it can't be this simple because this would create a retain cycle that can't be broken by GC.
> 
> I think you could achieve something similarly elegant if you used JSValueInWrappedObject, and had the global object or the document visit this registry and these values during GC, in visitAdditionalChildren.

Also, we should create the test case that shows this. I made a lot of those when I first created JSValueInWrappedObject and started to deploy it to fix the retain cycles we had back then that can’t be broken by GC.

[Alexey Shvayka]

Dear Geoff and Darin,

Could you please help me understand adverse effects of having a strong reference in this particular case?

I agree that in general case this is bad, but JSCustomElementInterface can't possibly be destructed until a global object -- which holds a strong reference to never-destructible CustomElementRegistry, which in turn holds strong references to its JSCustomElementInterface -- is dead.

Custom element interfaces can't be removed from the registry per spec, only added. Even a cross-realm callback / constructor, which is only reference to its global object, can't be released per spec as custom element interface definition should hold a strong reference to it.

[Darin Adler]

If you can’t make a test showing a leak, then I think the leak probably doesn’t exist. I was taking Geoff’s word for it, and drawing analogy with other similar places, not assessing whether a strong reference was a problem.

[Geoffrey Garen]

> Custom element interfaces can't be removed from the registry per spec, only
> added. Even a cross-realm callback / constructor, which is only reference to
> its global object, can't be released per spec as custom element interface
> definition should hold a strong reference to it.

I think this means that there's no noticeable memory leak for the duration of an individual window/document on screen, but once the window/document is off screen and otherwise unreachable, that's the case where the JSC::Strong will cause a noticeable memory leak. The easiest ways to trigger this condition in a test are: insert and <iframe> into a document and then remove it (repeatedly); or navigate an <iframe> (repeatedly).

[Radar WebKit Bug Importer]

<rdar://problem/85574850>

[Alexey Shvayka]

Pull request: https://github.com/WebKit/WebKit/pull/3066

[Alexey Shvayka]

(In reply to Geoffrey Garen from comment #6)
> > Custom element interfaces can't be removed from the registry per spec, only
> > added. Even a cross-realm callback / constructor, which is only reference to
> > its global object, can't be released per spec as custom element interface
> > definition should hold a strong reference to it.
> 
> I think this means that there's no noticeable memory leak for the duration
> of an individual window/document on screen, but once the window/document is
> off screen and otherwise unreachable, that's the case where the JSC::Strong
> will cause a noticeable memory leak. The easiest ways to trigger this
> condition in a test are: insert and <iframe> into a document and then remove
> it (repeatedly); or navigate an <iframe> (repeatedly).

Thank you for your help on this, Geoff.

I've managed to prove that Strong causes a memory leak with cross-realm callback and WeakRef.
Updated the patch to rely on visitAdditionalChildren() just as you suggested.

[EWS]

Committed 253330@main (1b11bc2ebce8): <https://commits.webkit.org/253330@main>

Reviewed commits have been landed. Closing PR #3066 and removing active labels.