:Felipe Gomes (needinfo for replies!) Reporter
	Description • 11 years ago

Some of the password manager code already runs from a content script, e.g. LoginManagerContent.jsm, thanks to bug 839961.

But that jsm access the storage (nsILoginManager) directly, so it still crosses the process boundary.

A simple implementation would be to stub the calls to that interface to the parent, and get responses. Bug 853549 should help with that since it's making the interface async.

It can be done better than that as some of the steps could be entirely taken care by the parent*, but that requires more refactoring. We should consider how much that is worth it, vs. the direct naive implementation.

Another thing to do is to feed the autocomplete popup with results from the login db vs. the form history db. Hopefully the structure from bug 897061 will help there.

See also: bug 552827


* Here's an example: when a password form is detected, LoginManagerContent.jsm asks for all the existing passwords for that page. If the amount of results is equal to 1, it autofills in advance. If it's greater than 1, it won't autofill, just let the user pick from the popup when typing.
It would be better if the parent did this instead, and only ever sent to the child process the login that is to be used. How much this kind of optimization is important is up in the air.

	Justin Dolske [:Dolske]
	Comment 1 • 11 years ago

Note that bug 853549 isn't actually making the APIs async -- they're still syncronous, but won't trigger main thread I/O (except possibly on the first call).

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 2 • 10 years ago

Update: I've mostly gotten the password manager working (so far I've only tested without a master password, but that should Just Work (TM)). The one big piece remaining is making the autocomplete popup work (unfortunately it's absolutely necessary, otherwise the PM doesn't work at all if you have multiple logins for a site).

Dolske, do you prefer giant rolled-up patches or should I keep things in relatively smaller, separate patches? The splits aren't perfect, but are fairly logical.

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 3 • 10 years ago

Blake: fwiw, autocomplete popups for form autocomplete were hooked up for e10s in bug 897061. I think a lot of that code might be reusable here, though I haven't looked closely to know for sure. If you have any questions about the implementation in bug 897061 let me know!

	Justin Dolske [:Dolske]
	Comment 4 • 10 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #2)
> Update: I've mostly gotten the password manager working (so far I've only
> tested without a master password, but that should Just Work (TM)).

We should talk if it (1) doesn't and (2) is hard to fix. I think MP is on limited life-support at this point.

> Dolske, do you prefer giant rolled-up patches or should I keep things in
> relatively smaller, separate patches? The splits aren't perfect, but are
> fairly logical.

Small patches FTW.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 6 • 10 years ago

This splits the work between the parent and child properly.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 7 • 10 years ago

This makes us properly respect the autocomplete API that lets the user select
from multiple pre-existing logins on a page (or even a single one).

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 9 • 10 years ago

This commit makes us properly fill in forms on page load. It also makes us
find the logins when submitting a form, but that won't work until a later
patch.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 10 • 10 years ago

dolske, please note that attachment 8408543 [details] [diff] [review] should actually be the *first* patch.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 12 • 10 years ago

Update: I'm working through mochitest failures. I've been converting the testsuite to be OK with asynchronous filling of forms. For the most part, this has been straightforward with a couple of tests (I'm looking at *you* test_basic_form_autocomplete.html). I think I'm about 3/4 the way done. I've found three very minor bugs so far in my patches.

One note: these tests are going to need another, pretty major, overhaul for actual e10s. Many of them assume that they can touch the password manager directly and add new logins, etc, which is no longer the case.

	Justin Dolske [:Dolske]
	Comment 13 • 10 years ago

Comment on attachment 8408543 [details] [diff] [review]
Introduce an asynchronous findLogins and use it.

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

I few things that need to be fixed, but hopefully they're all obvious enough that I can avoid needing to look at this ever again, so r+. :)

::: browser/base/content/content.js
@@ +38,5 @@
>    }
>  });
>  
> +addEventListener("DOMFormHasPassword", function(event) {
> +  InsecurePasswordUtils.checkForInsecurePasswords(event.target);

Is InsecurePasswordUtils ready for E10S?

::: browser/components/nsBrowserGlue.js
@@ +505,5 @@
>        ContentClick.init();
>        RemotePrompt.init();
>      }
>  
> +    LoginManagerContent.initParent();

I've grumbled about this before, but we really need a common place (or some kind of manifest thing) to set up basic message listeners, so that we don't have to pull in tons of JS on startup just to do that simple task.

I guess I'll file a bug.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +105,5 @@
> +                   .getService(Ci.nsIMessageSender);
> +        mm.addMessageListener("RemoteLogins:loginsFound", this);
> +    },
> +
> +    initParent: function() {

Hmm, we should... oh, good, a later patch splits up the parent/child code.

@@ +113,5 @@
> +    },
> +
> +    // Map from form login requests to documents.
> +    _requests: Object.create(null),
> +

Plain old |{}| should be fine here too? The only properties we set are the uuids, so the subtle different in prototypes shouldn't matter? Or does it?

(I suppose it's fine as-is, just wondering if there's a specific reason for this.)

@@ +161,5 @@
> +      messageManager.sendAsyncMessage("RemoteLogins:findLogins",
> +                                      { formOrigin: formOrigin,
> +                                        actionOrigin: actionOrigin,
> +                                        showMasterPassword: showMasterPassword,
> +                                        cookie: requestid });

Unless we're consistently using "cookie" elsewhere like this for E10S, I think it causes ambiguity with The Other Kind Of Cookie. Just "requestID"?

@@ +164,5 @@
> +                                        showMasterPassword: showMasterPassword,
> +                                        cookie: requestid });
> +      let deferred = Promise.defer();
> +      this._requests[requestid] = { form: form, // XXX weak?
> +                                    promise: deferred };

The "XXX weak" is exactly what I was wondering too. :)

Can you just use a WeakMap here? Seems like it would be a trivial drop-in replacement for _requests.

In theory, I think even a strong reference would mostly be ok... The master password dialog is window modal (and so kind of hard to ignore), and pathological disk access seem to be the old other source of delay. So I'd suspect the more likely scenario is actually some bug causing us to not clear out the map entry (and thus leaking), so WeakMap still seems like a good safeguard. :)

@@ +177,5 @@
>            return;
>  
>        let form = event.target;
> +      log("onFormPassword for", form.ownerDocument.documentURI);
> +      this._asyncFindLogins(form)

This was is a little confusing, until I saw the later patch setting the 2nd |showMasterPassword| arg here to true.

@@ +185,5 @@
>  
> +    childLoginsFound: function({ form, loginsFound }) {
> +        // If we didn't find any logins, then we can stop doing anything now.
> +        if (loginsFound === null)
> +            return;

Can we instead make this just be an empty array instead of null (when there are no logins).

@@ +209,5 @@
>  
>        // If we're currently displaying a master password prompt, defer
>        // processing this form until the user handles the prompt.
>        if (Services.logins.uiBusy) {
>          log("deferring onFormPassword for", doc.documentURI);

s/doc.documentURI/formOrigin

@@ +237,5 @@
>          // a dummy event listener (a strong reference) to keep it alive
>          // until the form is destroyed.
>          Services.obs.addObserver(observer, "passwordmgr-crypto-login", true);
>          Services.obs.addObserver(observer, "passwordmgr-crypto-loginCanceled", true);
> +        //form.addEventListener("mozCleverClosureHack", observer); // XXX XXX XXX

Blech. I don't see an easy replacement for this, since that would need some mechanism for the parent to know about the lifetime of a node (or maybe document?) in the child. I don't know if such a thing exists, and I suspect it would be hard to create.

I was going to say just make it strong and don't worry about it, since one of these two notifications should always fire. Except that's not strictly true. :(

These don't get fired by the actual master password prompt, but rather by login manager itself (crypto-SDR.js). In practice that's not an important distinction, since generally login manager is the only thing triggering this. But it's possible -- if a user has a personal certificate protected by the MP, and starts to use it just before triggering this code path, their MP entry/cancellation won't result in these notifications.

I think we should do this:

1) Make the addObserver calls here strong
2) Remove the hack and comments about it
3) hold your nose at the (unlikely) potential for leaks
4) file a bug to move these notifications into PSM
5) fix said bug

I mainly didn't do 4-5 when implementing bug 499233 due to PSM's reputation for being an immutable blackhole of despair, but I think that's no longer quite so true.

	Justin Dolske [:Dolske]
	Comment 14 • 10 years ago

Comment on attachment 8408538 [details] [diff] [review]
Properly save passwords when we submit a form

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

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +164,5 @@
>  
> +      let messageManager = messageManagerFromWindow(win);
> +
> +      // TODO Only do this once.
> +      messageManager.addMessageListener("RemoteLogins:loginsFound", this);

Err, I'm confused. The first patch added this in initChild() (as well as again here), and now this patch is deleting initChild().

@@ +291,5 @@
>          // same field as the autocomplete was activated on.
>          var [usernameField, passwordField, ignored] =
>              this._getFormFields(acForm, false);
>          if (usernameField == acInputField && passwordField) {
> +            this._asyncFindLogins(acForm, { showerMasterPassword: false })

I do not recommend submerging the master password in water.

@@ +553,5 @@
> +
> +    parentOnFormSubmit: function(hostname, formSubmitURL,
> +                                 usernameField, newPasswordField,
> +                                 target) {
> +        // For E10S this will need to move.

It has, remove. :)

@@ +560,5 @@
> +                              createInstance(Ci.nsILoginManagerPrompter);
> +            // XXX For E10S, we don't want to use the browser's contentWindow
> +            // because it's in another process, so we use our chrome window as
> +            // the window parent (the content process is responsible for
> +            // making sure that its window is not in private browsing mode).

Has Ehsan weighed on this? AIUI we were trying to keep the private browsing stuff tied to the content window, so that add-ons could enable a per-tab private browsing mode.

@@ +564,5 @@
> +            // making sure that its window is not in private browsing mode).
> +            // In the same-process case, we can simply use the content window.
> +            prompterSvc.init(target.isRemoteBrowser ?
> +                                target.ownerDocument.defaultView :
> +                                target.contentWindow);

I don't understand this, and in part therefore don't see how this can work.

In E10S, what is |target.ownerDocument|? browser.xul in the parent?

The prompting code currently relies on getting a content window, so it can use that to set up the tab-specific doorhanger / notification bar.

_getNotifyWindow() also wants to use contentWindow.top.opener to deal with a special case of transient popup windows asking for passwords.

[Note that there's a lot of crufty code in nsPrompter, retaining backwards-compat with Thunderbird and Seamonkey. :( I think there are only a couple cases where Firefox will trigger a modal window dialog for password manager, and the notification-bar code should be entirely unused in Firefox. But the doorhanger code is obviously still alive.)

@@ +664,5 @@
>          }
>  
>  
>          // Prompt user to save login (via dialog or notification bar)
> +        prompter = getPrompter();

There are 2 other getPrompter() calls here to adjust.

	Justin Dolske [:Dolske]
	Comment 15 • 10 years ago

Comment on attachment 8408540 [details] [diff] [review]
Split the parent and child portions of LoginManagerContent into separate objects.

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

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +this.EXPORTED_SYMBOLS = [ "LoginManagerContentParent",
> +                          "LoginManagerContent",

Seems like this should probably just split into LoginManagerParent.jsm and LoginManagerChild.jsm.

"ContentParent" seems like a bit of an oxymoron, and I'm concerned about the potential of bugs if someone doesn't realize the same code is loaded in each.

For example, as-is the shared code is adding an earlyformsubmit observer in both parent and child, which seems wrong.

This does mean the (trivial) pref observer and log() is duplicated in two files, but I think that's ok.

@@ +466,4 @@
>            .then(null, Cu.reportError);
>      },
>  
> +    loginsFound: function({ form, loginsFound }) {

Other than this trivial rename, are the other changes here just big cut'n'paste moves?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 16 • 10 years ago

(In reply to Justin Dolske [:Dolske] from comment #13)
> I've grumbled about this before, but we really need a common place (or some
> kind of manifest thing) to set up basic message listeners, so that we don't
> have to pull in tons of JS on startup just to do that simple task.

We have the message wakeup service:
content/base/src/messageWakeupService.js

I suppose it's only usable by XPCOM components at the moment. Tweaking it to support JSMs seems worthwhile!

	Justin Dolske [:Dolske]
	Comment 17 • 10 years ago

(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #16)
> (In reply to Justin Dolske [:Dolske] from comment #13)
> > I've grumbled about this before, but we really need a common place (or some
> > kind of manifest thing) to set up basic message listeners, so that we don't
> > have to pull in tons of JS on startup just to do that simple task.
> 
> We have the message wakeup service:
> content/base/src/messageWakeupService.js

Ah, yes, that's exactly what I was thinking of for "some kind of manifest thing".

I'd sort of want to lean towards simplicity (vs crufty manifest magic), and have a simple chrome equivalent of content.js (chrome.js?!) that just adds a bunch of message listeners in the obvious manner and demultiplexes them out to the various implementations... OTOH content.js is already becoming a cluttered dumping ground, so maybe that's not a great model to follow.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 18 • 10 years ago

A new message-listener.js content script that just invokes the right JSMs sounds good to me. You should file that bug :)

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 19 • 10 years ago

(In reply to Justin Dolske [:Dolske] from comment #13)
> Plain old |{}| should be fine here too? The only properties we set are the
> uuids, so the subtle different in prototypes shouldn't matter? Or does it?
> 
> (I suppose it's fine as-is, just wondering if there's a specific reason for
> this.)

I could use {}, but Object.create(null) (I believe) makes an object in dictionary mode to begin with and this says "map" a little more clearly to me. Actually, I just found out that there's a Map built in to JS now. Would you prefer that? (It even has a Map.delete function, avoiding the weird JS delete operator).

> Can you just use a WeakMap here? Seems like it would be a trivial drop-in
> replacement for _requests.

I don't think WeakMaps work the way they need to in order to be useful. A WeakMap weakly holds onto its *keys* and I need to use the requestID as the key. I'm willing to take the risk of this "leaking" :)

> I mainly didn't do 4-5 when implementing bug 499233 due to PSM's reputation
> for being an immutable blackhole of despair, but I think that's no longer
> quite so true.

I'll file a followup bug.

(In reply to Justin Dolske [:Dolske] from comment #14)
> Err, I'm confused. The first patch added this in initChild() (as well as
> again here), and now this patch is deleting initChild().

Sorry, about that. The initChild removal should have happened in the first patch (and therefore wouldn't have appeared in any diffs at all). I have a patch here that gets rid of the TODO, as well.

> I do not recommend submerging the master password in water.

That explains the Gremlin infestation I've been dealing with :-/

> Has Ehsan weighed on this? AIUI we were trying to keep the private browsing
> stuff tied to the content window, so that add-ons could enable a per-tab
> private browsing mode.

I'll talk to him, but the private browsing stuff *is* tied to the content window. The problem that I'm dealing with here is that the content window lives in the content process and therefore I can't do any of those checks in the parent. Fortunately, the child process *can* and is able to control the control flow well enough without additional checks in the parent (if we need to pass that information around, we can do so.

> @@ +564,5 @@
> I don't understand this, and in part therefore don't see how this can work.
> 
> In E10S, what is |target.ownerDocument|? browser.xul in the parent?
> 
> The prompting code currently relies on getting a content window, so it can
> use that to set up the tab-specific doorhanger / notification bar.

If we can, I'd really like to avoid fixing this here. We haven't implemented doorhangers in e10s and they're going to have to deal (somehow) with not having a handle to the content window in the parent. The way that we tend to deal with that is to pass browsers around instead of parents. Passing in the chrome window allows the existing code to simply pop up an old-style modal dialog (which works well enough, even if it's quite old-fashioned) and gives us time to fix the doorhangers properly. When we do that, we'll be able to pass in target (probably) in the e10s case and have the doorhanger code deal with it properly.

> But the doorhanger code is obviously still alive.)

Except in e10s.

(In reply to Justin Dolske [:Dolske] from comment #15)
> Seems like this should probably just split into LoginManagerParent.jsm and
> LoginManagerChild.jsm.

Sure, I can do that. Conceptually the two are close enough that it seems OK to me to combine them (and it isn't that much code bloat in either the parent or the child), but I don't feel strongly about it.

> "ContentParent" seems like a bit of an oxymoron, and I'm concerned about the
> potential of bugs if someone doesn't realize the same code is loaded in each.

Suggestions welcome :-) I took a straw poll at the office for better names and nobody had anything. This code is the "part of the login manager that deals with content, but that runs in the parent." I can add comments or change the name, but I really don't have a better offering.

> For example, as-is the shared code is adding an earlyformsubmit observer in
> both parent and child, which seems wrong.

Well "sharing" isn't quite the right word. We need earlyformsubmit observers in both the parent and child though because the password manager works in chrome pages which currently still run in the parent. 

> Other than this trivial rename, are the other changes here just big
> cut'n'paste moves?

Yep.

Unfortunately, merging this patch across the new changes is going to be a massive pain. I'm going to squash most of the changes to LoginManagerContent.js/nsLoginManager.js and attach patches for the mochitests that I had to fix as well. Note that I'm working on fixing a leak somewhere in my patches. The combined patch isn't that terrible (surprisingly), so it shouldn't be too bad to review.

	Justin Dolske [:Dolske]
	Comment 20 • 10 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #19)

> I could use {}, but Object.create(null) (I believe) makes an object in
> dictionary mode to begin with and this says "map" a little more clearly to
> me. Actually, I just found out that there's a Map built in to JS now. Would
> you prefer that? (It even has a Map.delete function, avoiding the weird JS
> delete operator).

Either way, but Map seems better.

> > Can you just use a WeakMap here? Seems like it would be a trivial drop-in
> > replacement for _requests.
> 
> I don't think WeakMaps work the way they need to in order to be useful. A
> WeakMap weakly holds onto its *keys* and I need to use the requestID as the
> key. I'm willing to take the risk of this "leaking" :)

Oh, bah. I had even reread the article. Now I remember this being the dumb thing that works backwards from what one usually wants. :(


> > @@ +564,5 @@
> > I don't understand this, and in part therefore don't see how this can work.
> > 
> > In E10S, what is |target.ownerDocument|? browser.xul in the parent?
> > 
> > The prompting code currently relies on getting a content window, so it can
> > use that to set up the tab-specific doorhanger / notification bar.
> 
> If we can, I'd really like to avoid fixing this here. We haven't implemented
> doorhangers in e10s

Ok. Can this be left more-or-less unchanged? I'd rather leave a "TODO fix for e10s in bug xxx" than change this to something that still doesn't actually work.


> (In reply to Justin Dolske [:Dolske] from comment #15)
> > Seems like this should probably just split into LoginManagerParent.jsm and
> > LoginManagerChild.jsm.
> 
> Sure, I can do that. Conceptually the two are close enough that it seems OK
> to me to combine them (and it isn't that much code bloat in either the
> parent or the child), but I don't feel strongly about it.
> 
> > "ContentParent" seems like a bit of an oxymoron, and I'm concerned about the
> > potential of bugs if someone doesn't realize the same code is loaded in each.
> 
> Suggestions welcome :-) I took a straw poll at the office for better names
> and nobody had anything. This code is the "part of the login manager that
> deals with content, but that runs in the parent."

Probably not a useful distinction, the "these bits are content specific" split was mostly just done to help split things up with an eye towards e10s coming, and it was previously pretty well isolated to a single module of code.

I don't care deeply about the name, LoginManagerParent.jsm seems straightforward enough. Really it could just be LoginManager.jsm.

> > For example, as-is the shared code is adding an earlyformsubmit observer in
> > both parent and child, which seems wrong.
> 
> Well "sharing" isn't quite the right word. We need earlyformsubmit observers
> in both the parent and child though because the password manager works in
> chrome pages which currently still run in the parent.

"Need"? :) I'm not sure how useful that is or if it's being used by addons, but I'd have assumed anything with chrome privs should be using the password manager API directly. (Related, this ended up causing a problem for sync: bug 981941).

It would also remove the need to have tests (!) that ensure all this stuff works when run in both modes...

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 21 • 10 years ago

(In reply to Justin Dolske [:Dolske] from comment #20)
> Ok. Can this be left more-or-less unchanged? I'd rather leave a "TODO fix
> for e10s in bug xxx" than change this to something that still doesn't
> actually work.

Well, we can't leave it as 'target.contentWindow' because that's a CPOW and we need a real window in the code. The chrome window is really the only window around that we can use (in addition, it's the most correct in terms of parenting the modal dialog that we'll end up with).

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 22 • 10 years ago

This should be the first 5 patches in this bug, but as a single patch.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 24 • 10 years ago

This patch fixes all but one of the tests (and I used setTimeout for two of them, I can fix that if need be, I guess). The last test deserves its own patch, since it's gigantic and I changed it rather a lot.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 25 • 10 years ago

I have a try run at https://tbpl.mozilla.org/?tree=Try&rev=188f0c3c8644

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 26 • 10 years ago

Also note that I plan to land the 3 last patches as a single patch (since they don't stand alone).

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 27 • 10 years ago

This is the last test that needed fixing. I also fixed a couple of dump statements that crept in from other patches (that have already landed).

This applies on top of the other patches in this bug.

	Justin Dolske [:Dolske]
	Comment 30 • 10 years ago

Comment on attachment 8427889 [details] [diff] [review]
Final test fix

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

::: toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html
@@ +218,2 @@
>   */
> +function* runTest() {

"function*"?

@@ +249,5 @@
> +  // happen, but where if it did it would do so asynchronously. It isn't
> +  // perfect, but it's better than nothing.
> +  function spinEventLoop() {
> +    setTimeout(function() { tester.next(); }, 0);
> +  }

:-/

@@ +260,5 @@
> +    SpecialPowers.addObserver(observer, "passwordmgr-found-logins", false);
> +  }
> +
> +  // XXX The switch here is an artifact of the old way this test was written.
> +  switch(1) {

Please make this a plain block (or remove it and fix the indenting if you're so inclined), and regex/replace the "case #:" to "/* test # */". The dead cases are a little too crufty for me, and will confuse someone eventually :)

	Justin Dolske [:Dolske]
	Comment 31 • 10 years ago

Before checkin, I think it would probably be a good idea to kick off a Try run and have it repeat the password manager tests being modified here 10 or 20 times.  Just to help flush out if any rando-orange happens with the changes here.

	Justin Dolske [:Dolske]
	Comment 32 • 10 years ago

Comment on attachment 8427887 [details] [diff] [review]
This patch addresses dolske's comments that I haven't already responded to in comments above.

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

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +138,5 @@
> +                                    requestId, target);
> +                },
> +                handleEvent: function (event) {
> +                    // Not expected to be called
> +                }

I think the handleEvent is dead now. (Leftover from cleverClosureHack thing that was here before)

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +122,2 @@
>  
> +        // TODO: Make this class useful in the child process (in addition to

File and note a bug # here?

IIRC, passwordmgr-storage-replace is only used for testing, so that won't matter much. I guess you're asking for nsILoginManager to become usable by addons running in the child process?

@@ +424,5 @@
>       * auto-complete provider, and not have satchel calling pwmgr directly.
>       */
>      autoCompleteSearchAsync : function (aSearchString, aPreviousResult,
>                                          aElement, aCallback) {
> +        // aPreviousResult are nsIAutoCompleteResult, aElement is

s/are/is/ :)

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 33 • 10 years ago

(In reply to Justin Dolske [:Dolske] from comment #30)
> "function*"?

See <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*> "function*" basically means "this function is a generator".

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 34 • 10 years ago

One last try push with all comments addressed: https://tbpl.mozilla.org/?tree=Try&rev=4ac5cf78d98f

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 35 • 10 years ago

Remember when I said "last test?" This one snuck in before checkin. This test was based on the test_basic_form_autocomplete.html test, so I've cribbed the changes from that mold. This should be a quick stamp based on that. I am happy to attach a -w if needed or anything really to get this in sooner rather than later.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 36 • 10 years ago

My last try push found the need for attachment 8444675 [details] [diff] [review] and bug 1029128 slowed me down while writing that patch. A new try push with all fixes can be found at <https://tbpl.mozilla.org/?tree=Try&rev=54243e803de6>. Note that this patch can land without the patches in bug 1029128 so I might do that based on the order of reviews.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 37 • 10 years ago

Comment on attachment 8444675 [details] [diff] [review]
fix this test too

Whoever gets to this first should stamp. So close to landing.

Ehsan, note the green try run from the link in comment 36 and that this patch is basically the same patch as attachment 8427889 [details] [diff] [review], but much smaller and simpler (it would be good to common-out some of the common logic between them, but I really would like to wait for a separate bug to do that).

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 38 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/daa9c46d19dc

(with ehsan's blessing)

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 10 years ago

Something in this push made Android sadface. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f291b1b47c7

https://tbpl.mozilla.org/php/getParsedLog.php?id=42378026&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=42378331&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=42377296&tree=Mozilla-Inbound

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 40 • 10 years ago

The saga continues. blassey tested this patch and said that things appear to work.

I've thrown in a completely untested metro patch for free here. jimm, let me know if you want me to forgo landing this until someone tests it.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 41 • 10 years ago

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

	(no longer active)
	Comment 42 • 10 years ago

Comment on attachment 8444675 [details] [diff] [review]
fix this test too

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

Looks good!

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 43 • 10 years ago

Comment on attachment 8445438 [details] [diff] [review]
Fix Android and maybe metro.

Android part looks OK. I do wonder how these new LoginManager JSMs affect the old XPCOM component which gets instantiated here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#400

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 44 • 10 years ago

Take two: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1ee05fbbd6

I took the backout opportunity to move a hunk from this patch into the patch for bug 993197, where it belonged.

mfinkle, the LoginManagerContent stuff and nsLoginManager play nicely together. I'm about to file a followup bug to make nsLoginManager work properly in content processes with e10s enabled.

	Carsten Book [:Tomcat]
	Comment 45 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8c1ee05fbbd6

	Carsten Book [:Tomcat]
	Comment 46 • 10 years ago

sorry had to back this out from mozilla-central (and so with the merge in the integration trees) in https://tbpl.mozilla.org/?rev=83f9b25520bf since one of this checkins seems to have caused Bug 1030663 which is a frequent test failure now

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 47 • 10 years ago

I suppose it's completely mandatory to back out the whole patchset for one flapping test rather than just to disable the test temporarily?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 48 • 10 years ago

This wasn't implicated in the backout, but it's now possible to fix this, so why not.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 49 • 10 years ago

This got really easy to fix since the last time I had to touch it.

	Mike Conley (:mconley) (:⚙️)
	Comment 50 • 10 years ago

Comment on attachment 8447324 [details] [diff] [review]
Fix the pwevent test

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

Switching to event-listening over waiting some magical amount of time? I sure like the sound of that.

	Mike Conley (:mconley) (:⚙️)
	Comment 51 • 10 years ago

Comment on attachment 8447323 [details] [diff] [review]
Fix the master password tests

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

This looks fine - just curious about the event listener in subst_master_pass.html. Withholding r+ until I hear more about that, and the style-fixup in test_master_password.html.

::: toolkit/components/passwordmgr/test/subtst_master_pass.html
@@ +3,5 @@
>  <form>
> +    <script>
> +        // Only notify when we fill in the password field.
> +        var count = 2;
> +        document.forms[0].addEventListener("input", function() {

Why can't we just put the listener on the password field directly?

::: toolkit/components/passwordmgr/test/test_master_password.html
@@ +54,5 @@
> +// filling in the password in the subtest (after we dismiss the master
> +// password dialog). In order to accomplish this, the test waits for an event
> +// and then posts a message back up to us telling us to continue.
> +var continuation = null;
> +addEventListener("message", function() {

Let's fix up the indentation here.

Something like:

addEventListener("message", () => {
    if (continuation) {
        var c = continuation;
        continuation = null;
        c();
    }
});

Fat arrow syntax optional.

I think it'd be better in general to have this suite of tests use add_task and Promises instead of callback chaining. I think that'd help with readability and maintainability. However, that's certainly outside of the scope of this bug, so I won't harp on it. :) Maybe good follow-up bug (or good first bug) fodder though?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 52 • 10 years ago

I don't know why my first version with the event listener on the password field didn't work. This does work.

	Mike Conley (:mconley) (:⚙️)
	Comment 53 • 10 years ago

Comment on attachment 8447418 [details] [diff] [review]
Master password test fixed up

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

LGTM!

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 54 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f1356e9b56d

Thanks for the extra reviews.

	Phil Ringnalda (:philor)
	Comment 55 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/2f1356e9b56d

	neil@parkwaycc.co.uk
	Comment 56 • 10 years ago

Comment on attachment 8445438 [details] [diff] [review]
Fix Android and maybe metro.

If only there was some way of automatically registering for startup notifications so that you didn't have to copy and paste for each app...

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 57 • 10 years ago

(In reply to neil@parkwaycc.co.uk from comment #56)
> If only there was some way of automatically registering for startup
> notifications so that you didn't have to copy and paste for each app...

Can you file a bug on fixing this, rather than just commenting in a resolved bug?

	neil@parkwaycc.co.uk
	Comment 58 • 10 years ago

(In reply to :Gavin Sharp from comment #57)
> Can you file a bug on fixing this
Sure, but it turns out we'd need to fix bug 930456 first anyway.

	neil@parkwaycc.co.uk
	Comment 59 • 9 years ago

(In reply to comment #58)
> (In reply to Gavin Sharp from comment #57)
> > Can you file a bug on fixing this
> Sure, but it turns out we'd need to fix bug 930456 first anyway.
I notice that's now fixed, so I filed bug 1153862.