Bug 262219 – Previously focused form input elements are not getting garbage collected

• Component: Forms
• Status: RESOLVED MOVED

[Kin Blas]

In our web application we’ve noticed a significant number of detached DOM subtrees not getting garbage collected when running in Safari Desktop and on iOS. We’ve managed to narrow the issue down to the existence of form input elements within these subtrees. If I remove these form and input elements, the subtrees in question get entirely garbage collected.


The retained (leaking) subtrees in question correspond to panel UI within our application, so you can imagine how the process size of the web application tab grows steadily as the user triggers the showing and hiding of various panels. Our panels are quite large and complex so things sometimes get the point where Safari iOS will reload our web application.


Attached to this bug is a zip file containing a minimal test case that illustrates the issue. The curious thing is that with the minimal test case, in order to see the issue I described, the user must first give the input element focus, either by clicking or tabbing to it, at some point before pressing the Remove button to detach the sample subtree from the DOM. Within our application, I don’t need to interact with the input element to see the issue. Also, the minimal test case using <input type=“search”> to match our application, but I am able to reproduce the same issue with <input type=“text”>.


There is also a video attached to this bug that demonstrates what we see.


I can reproduce this issue in Safari 16.5 as well as WebKit 268505@main.


Instructions:

* Load the test case in a Safari or Webkit build.
* Click all of the Remove buttons on the page.
* Trigger a garbage collection via the terminal with ‘notifyutil -p org.WebKit.lowMemory’
* Observe that all of the test cases turn green
* Reload the test case
* Click on any number of inputs on the page to give them focus
* Click all of the Remove buttons on the page
* Trigger a garbage collection.
* Notice only samples that you did NOT give the input focus will turn green. All others will remain white indicating no garbage collection has happened.
* Click on all of the “Unparent Children” buttons
* Trigger a garbage collection
* Notice how most elements turn red to indicate they have been garbage collected, while only the form and input elements remain white indicating they have not been garbage collected.

[Kin Blas]

Created attachment 467906 [details]
Minimal test case that demonstrates form and input elements being retained (not garbage-collected)

Added a minimal test case that demonstrates form and input elements being retained (not garbage-collected)

[Kin Blas]

Created attachment 467907 [details]
Video demonstrating the form and input element retainer issue.

Attaching a video demonstrating the form and input element retainer issue.

[Kin Blas]

Created attachment 467908 [details]
All-in-one HTML file version of the minimal test case.

Attached an all-in-one HTML file of the minimal test case so folks can just click on the test case attachment in the bug to run the test case.

[Radar WebKit Bug Importer]

<rdar://problem/116157495>

[Kin Blas]

*** Bug 262218 has been marked as a duplicate of this bug. ***

[Ryan Reno]

This isn't reproducing for me with 268598@main + instrumentation with MiniBrowser but it does reproduce with that commit + instrumentation and Safari.

I also get this console output after the low memory warning with WebKit and Safari:
LEAK: 1168 WebCoreNode

[Simon Fraser (smfr)]

Might be related to Safari autofill.

[Ryan Reno]

Thank you for the bug report and the detailed test cases. This is being fixed in Safari.

[Kin Blas]

Created attachment 468239 [details]
Video demonstrating the issue can be reproduced on iOS Safari

Attaching a video demonstrating the issue can be reproduced on iOS Safari

[Kin Blas]

Created attachment 468653 [details]
Test case that demonstrates that a focused text input can trigger other inputs to be retained.

Added a test case that demonstrates that a focused text input can trigger other inputs to be retained.

[Cristian Linte]

Created attachment 468685 [details]
Heap snapshot showing JS wrapper referenced from other world

This shows that there are 2 JS Wrappers for the same input HTML node. One is from the main world and another from another world. The one from main world is not referenced from JS and the node has no parent so it's not kept alive by other nodes. The wrapper from other world is referenced from JS and this keeps alive both JS wrappers.

[Cristian Linte]

Created attachment 468686 [details]
Heap snapshot showing that FormMetadataJS keeps reference to the wrapper

Heap snapshot showing that FormMetadataJS keeps reference to the wrapper. FormMetadataJS is loaded in another world/context (window) by Safari and this keeps references to JS Wrappers from main page and in turn causes leaks. Main page has no reference to the DOM node either via JS or other nodes.

[Cristian Linte]

The leak is from Safari own JS context (world) where it loads FormMetadataJS and keeps references to HTML elements from the page. JS wrappers referenced from that context will cause all other JS wrappers for same element, e.g. from page JS context, to be kept alive and that is why we detect that leak via JS WeakRef.deref in the test files.
There doesn't seem to be anything that a web page can do to workaround this leaks. At best it can try to minimize the leaks which can be infeasible.
As such I hope that this gets a high priority since this is a major leak for large apps that keep removing and adding new input elements or form elements and the user focuses one of them.

The references from other JS context are visible in Web Inspector memory(JS heap) snapshot. I've attached screenshot to showcase the leak seen in the memory snapshot. The snapshot was taken after focus on input element from first Form Sample and and after clicking Unparent Children button for both form samples. The form sample that had the input with focus has all form elements leaked. The screenshots only showcase the input elements.


FormMetadataJS on the global object in that Safari JS context has _controlUniqueIDToControlMap that is never cleared and will keep form control elements alive. This is a big issue with web sites that keep removing and add new elements and also because the JS wrapper itself might end up leaking other JS objects or JS wrappers.
Another JS context is one for ReaderArticleFinderJS which also keeps references to HTML elements and this one not only form and form controls but also divs and other elements.

There is another leak on the native side where Safari::BrowserBundlePageFormClient keeps a reference to Webkit API wrapper for the html input element that was last focused. Once a new input is focused and unfocused it will clear the reference to the old one. This only leaks the native DOM element and those have only weak references from what I understood how Webkit works so this leak is not that bad.
I'll add more details in the ticket.



Details:
1. Native leaks:
Safari will leak the last focused input element. It keeps a reference to the Webkit API wrapper which in turn keeps the native DOM node. This is less of an issue since it would leak only that DOM node, my understanding is that Webkit DOM nodes have weak references to other DOM nodes and strong references are usually kept only via the JS wrappers.
Steps to showcase the leak
- focus input element (invokes InjectedBundlePageFormClient::textFieldDidBeginEditing). Safari`Safari::BrowserBundlePageFormClient::textFieldDidBeginEditing exits after keeping 1 reference (via Safari::WK::Type stored on this + 0x38). RefCount:  1
- unfocus input element or browser window (invokes InjectedBundlePageFormClient::textFieldDidEndEditing). Safari::BrowserBundlePageFormClient::textFieldDidEndEditing exits after keeping 1 reference (via TextFieldDidEndEditingState stored on this + 0x50). RefCount:  2
- async invocation of block defined in BrowserBundlePageFormClient::textFieldDidEndEditing executes which calls resetStateAfterTextFieldDidEndEditing which clears reference from textFieldDidBeginEditing. RefCount: 1
- focus another input element. RefCount: 1 for old focused element and new focused element
- unfocus input element. TextFieldDidEndEditingState is overwritten removing the reference to the old element. RefCount: 0 for older focused element and 2 for last focused element.
- async invocation of block defined in textFieldDidEndEditing. RefCount: 1 for last focused element
I haven't included temporary refs that the code takes as it executes. The ref count is taken after end of execution for Webkit API callbacks.

2. JS leak
- focus input element (invokes InjectedBundlePageFormClient::textFieldDidBeginEditing). Safari`Safari::BrowserBundlePageFormClient::textFieldDidBeginEditing creates FrameMetadata which calls WKBundleScriptWorldCreateWorld and loads JS script for FormMetadataJS (embedded in SafariShared binary) and also AutomaticPasswordJS. It then calls "globalThis.FormMetadataJS.textFieldOrSelectElementMetadata(textField, 0, false)" where textField is the focused input element. That JS execution will end up storing in FormMetadataJS references to form and form controls, input/texarea/select/button from the page(or frame).
- unfocus input element doesn't call FormMetadataJS and references from that JS context (world) is leaked. The reference is kept from `_forms` and `_controlUniqueIDToControlMap` arrays. The `_forms` are is updated sometimes and references might get removed so is less of an issue while `_controlUniqueIDToControlMap` is never cleared and it is a permanent leak. 
- as new input elements are created and then focused the leak increases.

The JS wrappers for the DOM nodes from the FormMetadataJS context will keep alive the JS wrappers for the same DOM nodes from the main page context. This wrappers in return can keep alive other wrappers or normal JS objects.
A JS wrapper for DOM node uses the top DOM node ancestor as an opaque root and all JS wrappers for nodes that use the same root node are kept alive. This means that form and form elements referenced by FormMetadataJS will also keep alive their ancestors and their subtrees.
The Webkit documentation has details about JS wrappers and how they are kept alive via opaque roots to implement correct JS lifetime semantics for the web.
https://github.com/WebKit/WebKit/blob/main/Introduction.md#understanding-document-object-model

[Cristian Linte]

Additional observations:
Sometimes the heap snapshot shows more than 2 Windows that are usually expected: main window and window for FormMetadataJS. Sometimes window (context) is created that contains ReaderArticleFinderJS and two other windows that contain OpenSearchURLFinder and extractMediaServiceSubscription.

The ReaderArticleFinderJS window is problematic also because it keeps references to DOM nodes from the page and keeps references to many more DOM nodes than FormMetadataJS. I haven't investigate how much of an issue this is and when the references are removed or new ones added. Also ReaderArticleFinderJS is rarely loaded and not sure what triggers it making testing and reproducing harder.

Sometimes I've seen heap snapshots where there are two windows, TBD if more, with FormMetadataJS. Unclear what the impact of that is. That seems unexpected because Safari creates one FrameMetadata per frame and caches it in a HashMap with the key being `WKWebProcessPlugInFrame*` that was passed from Webkit. If multiple window shows up in the heap snapshot that might indicate that there are multiple FrameMetadata in that hash map for different frames but why would it show up in the heap snapshot of a different frame?

[Cristian Linte]

Forgot to mention that my testing was done on macOS 14.0 (23A344) with Safari 17.0 (19616.1.27.211.1) and Webkit https://commits.webkit.org/270592@main