Closed
Bug 1469719
Opened 6 years ago
Closed 6 years ago
Don't load LoginManagerContent.jsm if there are no forms
Categories
(Toolkit :: Password Manager, enhancement)
Toolkit
Password Manager
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: mccr8, Assigned: kmag)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [overhead:70k][fxperf:p1])
Attachments
(1 file)
According to my logging, LoginManagerContent.jsm uses something like 46,000 bytes of memory per content process. (LoginHelper.jsm uses another 22000 bytes.) This ends up being always loaded in content processes, even if no page with a login has been loaded, because content.js contains the following code: addEventListener("pageshow", function(event) { LoginManagerContent.onPageShow(event, content); }); [...] addEventListener("blur", function(event) { LoginManagerContent.onUsernameInput(event); }); So, whenever we have a pageshow or a blur, the JSM gets loaded. (I looked at the other uses of this JSM, and they look like they won't get loaded unless there is actually a form.) This seems reasonable in the current state where we only have a couple of content processes, but with Fission presumably we'll have many content processes that have zero forms in them.
Updated•6 years ago
|
Whiteboard: [overhead:46k]
Updated•6 years ago
|
Whiteboard: [overhead:46k] → [overhead:46k][fxperf]
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → kmaglione+bmo
Whiteboard: [overhead:46k][fxperf] → [overhead:70k][fxperf]
Comment 2•6 years ago
|
||
Quick drive-by before looking into more details: isn't the pageshow event used for detection of insecure forms? Is there any other event that will trigger this module being loaded when a form exists on the page? My brief reading last week was that we'd need to fire some notification when a form is present in order to make this lazy loadable. side note: it will be interesting to see if making this lazy loaded will also take down other jsms like LoginHelper.jsm, FormAutofillUtils.jsm (maybe that's already part of your 70k calculation?)
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to :Felipe Gomes (needinfo me!) from comment #2) > Quick drive-by before looking into more details: isn't the pageshow event > used for detection of insecure forms? Yes. But the event handler only checks state data for the content page that would have been generated by other LoginManagerContent events. If none of those have been triggered, there's no state data for the page that could tell us it has hidden forms, so it's guaranteed to have no effect. > side note: it will be interesting to see if making this lazy loaded will > also take down other jsms like LoginHelper.jsm, FormAutofillUtils.jsm (maybe > that's already part of your 70k calculation?) Yes. This stops us from eagerly loading LoginHelper.jsm, which is another 22K. We still wind up loading FormAutoFillUtils, for the moment, though.
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8987401 [details] Bug 1469719: Avoid loading LoginManagerContent before it's needed. https://reviewboard.mozilla.org/r/252646/#review259276 ::: commit-message-94b46:1 (Diff revision 1) > +Bug 1469719: Avoid loading LoginManagerContent before it's needed. r?felipe It would be good to have a test to ensure this doesn't regress… maybe adding to https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/browser/base/content/test/performance/browser_startup_content.js#25 is sufficient (but I'm not familiar with that test)? ::: browser/base/content/content.js (Diff revision 1) > -addEventListener("pageshow", function(event) { > - LoginManagerContent.onPageShow(event, content); > -}); > addEventListener("DOMAutoComplete", function(event) { > LoginManagerContent.onUsernameInput(event); > }); > -addEventListener("blur", function(event) { > - LoginManagerContent.onUsernameInput(event); > -}); Shouldn't you make the same change for Android in https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#97 ? ::: toolkit/components/passwordmgr/LoginManagerContent.jsm:166 (Diff revision 1) > .getInterface(Ci.nsIContentFrameMessageManager); > } > > // This object maps to the "child" process (even in the single-process case). > var LoginManagerContent = { > + init(global) { `init` seems like a less than ideal name since it's not used for intializing the singleton. Maybe `setupEventListeners` (like `setupProgressListener`) would be better. I would also then move it to right before `setupProgressListener` so it's not confused as a contructor/initializer and so the private variables remain first in the object.
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8987401 [details] Bug 1469719: Avoid loading LoginManagerContent before it's needed. https://reviewboard.mozilla.org/r/252646/#review259276 > It would be good to have a test to ensure this doesn't regress… maybe adding to https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/browser/base/content/test/performance/browser_startup_content.js#25 is sufficient (but I'm not familiar with that test)? Oh, I didn't know that test existed. Yeah, I wanted to add a test for this, but I didn't think there was a particularly easy way to do it. > Shouldn't you make the same change for Android in https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#97 ? Yeah. Good catch. Would be nice if we had comments that this logic is duplicated for Android...
Comment 6•6 years ago
|
||
(FYI, I'm right now converting browser_startup_content.js to a blacklist. Haven't filed a bug yet, but I'm about to do so)
Comment 7•6 years ago
|
||
I mean: a whitelist
Assignee | ||
Comment 8•6 years ago
|
||
(In reply to :Felipe Gomes (needinfo me!) from comment #7) > I mean: a whitelist Excellent.
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #5) > > Shouldn't you make the same change for Android in https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/BrowserCLH.js#97 ? > > Yeah. Good catch. Would be nice if we had comments that this logic is > duplicated for Android... I'm going to ignore the Android case for now. It still works after these changes, and it's different enough that I don't really want to try to adapt it just now. And given that Fennec doesn't have e10s support, it's not as big of an issue there, anyway.
Comment hidden (mozreview-request) |
Updated•6 years ago
|
Whiteboard: [overhead:70k][fxperf] → [overhead:70k][fxperf:p1]
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8987401 [details] Bug 1469719: Avoid loading LoginManagerContent before it's needed. https://reviewboard.mozilla.org/r/252646/#review259756
Attachment #8987401 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 12•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bab121b4dd84f9715e6a9efa652556a91ea60a3c Bug 1469719: Avoid loading LoginManagerContent before it's needed. r=felipe
Comment 13•6 years ago
|
||
Backed out 3 changesets (bug 1470023, bug 1469719, bug 1470965) for | toolkit/components/perfmonitoring/tests/browser/browser_compartments.js on a CLOSED TREE Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bab121b4dd84f9715e6a9efa652556a91ea60a3c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=43040128202efc47d4249e623e6d3ffd1a5d9588&filter-classifiedState=unclassified&selectedJob=185004097 Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=24aeaadfae0df355fb2fac7ea25ea90fa9c56ddf&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified Log: https://treeherder.mozilla.org/logviewer.html#?job_id=185004097&repo=mozilla-inbound&lineNumber=2513 12:29:59 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Error: Error: No matching message handler :: Async*background@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:3:11 12:29:59 INFO - async*@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:1:17 12:29:59 INFO - - 12:29:59 INFO - Stack trace: 12:29:59 INFO - background@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:187:7 12:29:59 INFO - async*@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:1:17 12:29:59 INFO - 12:29:59 INFO - Not taking screenshot here: see the one that was previously logged 12:29:59 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | executeScript - 12:29:59 INFO - Stack trace: 12:29:59 INFO - background@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:188:7 12:29:59 INFO - async*@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:1:17 12:29:59 INFO - 12:29:59 INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | test result correct - 12:29:59 INFO - Console message: [JavaScript Error: "Promise rejected after context unloaded: Message manager disconnected 12:29:59 INFO - " {file: "moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/script.js" line: 2}] 12:29:59 INFO - @moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/script.js:2:9 12:29:59 INFO - 12:29:59 INFO - Console message: Ignoring response to aborted listener for 1099511627780 12:29:59 INFO - Console message: Cannot send function call result: other side closed connection (call data: ({path:"tabs.executeScript", args:[409, {allFrames:null, code:"42", cssOrigin:null, file:null, frameId:null, matchAboutBlank:null, runAt:"document_idle"}]})) 12:29:59 INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Message manager count - 12:29:59 INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Pending response count - 12:29:59 INFO - Leaving test bound testExecuteScript 12:29:59 INFO - Console message: Ignoring response to aborted listener for 1668 12:29:59 INFO - Console message: Cannot send function call result: other side closed connection (call data: ({path:"tabs.executeScript", args:[410, {allFrames:null, code:"location.href", cssOrigin:null, file:null, frameId:null, matchAboutBlank:null, runAt:"document_idle"}]})) 12:29:59 INFO - GECKO(869) | MEMORY STAT | vsize 4724MB | residentFast 794MB | heapAllocated 190MB 12:29:59 INFO - TEST-OK | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | took 939ms 12:29:59 INFO - Not taking screenshot here: see the one that was previously logged 12:29:59 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Found an unexpected tab at the end of test run: about:blank - 12:29:59 INFO - Not taking screenshot here: see the one that was previously logged 12:29:59 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Found an unexpected tab at the end of test run: http://example.com/ - 12:29:59 INFO - Not taking screenshot here: see the one that was previously logged 12:29:59 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Found an unexpected tab at the end of test run: http://example.net/ - 12:29:59 INFO - checking window state
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 14•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/255aed5d516bd3c7ec8e2c7c9b8a7bf37aaec030 Bug 1469719: Avoid loading LoginManagerContent before it's needed. r=felipe
Assignee | ||
Comment 15•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2da1cbe9d57d774ef25b993299075ed471c0872 Bug 1469719: Follow-up: Update test expectations. r=me,test-only
Assignee | ||
Comment 16•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1bc28680502307702d096c81c462986388e8170 Bug 1469719: Follow-up: Fix botched file save. r=stupid-fixup
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/255aed5d516b https://hg.mozilla.org/mozilla-central/rev/f2da1cbe9d57 https://hg.mozilla.org/mozilla-central/rev/f1bc28680502
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(kmaglione+bmo) → qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•