1851684 - MozLitElement leaks render roots for subclasses that are added and removed to the DOM

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

I think this is the cause for the leak that jsudiaman is running into in bug 1849124.

Most of our LitElements inherit from MozLitElement, which is defined here: https://searchfox.org/mozilla-central/rev/b6cd901cf074c5e75e3902d660bafa3cf3967c40/toolkit/content/widgets/lit-utils.mjs#84-126

one of the things that MozLitElement does is provide Fluent support for subclasses. It does this by connecting the render root in the override for connectedCallback: https://searchfox.org/mozilla-central/rev/b6cd901cf074c5e75e3902d660bafa3cf3967c40/toolkit/content/widgets/lit-utils.mjs#110

It looks like when one connects a render root, it's added to a hashmap and a mutation observer is attached to it. The hashmap appears to hold a strong reference to the render root.

I believe this means that if many MozLitElement's are added and removed from the DOM, they'll be considered "orphaned", and kept alive off of the DOMLocalization window object.

I can observe this by hacking history.mjs to create many thousand card-container elements, and then switching back and forth between History and Recent Browsing in Firefox View. Then I go to about:memory, minimize memory usage, and take a memory snapshot. I see many megabytes of orphan nodes reported after these STR.

There's a disconnectRoot that I believe we're supposed to call if an item translated with Fluent is removed from the DOM: https://searchfox.org/mozilla-central/rev/b6cd901cf074c5e75e3902d660bafa3cf3967c40/dom/webidl/DOMLocalization.webidl#54-58

We do this in places like this: https://searchfox.org/mozilla-central/rev/b6cd901cf074c5e75e3902d660bafa3cf3967c40/toolkit/mozapps/extensions/content/aboutaddons.js#968-970

I suspect we should do the same thing here for MozLitElement. This also means that all subclasses of MozLitElement should make sure to call super.disconnectedCallback().

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1851684 - Make MozLitElement disconnect root from DocumentL10n in disconnectedCallback. r?mstriemer!

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1851684 - Make sure to call the MozLitElement disconnectedCallback in subclass overrides. r?#fxview-reviewers!,#shopping-reviewers!

Depends on D187505

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/159ff8cfdf58
Make MozLitElement disconnect root from DocumentL10n in disconnectedCallback. r=mstriemer,reusable-components-reviewers,hjones
https://hg.mozilla.org/integration/autoland/rev/36630f215585
Make sure to call the MozLitElement disconnectedCallback in subclass overrides. r=fxview-reviewers,shopping-reviewers,kcochrane,kpatenio

Comment 4

[Iulian Moraru]

Backed out for causing mochitest-chrome failures on test_login_filter.html.

• Push with failures

• Failure log

• Backout link

[task 2023-09-06T00:17:03.682Z] 00:17:03     INFO - TEST-PASS | browser/components/aboutlogins/tests/chrome/test_login_filter.html | Event includes input value 
[task 2023-09-06T00:17:03.683Z] 00:17:03     INFO - add_task | Leaving test_input_events
[task 2023-09-06T00:17:03.684Z] 00:17:03     INFO - add_task | Entering test_list_filtered
[task 2023-09-06T00:17:03.685Z] 00:17:03     INFO - Buffered messages finished
[task 2023-09-06T00:17:03.688Z] 00:17:03     INFO - TEST-UNEXPECTED-FAIL | browser/components/aboutlogins/tests/chrome/test_login_filter.html | uncaught exception - TypeError: document.l10n.disconnectRoot is not a function at disconnectedCallback@chrome://global/content/lit-utils.mjs:122:21
[task 2023-09-06T00:17:03.689Z] 00:17:03     INFO - setLogins@chrome://browser/content/aboutlogins/components/login-list.mjs:471:5
[task 2023-09-06T00:17:03.690Z] 00:17:03     INFO - test_list_filtered@chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_filter.html:69:14
[task 2023-09-06T00:17:03.690Z] 00:17:03     INFO - add_task/nextTick/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2147:34
[task 2023-09-06T00:17:03.691Z] 00:17:03     INFO - async*nextTick@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2182:11
[task 2023-09-06T00:17:03.692Z] 00:17:03     INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:920:41
[task 2023-09-06T00:17:03.693Z] 00:17:03     INFO - add_task@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2096:17
[task 2023-09-06T00:17:03.693Z] 00:17:03     INFO - add_setup@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2202:10
[task 2023-09-06T00:17:03.694Z] 00:17:03     INFO - @chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_filter.html:31:10
[task 2023-09-06T00:17:03.695Z] 00:17:03     INFO - 
[task 2023-09-06T00:17:03.695Z] 00:17:03     INFO - simpletestOnerror@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2003:18
[task 2023-09-06T00:17:03.696Z] 00:17:03     INFO - setLogins@chrome://browser/content/aboutlogins/components/login-list.mjs:471:5
[task 2023-09-06T00:17:03.697Z] 00:17:03     INFO - test_list_filtered@chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_filter.html:69:14
[task 2023-09-06T00:17:03.698Z] 00:17:03     INFO - add_task/nextTick/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2147:34
[task 2023-09-06T00:17:03.699Z] 00:17:03     INFO - async*nextTick@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2182:11
[task 2023-09-06T00:17:03.699Z] 00:17:03     INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:920:41
[task 2023-09-06T00:17:03.700Z] 00:17:03     INFO - add_task@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2096:17
[task 2023-09-06T00:17:03.701Z] 00:17:03     INFO - add_setup@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2202:10
[task 2023-09-06T00:17:03.701Z] 00:17:03     INFO - @chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_filter.html:31:10
[task 2023-09-06T00:17:03.703Z] 00:17:03     INFO - GECKO(1559) | JavaScript error: chrome://global/content/lit-utils.mjs, line 122: TypeError: document.l10n.disconnectRoot is not a function
[task 2023-09-06T00:17:03.704Z] 00:17:03     INFO - Not taking screenshot here: see the one that was previously logged
[task 2023-09-06T00:17:03.707Z] 00:17:03     INFO - TEST-UNEXPECTED-FAIL | browser/components/aboutlogins/tests/chrome/test_login_filter.html | uncaught exception - TypeError: document.l10n.disconnectRoot is not a function at disconnectedCallback@chrome://global/content/lit-utils.mjs:122:21
[task 2023-09-06T00:17:03.707Z] 00:17:03     INFO - setLogins@chrome://browser/content/aboutlogins/components/login-list.mjs:471:5
[task 2023-09-06T00:17:03.708Z] 00:17:03     INFO - test_list_filtered@chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_filter.html:69:14
[task 2023-09-06T00:17:03.709Z] 00:17:03     INFO - add_task/nextTick/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2147:34
[task 2023-09-06T00:17:03.710Z] 00:17:03     INFO - async*nextTick@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2182:11
[task 2023-09-06T00:17:03.710Z] 00:17:03     INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:920:41
[task 2023-09-06T00:17:03.711Z] 00:17:03     INFO - add_task@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2096:17
[task 2023-09-06T00:17:03.712Z] 00:17:03     INFO - add_setup@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2202:10
[task 2023-09-06T00:17:03.713Z] 00:17:03     INFO - @chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_filter.html:31:10
[task 2023-09-06T00:17:03.714Z] 00:17:03     INFO - 
[task 2023-09-06T00:17:03.714Z] 00:17:03     INFO - simpletestOnerror@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2003:18
[task 2023-09-06T00:17:03.715Z] 00:17:03     INFO - setLogins@chrome://browser/content/aboutlogins/components/login-list.mjs:471:5
[task 2023-09-06T00:17:03.716Z] 00:17:03     INFO - test_list_filtered@chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_filter.html:69:14
[task 2023-09-06T00:17:03.717Z] 00:17:03     INFO - add_task/nextTick/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2147:34
[task 2023-09-06T00:17:03.717Z] 00:17:03     INFO - async*nextTick@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2182:11
[task 2023-09-06T00:17:03.718Z] 00:17:03     INFO - setTimeout handler*SimpleTest_setTimeoutShim@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:920:41
[task 2023-09-06T00:17:03.719Z] 00:17:03     INFO - add_task@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2096:17
[task 2023-09-06T00:17:03.720Z] 00:17:03     INFO - add_setup@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:2202:10
[task 2023-09-06T00:17:03.721Z] 00:17:03     INFO - @chrome://mochitests/content/chrome/browser/components/aboutlogins/tests/chrome/test_login_filter.html:31:10
[task 2023-09-06T00:17:03.722Z] 00:17:03     INFO - GECKO(1559) | JavaScript error: chrome://global/content/lit-utils.mjs, line 122: TypeError: document.l10n.disconnectRoot is not a function
[task 2023-09-06T00:17:03.723Z] 00:17:03     INFO - Testcase: 0
[task 2023-09-06T00:17:03.724Z] 00:17:03     INFO - Not taking screenshot here: see the one that was previously logged
[task 2023-09-06T00:17:03.728Z] 00:17:03     INFO - TEST-UNEXPECTED-FAIL | browser/components/aboutlogins/tests/chrome/test_login_filter.html | uncaught exception - TypeError: document.l10n.disconnectRoot is not a function at disconnectedCallback@chrome://global/content/lit-utils.mjs:122:21
[task 2023-09-06T00:17:03.729Z] 00:17:03     INFO - render@chrome://browser/content/aboutlogins/components/login-list.mjs:236:19
[task 2023-09-06T00:17:03.730Z] 00:17:03     INFO - handleEvent@chrome://browser/content/aboutlogins/components/login-list.mjs:394:14
[task 2023-09-06T00:17:03.730Z] 00:17:03     INFO - _dispatchFilterEvent@chrome://browser/content/aboutlogins/components/login-filter.mjs:88:10
<...>

Comment 5

[:Gijs (he/him)]

As an aside/addition, should we add a linter for this? And probably the same for connectedCallback and any other important lit/CE lifecycle methods?

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #5)

As an aside/addition, should we add a linter for this? And probably the same for connectedCallback and any other important lit/CE lifecycle methods?

Ah, spotted bug 1851707 - as usual you are ahead of me. :-)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a1c50494e274
Make MozLitElement disconnect root from DocumentL10n in disconnectedCallback. r=mstriemer,reusable-components-reviewers,hjones,credential-management-reviewers,mtigley
https://hg.mozilla.org/integration/autoland/rev/c10d577d7dd0
Make sure to call the MozLitElement disconnectedCallback in subclass overrides. r=fxview-reviewers,shopping-reviewers,kcochrane,kpatenio

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/a1c50494e274
https://hg.mozilla.org/mozilla-central/rev/c10d577d7dd0