839961 - Refactor login manager's content actions into a separate JSM

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Justin Dolske [:Dolske]]

Bug 771331 (by way of bug 355063) seeks to make login manager driven by the addition of password fields to the page, instead of having login manager scan every page exactly once when DOMContentLoad fires.

In preparation for this, I'd like to refactor nsLoginManager.js so that the XPCOM service solely deals with its XPCOM APIs, and move all of the content-touching pieces into a separate JSM. This has a few advantages:

  * Cleaner separation of functionally-different code
  * Easier to roll into a frameScript
  * Needed anyway for E10S and unforking the mobile/metro version of pwmgr.
  * Probably simplifies an eventual async pwmgr update

Doing this now sets up better foundation for the changes needed for bug 355063, and helps mitigate that risk by getting some of the work out of the way now.

[Note to self: I started off on the wrong foot in bug 839741, starting fresh here.]

Comment 1

[Justin Dolske [:Dolske]]

Attachment: Part 1: Make logging global

Trivial patch to make the logging function global. Will cut down on diffs when refactoring.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Part 2: Create content JSM

Comment 3

[Justin Dolske [:Dolske]]

Needs a bit more polish, but is currently good enough to fill and save password. Next chunk of work is to move the autocomplete bits over.

Comment 4

[Justin Dolske [:Dolske]]

Attachment: Proof of concept for bug 771331

This is a crude hack, but shows the first step of hooking up the code in bug 771331.

Comment 5

[Justin Dolske [:Dolske]]

Attachment: Part 2: Create content JSM

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Part 3: autocomplete bits

I've never really liked how autocomplete works, and I still don't. :/ But things are generally working with this.

* Seems like nsILoginManager.autoCompleteSearch and the nsIAutoCompleteResult implementation should continue to live in the XPCOM service, since I think the autocomplete dropdown is effectively "chrome UI" like the context menu... I'm curious if felipe has opinions on that wrt E10S.

* I'm not sure if the blur _and_ DOMAutoComplete listeners are still needed. From brief testing, I didn't seem to be able to trigger the |blur| case, I always got an actual DOMAutoComplete. I wonder if it's no longer necessary or was due to some since-fixed bug. That would be ideal, because I'm not thrilled about having code listening for all blur events on every page.

Comment 7

[Justin Dolske [:Dolske]]

Oh, and two general notes:

* A lot of the code in LoginManagerContent.jsm is just moved over from nsLoginManager.js with minimal (if any) change. For the flagged feedback passes, I won't stare at it much. I should be able to make more minimal diffs manually when it's ready for a real review.

* It's nice how the frameLoader helps to get rid of the ugly docloader/webprogress listener. Quite nice.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 712353 [details] [diff] [review]
Part 1: Make logging global

Review of attachment 712353 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +10,5 @@
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
>  
> +var debug = false;
> +function log(message) {

While we're touching every log call it would be nice to switch the function to use rest arguments to avoid string concatenation overhead.

Comment 9

[David E. Ross]

If you are making any significant changes to Password Manager, can you also address bug #227632, bug #242418, bug #376668, bug #425145, and bug #658936?

Comment 10

[Justin Dolske [:Dolske]]

This bug is about a refactoring. No fixing the whole world.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 712365 [details] [diff] [review]
Part 3: autocomplete bits

Review of attachment 712365 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah the frameloader do help with handling the targets/loading/etc. The caveat with this type of change is making sure not to make code that ran once per window (in browser.js) end up in a content script that runs once per tab.  I think using .jsms are indeed the way to go.

FWIW have you seen this? http://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/LoginManagerChild.js  It seems that a reasonable amount of similar code has be written/copied there. (though it's a big chunk of content script code that suffers from the problem mentioned above).

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +90,5 @@
> +        if (!event.isTrusted)
> +            return;
> +
> +        var acInputField = event.target;
> +        // XXX probably want stricter checking, but meh?

I think this is fine, the check for isTrusted should cover it. Unless content can somewhat generate a isTrusted event, in which case there are bigger problems..

This comment is worth noting: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsFormFillController.cpp#541
but I think it is just telling the backend to not screw up.. I would assume these changes here won't change how the .cpp part of the autocomplete svc works

@@ +92,5 @@
> +
> +        var acInputField = event.target;
> +        // XXX probably want stricter checking, but meh?
> +        // XXX also audio any previous assumptions since context is different
> +        // eg, is there an autofill pref check happing somewhere still?

I think these prefs are controlled by the front-end, right? If so unless the pref checks were also moved together in one of the other patches, I'd assume it's not being checked. But FWIW the content scripts (and child processes) have access to the pref service through normal means, e.g. Services.prefs.getBoolPref etc.

@@ +616,2 @@
>          if (usernameField)
> +            this._formFillService.markAsLoginManagerField(usernameField);

I don't understand exactly why this part was commented out before but now it isn't

Comment 12

[Justin Dolske [:Dolske]]

(In reply to :Felipe Gomes from comment #11)

> FWIW have you seen this?
> ...browser/metro/base/content/LoginManagerChild.js

Yeah. This got forked back when Fennec XUL+E10S was rushing to ship.


> > +        // XXX probably want stricter checking, but meh?
> 
> I think this is fine, the check for isTrusted should cover it. Unless
> content can somewhat generate a isTrusted event, in which case there are
> bigger problems.

This was actually a (poorly phrased) note to myself... Previously the blue/autocomplete event would only be coming from a text field we had already processed at page load -- now it could come from anything in the page at any time.

I added a _isPossibleUsernameField() to unify the check here with the check in _getFormFields(), and also made a run through the old code path and added some more checks to onUsernameInput() to make sure it retains the existing behavior.

> This comment is worth noting:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/
> nsFormFillController.cpp#541
> but I think it is just telling the backend to not screw up.. I would assume
> these changes here won't change how the .cpp part of the autocomplete svc
> works

Correct.

> >          if (usernameField)
> > +            this._formFillService.markAsLoginManagerField(usernameField);
> 
> I don't understand exactly why this part was commented out before but now it
> isn't

That's just due to the interdependency of the two patches in this bug. This had been calling attachToInput(), part 1 moved this code around, and part 2 replaces it. I updated the comment in part 1. :)

Comment 13

[Justin Dolske [:Dolske]]

Oh, hey. By moving the autocomplete stuff to a frameScript, this partially fixes bug 355063. We still can't autofill a post-load script-generated form (for a 1-click signin), but now autocomplete will work. So you can start typing a login (or just click to trigger the autocomplete dropdown), and tab your way to success. \o/

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Part 1: Create content JSM

This is on top of the logging patch in bug 851846 (which was previously "Part 1" here).

Comment 15

[Justin Dolske [:Dolske]]

Attachment: Part 2: autocomplete bits

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 725834 [details] [diff] [review]
Part 1: Create content JSM

Review of attachment 725834 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/Makefile.in
@@ +24,5 @@
>    storage-mozStorage.js \
>    $(NULL)
>  
> +EXTRA_JS_MODULES = \
> +  LoginManagerContent.jsm \

I think you need to add this file to package-manifest.in for the relevant platforms.

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +135,1 @@
>          // Form submit observer checks forms for new logins and pw changes.

I think this comment was supposed to move too

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 725835 [details] [diff] [review]
Part 2: autocomplete bits

Review of attachment 725835 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -582,5 @@
>          // Attach autocomplete stuff to the username field, if we have
>          // one. This is normally used to select from multiple accounts,
>          // but even with one account we should refill if the user edits.
> -/*
> -XXX fixed next in patch series.

Because of the dependency between the two patches, I would probably fold the two together before landing.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] from comment #16)
> > +  LoginManagerContent.jsm \
> 
> I think you need to add this file to package-manifest.in for the relevant
> platforms.

Not needed for modules, those are covered by the catch-all at http://hg.mozilla.org/mozilla-central/annotate/0bba6ba92467/browser/installer/package-manifest.in#l528.

Comment 19

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Yeah, I guess having this split into 2 parts isn't a big help, so I qfold'd them. Cleaned up a bit and made sure it passes xpcshell & mochitest locally.

Comment 20

[[e-mail]]

Comment on attachment 727520 [details] [diff] [review]
Patch v.2

>+addEventListener("DOMContentLoaded", function(event) {
>+  LoginManagerContent.onContentLoaded(event);
>+});
>+addEventListener("DOMAutoComplete", function(event) {
>+  LoginManagerContent.onUsernameInput(event);
>+});
>+addEventListener("blur", function(event) {
>+  LoginManagerContent.onUsernameInput(event);
>+});
So anyone wanting inline password filling now has to do this?
[Any reason why LoginManagerContent isn't an event listener?]

Comment 21

[Justin Dolske [:Dolske]]

Attachment: Patch v.2 (fix trivial bitrot)

Comment 22

[Justin Dolske [:Dolske]]

Attachment: Fix Metro and Android (v.0)(WIP)

Blind attempt at updating Android and Metro.

Comment 23

[Frank Yan (:fryn)]

Over IRC, Dolske asked me to check on the state of this in Metro, but I actually haven't looked at the Metro code for this, so I'm handing this over to Tim, who is working on prompting and related code for Metro:

(In reply to Justin Dolske [:Dolske] from IRC)
> any chance you could try out my two patches in bug 839961?
> It's pretty simple and should work, I'm just not set up to build Metro.
> and I guess I'm a little unclear how/if things work today for 
> the autocomplete stuff on metro, so maybe it's "just as broken".
> (eg if you have multiple Google accounts, and go to log in on 
> google.com... what happens? can you tap and select one? does 
> using a mouse work?)

Comment 24

[Justin Dolske [:Dolske]]

Tim and I both verified that this appears to be working on metro (I didn't know --enable-metro + --metrodesktop was so easy now).

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

https://tbpl.mozilla.org/?tree=Try&rev=5df6490b77ec

note the Android M8 failures

Comment 26

[:Margaret Leibovic]

I made a local build with these patches, and the login manager works as expected, so I'm not sure why there were test failures. I wonder if there could be a problem with the way the tests are structured? Although I would have though they would fail on desktop, too, if that were the case.

One difference I see is that the Android event listeners initiate capture, but the desktop/metro listeners don't.

Comment 27

[Justin Dolske [:Dolske]]

Pushed to Try again lest it have started working, but it still reports the same failures: https://tbpl.mozilla.org/?tree=Try&rev=bdd6f5902d74

Comment 28

[:Margaret Leibovic]

So, I was able to reproduce this when running the tests locally, but suspiciously, they only fail when I run the whole test suite, not when I run them individually.

Who knows about the infrastructure of how these tests run on Android? jmaher? I can dig into this more, but it seems more like Android test flakiness than an issue with dolske's patches.

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

the main difference for how the tests run on android vs desktop is that we run them to a remote webserver instead of a webserver on localhost.  There are many tests that depend on the results or states left behind of previous tests, it is unfortunate but the reality (both mobile and desktop).

For these tests, I see that we are not detecting any values for the login manager (username/password).  Could it be something different with the way mobile handles forms, focus, input?

Comment 30

[Justin Dolske [:Dolske]]

Attachment: Fix Metro and Android (v.1)

Margaret was able to do do some debugging with the tests locally, and found the DOMContentLoaded stuff was only firing once. That lead to the discovery that the handleEvent() code for DOMContentLoaded was in fact explicitly ignoring from subframes... Mochitests run in an iframe and were thus broken, but most sites don't and so manual testing was ok.

Moving the login manager call to before the subframe check makes things work: Andoird M8 now green. https://tbpl.mozilla.org/?tree=Try&rev=d5ec17db4d84

Comment 31

[Justin Dolske [:Dolske]]

(Huge thanks to Margaret for helping out on this!)

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 758974 [details] [diff] [review]
Fix Metro and Android (v.1)

This patch should also remove these lines:

http://hg.mozilla.org/mozilla-central/annotate/204de5b7e0a6/browser/metro/base/content/browser-ui.js#l139
http://hg.mozilla.org/mozilla-central/annotate/204de5b7e0a6/mobile/android/chrome/content/browser.js#l354

because they're no longer needed, right?

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Oh, and http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1038.

Comment 34

[Justin Dolske [:Dolske]]

That's bug 856470. :-) Could do it here, I guess, just hadn't looked as closely at making sure that removal had no impact.

Comment 35

[Justin Dolske [:Dolske]]

FML. Try pushes are now causing M-oth asserts and crashes in layout/base/tests/chrome/test_printpreview_bug396024.xul. I have no clue how this bug could possibly cause that, unless I'm just managing to tickle some existing JS bug.

(Except on OS X, which seem to be working?)

https://tbpl.mozilla.org/?tree=Try&rev=450d41e18569

Comment 36

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 758974 [details] [diff] [review]
Fix Metro and Android (v.1)

My initial thoughts were to get LoginManagerContent to add it's own listeners, since sprinkling the LoginManagerContent calls in the existing code could be fragile and easy to break.

I'm OK with letting this play out though.

Comment 37

[Justin Dolske [:Dolske]]

Attachment: Workaround for bug 881996

This is gross to do, but it avoids the problem in bug 881996 by putting back the code that already exists today. Hopefully we can fix 881996 and remove this then.

Comment 38

[Justin Dolske [:Dolske]]

I did a Try push with the workaround -- all green.

https://tbpl.mozilla.org/?tree=Try&rev=a1dd725fe967

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/58d5552a5ef4
https://hg.mozilla.org/mozilla-central/rev/f5df5ee7bd17
https://hg.mozilla.org/mozilla-central/rev/1213ea1ff91a

Comment 40

[Julian Fitzell]

I'm the author of the Keychain Services extension, which adds an alternative login storage module that uses the OS X Keychain. I had a user report a few days ago that the extension stopped working with Nightly and after a bit of investigation this seems to be related to the signon.autofillForms config setting being set to false. When it is set to false, no passwords seem to be getting filled in when the extension is enabled (they are still filled when using the default storage module).

This seems to be the only change to login manager, but because of the nature of the diff, I can't easily tell what might have changed. Do you have any ideas?

Sample log output below (load a page with a password form, type in the username "foo" and tab to the password field):

[22:32:55.242] Login Manager: Counting logins matching host: http://localhost formSubmitURL:  httpRealm: null
[22:32:55.277] Login Manager (content): fillDocument processing 1 forms on http://localhost/~julian/
[22:32:55.278] Login Manager (content): _fillDocument processing form[ 0 ]
[22:32:55.278] Login Manager: Searching for logins matching host: http://localhost formSubmitURL: http://localhost httpRealm: null
[22:32:55.314] Login Manager (content): found 1 matching logins.
[22:32:59.866] Login Manager (content): autofillForms=false but form can be filled; notified observers
--
[22:33:23.750] Login Manager: AutoCompleteSearch invoked. Search is: f
[22:33:23.750] Login Manager: Creating new autocomplete search result.
[22:33:23.751] Login Manager: Searching for logins matching host: http://localhost formSubmitURL: http://localhost httpRealm: null
[22:33:23.789] Login Manager: 1 autocomplete logins avail.
[22:33:23.930] Login Manager: AutoCompleteSearch invoked. Search is: fo
[22:33:23.930] Login Manager: Using previous autocomplete result
[22:33:24.097] Login Manager: AutoCompleteSearch invoked. Search is: foo
[22:33:24.098] Login Manager: Using previous autocomplete result
[22:33:24.953] Login Manager (content): onUsernameInput from DOMAutoComplete

Comment 41

[Julian Fitzell]

I just dug around a bit further and it seems like it may be the addition of the a check of a new attribute "isLoggedIn" on the nsILoginStorage interface. Does that seem plausible?

And if so, is that check really necessary? It seems like the nsILoginStorage interface and the current LoginManager implementation are getting increasingly tied to a single storage implementation, which doesn't feel very extensible. I guess I could just return "true" for isLoggedIn all the time, but that doesn't feel very honest.

Couldn't master password logic be encapsulated in the storage modules that use it? Or at very least, that attribute could be named in a more semantic way: areLoginsAvailable, or isActive, or something...

An earlier example of dependence on a particular implementation is the assumption that passwords are available in plaintext: a previous LoginManager change started checking password length on all passwords, which can result in a prompt for all possible entries from the keychain before the user has even chosen an account (the Keychain returns account details without authentication but required permission, possible per-entry, when you access the password).

Happy to carry on this discussion elsewhere, but is there room for relaxing the coupling here?

Comment 42

[Justin Dolske [:Dolske]]

(In reply to Julian Fitzell from comment #41)
> I guess I could just return "true" for isLoggedIn all the
> time, but that doesn't feel very honest.

That's probably your best option. This check is mainly used to avoid triggering repeated master-password prompts, so if your code can't actually do that it shouldn't matter.

Comment 43

[Julian Fitzell]

(In reply to Justin Dolske [:Dolske] from comment #42)
> (In reply to Julian Fitzell from comment #41)
> > I guess I could just return "true" for isLoggedIn all the
> > time, but that doesn't feel very honest.
> 
> That's probably your best option. This check is mainly used to avoid
> triggering repeated master-password prompts, so if your code can't actually
> do that it shouldn't matter.

Hmm... really seems a shame to be changing the interface only to make it *more* tied to the default implementation.

So, yes, I could implement isLoggedIn to always be true, which works ok given the one place it's currently being used. But I have no idea what other users of that attribute might pop up in the future and I have to know what the caller is using it for in order to come up with an answer, which isn't cool.

If a new attribute is really needed—I guess the old approach doesn't work anymore?—couldn't we create one that describes the question you actually need an answer to, namely "are usernames currently available without prompting the user?". I could confidently and unambiguously answer true to that question in all cases (Keychain only encrypts passwords) and it would be future-proof.

On a related note, it would awesome if nsILoginInfo knew whether its password was decrypted. Sometime back code was added to look at the password length when looking for autocomplete matches; this is a real bore for Keychain users because in some cases they can get password prompts for several accounts (sometimes several times) before they've even chosen which one to fill with. If the LoginInfo knew whether it was decrypted we could bypass that check when false.

Again, happy to take the discussion elsewhere or even try to help out, but it seems a shame to make changes to the interface that make it worse for alternative implementations...

Comment 44

[Justin Dolske [:Dolske]]

Please file new bugs for your issues. But I'll note that I don't think it's likely that we'll be making significant changes to support Keychain.