LoginManagerContent.jsm leaks DOM nodes via loginFormRootElements

RESOLVED FIXED in Firefox 59

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: u606404, Assigned: mconley)

Tracking

58 Branch
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [MemShrink:P1][qf:f60][qf:p1])

Attachments

(2 attachments)

Reporter

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171118100420

Steps to reproduce:

Clean profile, Firefox 58.0b4 (64 bit), Windows 10

Open https://8ch.net/tech/res/733048.html
(Any thread will do but the issue is easier to notice on threads with lots of posts.)
Scroll to the bottom of the page.
Click on [Update] repeatedly.


Actual results:

Memory usage keeps increasing indefinitely, reaching a whole gigabyte after a few dozens clicks.

If the page is refreshed memory usage will decrease back to normal levels after a few seconds after a short spike.


Expected results:

Memory usage should stay roughly level the whole time as it does in Chrome.
Component: Untriaged → General
Product: Firefox → Core
Its possible we are getting some polyfill or different library than chrome that is leaking.  We should verify we are not keeping things around unnecessarily, though.
Whiteboard: [MemShrink]
Reporter

Comment 2

2 years ago
I managed to create a minimal test case. It's not a polyfill issue.

<html>
    <head>
        <script>
            setInterval(function() {
                document.body.innerHTML = '<form><input type="password"/></form>';
            }, 0);
        </script>
    </head>
    <body>
    </body>
</html>

This seems to do the same thing albeit a lot slower. Memory snapshots look roughly the same. Thousands upon thousands of HTMLInputElement and HTMLFormElement objects with call stacks starting in onDOMFormHasPassword
It would still be nice if someone more competent tried the original reproduction steps and checked if I missed something.
Olli, did you work on input element stuff recently for qf?  Any ideas why the case in comment 2 would leak?

I don't see anything immediately wrong with the code.  Since its doing innerHTML there are no js exposed DOM objects that can be leaked AFAICT.
Flags: needinfo?(bugs)
Looking.

"Thousands upon thousands of HTMLInputElement and HTMLFormElement objects with call stacks starting in onDOMFormHasPassword"
What call stack? Where?
But onDOMFormHasPassword sounds suspicious.
https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/toolkit/components/passwordmgr/LoginManagerContent.jsm#372
Reporter

Comment 5

2 years ago
> What call stack? Where?

The call stack that is recorded by the memory snapshot developer tool.

The top entry in View:Aggregate, Group by:Call Stack is
content.js:51:3
\ onDOMFormHasPassword LoginManagerContent.jsm:378:20
  \ createFromForm LoginManagerContent.jsm:1587:20
    \ createFromForm FormLikeFactory.jsm:36:18
      \ next self-hosted:575:9

I'm no expert, that's just how I figured out that it's related to password input fields.
(In reply to Dave from comment #5)
> > What call stack? Where?
> 
> The call stack that is recorded by the memory snapshot developer tool.
> 
> The top entry in View:Aggregate, Group by:Call Stack is
> content.js:51:3
> \ onDOMFormHasPassword LoginManagerContent.jsm:378:20
>   \ createFromForm LoginManagerContent.jsm:1587:20
>     \ createFromForm FormLikeFactory.jsm:36:18
>       \ next self-hosted:575:9
> 
Thanks. I don't use devtools for leaks but other stuff. But looks like it is useful here.
Component: General → Form Manager
Flags: needinfo?(bugs)
Product: Core → Toolkit
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [MemShrink] → [MemShrink][qf:p1]
Posted file testcase
GC log shows tons of 
0x7f483f488e80 B Call <no private>
> 0x7f484c998e20 B group
> 0x7f483f24eb50 B shape
> 0x7f483f2523a0 B enclosing_environment
> 0x7f483f2731a0 B callee_slot
> 0x7f483fbb0ac0 B aFormLike
> 0x7f4835395d80 B prettyElementOutput

0x7f483fbb0ac0 B Object <no private>
> 0x7f483f2751f0 B group
> 0x7f483f24ecb8 B shape
> 0x7f4834ee3820 B elements
> 0x7f4835334080 B rootElement
> 0x7f48384590b8 B action
> 0x7f484c90e6a0 B autocomplete
> 0x7f48433d5540 B ownerDocument
> 0x7f4834ee4060 B toJSON
Flags: needinfo?(paolo.mozmail)
BTW, thanks Dave. This was a great bug report.
Hey MattN - looks like login manager might be leaking. Got cycles to look at this?
Flags: needinfo?(MattN+bmo)
Whiteboard: [MemShrink][qf:p1] → [MemShrink:P1][qf:p1]
Component: Form Manager → Password Manager
Summary: Memory leak when updating threads on 8ch.net (not present in Chrome) → LoginManagerContent.jsm leaks DOM nodes via loginFormRootElements
I think MattN's busy. I'll look at this.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(MattN+bmo)

Updated

2 years ago
Whiteboard: [MemShrink:P1][qf:p1] → [MemShrink:P1][qf:i60][qf:p1]
Priority: -- → P1

Updated

2 years ago
Whiteboard: [MemShrink:P1][qf:i60][qf:p1] → [MemShrink:P1][qf:f60][qf:p1]
Without this patch, smaug's testcase makes my content process' memory usage balloon up to > 1GB in very little time. I usually kill it around then.

With the patch, smaug's testcase makes my content process' memory usage stabilize around 350MB.
If this approach isn't desirable, the other thing I can think of is to fire some kind of event any time a <form> element is unbound from the DOM tree, and remove from the Set() then.

Comment 15

2 years ago
mozreview-review
Comment on attachment 8937281 [details]
Bug 1418886 - Make LoginManagerContent use a WeakSet for login form root elements.

https://reviewboard.mozilla.org/r/207974/#review214676

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:495
(Diff revision 1)
>  
>      // Returns true if this window or any subframes have insecure login forms.
>      let hasInsecureLoginForms = (thisWindow) => {
>        let doc = thisWindow.document;
> -      let hasLoginForm = this.stateForDocument(doc).loginFormRootElements.size > 0;
> +      let rootElsWeakSet = this.stateForDocument(doc).loginFormRootElements;
> +      let hasLoginForm = ChromeUtils.nondeterministicGetWeakSetKeys(rootElsWeakSet)

Thanks! I didn't know `nondeterministicGetWeakSetKeys` was allowed to be used in shipping code, I thought it was just for tests/debugging.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:891
(Diff revision 1)
> -    for (let formRoot of state.loginFormRootElements) {
> +    for (let formRoot of weakLoginFormRootElements) {
> +      if (!formRoot.isConnected) {
> +        continue;

Just to make sure, is it possible for a key to be GC'd while iterating over `weakLoginFormRootElements`?
Attachment #8937281 - Flags: review?(MattN+bmo) → review+
Assignee

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8937281 [details]
Bug 1418886 - Make LoginManagerContent use a WeakSet for login form root elements.

https://reviewboard.mozilla.org/r/207974/#review214676

> Thanks! I didn't know `nondeterministicGetWeakSetKeys` was allowed to be used in shipping code, I thought it was just for tests/debugging.

I've found at least one instance of it being used elsewhere in production code:

https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/toolkit/components/extensions/ExtensionContent.jsm#180

I'll double-check with someone from the JS team before proceeding.

> Just to make sure, is it possible for a key to be GC'd while iterating over `weakLoginFormRootElements`?

I somehow don't think so - as soon as we use nondeterministicGetWeakSetKeys, I think we've got references to each of the WeakSet elements, and so I don't think they're going to get GC'd until (at least) `weakLoginFormRootElements` goes away. I'll double-check with the JS team though to be sure.
Hey evilpie, do you know the answers to MattN's questions above re: nondeterministicGetWeakSetKeys ?
Flags: needinfo?(evilpies)
Redirecting comment 17 to sfink.
Flags: needinfo?(evilpies) → needinfo?(sphink)
I spoke to jorendorff about this in #jsapi. He mentioned the non-determinism of nondeterministicGetWeakSetKeys, but the isConnected checks I'm doing should allow us to side-step those cases, since we don't really care about form/form-like elements that aren't in the DOM.

Also, he confirmed that once we use nondeterministicGetWeakSetKeys, we have real strong references to the nodes, so they won't get GC'd out from underneath us.
Flags: needinfo?(sphink)

Comment 20

2 years ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a6dc8478a57
Make LoginManagerContent use a WeakSet for login form root elements. r=MattN

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0a6dc8478a57
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.