Closed Bug 1418886 Opened 7 years ago Closed 7 years ago

LoginManagerContent.jsm leaks DOM nodes via loginFormRootElements

Categories

(Toolkit :: Password Manager, defect, P1)

Product:
Toolkit
Component:
Password Manager
Version:
58 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: u606404, Assigned: mconley)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(2 files)

testcase
297 bytes, text/html
Details
Bug 1418886 - Make LoginManagerContent use a WeakSet for login form root elements.
59 bytes, text/x-review-board-request
MattN
: review+
Details
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]
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
> 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]
Attached 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)
Whiteboard: [MemShrink:P1][qf:p1] → [MemShrink:P1][qf:i60][qf:p1]
Priority: -- → P1
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 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+
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)
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
https://hg.mozilla.org/mozilla-central/rev/0a6dc8478a57
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Performance Impact: --- → P1
Whiteboard: [MemShrink:P1][qf:f60][qf:p1] → [MemShrink:P1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: