Closed Bug 1469719 Opened Last year Closed Last year

Don't load LoginManagerContent.jsm if there are no forms

Categories

(Toolkit :: Password Manager, enhancement)

enhancement
Not set

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.
Whiteboard: [overhead:46k]
Whiteboard: [overhead:46k] → [overhead:46k][fxperf]
Assignee: nobody → kmaglione+bmo
Whiteboard: [overhead:46k][fxperf] → [overhead:70k][fxperf]
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?)
(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 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.
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...
(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)
I mean: a whitelist
(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> I mean: a whitelist

Excellent.
(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.
Whiteboard: [overhead:70k][fxperf] → [overhead:70k][fxperf:p1]
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+
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)
Flags: needinfo?(kmaglione+bmo) → qe-verify-
You need to log in before you can comment on or make changes to this bug.