552827 - fennec - login manager - e10s

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Doug Turner (:dougt)]

Investigate how the login manager will work in e10s.

Comment 1

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

I started to work on a patch. My plan is to create a remote frame script and move any and all code that directly touches the DOM into the script. Then, use some messages to communicate between chrome and content.

A significant portion of the code in nsLoginManager.js will get moved into the remote script. Since the DOM code would still be needed for non-ipc content, nsLoginManager would Cu.import the script and share the code.

Comment 2

[Justin Dolske [:Dolske]]

Attachment: Patch for content-driven pwmgr (WIP)

I started working on part of this a few weeks ago (but set it aside, and now it's bitrotted). The basic idea here is to have content drive password manager, when password fields are added to the DOM, instead of having pwmgr watch each document load and scan it for forms. This would be the first step towards an E10S-ready pwmgr, and also has the nice side effect of bug 355063 (eg, at one point I has this patch filling in Digg logins, which doesn't work with the current pwmgr).

Instead of moving large chucks of pwmgr into content, I suspect it would be better to take what's started here, and modify the way the observer works so the (IPC) notification from content has what's needed for pwmgr to fill the form. Or some hybrid that minimizes the code that needs to run in the content process.

Comment 3

[Justin Dolske [:Dolske]]

Oh, I should mention that I know the content pieces in that patch are not quite right. Need to talk with sicking to better understand how/when stuff gets inserted into the DOM.

Comment 4

[Benjamin Stover (:stechz)]

What's the status on this?

Comment 5

[Benjamin Stover (:stechz)]

Attachment: WIP e10s bits

This patch uses messageManager to fill out password forms and get notifications.  What's missing:
* This does not fill in forms in iframes!  When content sends an async message to find the passwords, the parent send back a message as a map from form actions to logins.  By that point, we no longer have the document we were looking at.  We might have to add an individual message listener for each password request, and make sure that it is always answered?  Or maybe we iterate through all subdocuments (ugh)?
* fillForm no longer does its job of filling out forms.  Need to make a shared module for both processes to use here.

Comment 6

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

I think you should use sendSyncMessage in _askForPasswords. That way you can return the logins right there and you still have the document. You could change the name of the message from "PasswordMgr:FormActionOrigins" to "PasswordMgr:Passwords". You would then drop the existing "PasswordMgr:Passwords".

I think sync would be the right choice here and would not hurt performance.

Comment 7

[Benjamin Stover (:stechz)]

Attachment: v2

Latest work fixes all the aforementioned problems.  There is currently a memory leak, but I haven't tracked it down yet.

Comment 8

[Benjamin Stover (:stechz)]

Comment on attachment 457731 [details] [diff] [review]
WIP e10s bits

(sorry for spam, marked wrong patch obsolete)

Comment 9

[Benjamin Stover (:stechz)]

Fixed the memory leak, but here's some new problems I'm running into on try.  Could be related to message manager?


###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279848970.1279854727.25748.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279843875.1279849857.8025.gz
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279854624.1279855241.28100.gz (only shows up in full log)


###!!! [Parent][RPCChannel] Error: Channel error: cannot send/recv

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1279862368.1279863456.28296.gz
http://tinderbox.mozilla.org/showlog.cgi?tree=MozillaTry&errorparser=unittest&logfile=1279853510.1279856007.30977.gz&buildtime=1279853510&buildname=WINNT%205.2%20tryserver%20debug%20test%20mochitest-other&fulltext= (only shows up in full log)

Comment 10

[Benjamin Stover (:stechz)]

Attachment: Latest patch

Here's the patch I pushed to try server above.

Comment 11

[Benjamin Stover (:stechz)]

The above failures all seem to be tied to layout.  After trying again, here are my failures:

Mochitest debug 5:
* http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280168230.1280168899.24734.gz
* http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280173007.1280174263.15085.gz
* http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280174100.1280175698.20471.gz
* http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280177883.1280179091.1641.gz

Linux debug:
* http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280166237.1280168389.22609.gz

Mochitest other:
* http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280173145.1280177723.28495.gz
* http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280177625.1280180158.6346.gz

I don't think others are seeing these problems from a random orange quicksearch, so I'm not sure what to do about them.

Comment 12

[Benjamin Stover (:stechz)]

Attachment: Patch with test fix

Oddly, there's a microformat mochitest that seems to keep its entire page around for the rest of the browser's lifetime, but only with this patch.  Running this through try as we speak.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 461086 [details] [diff] [review]
Patch with test fix

>diff --git a/toolkit/components/microformats/src/Microformats.js b/toolkit/components/microformats/src/Microformats.js
>--- a/toolkit/components/microformats/src/Microformats.js
>+++ b/toolkit/components/microformats/src/Microformats.js
>@@ -307,16 +307,26 @@ var Microformats = {
>       Microformats.list.push(microformat);
>     }
>     Microformats[microformat] = microformatDefinition;
>     microformatDefinition.mfObject.prototype.debug =
>       function(microformatObject) {
>         return Microformats.debug(microformatObject)
>       };
>   },
>+  remove: function remove(microformat) {
>+    if (Microformats[microformat]) {
>+      var list = Microformats.list;
>+      var index = list.indexOf(microformat, 1);
>+      if (index != -1) {
>+        list.splice(index, 1);
>+      }
>+      delete Microformats[microformat];
>+    }
>+  },
>   /* All parser specific functions are contained in this object */
>   parser: {
>     /**
>      * Uses the microformat patterns to decide what the correct text for a
>      * given microformat property is. This includes looking at things like
>      * abbr, img/alt, area/alt and value excerpting.
>      *
>      * @param  propnode   The DOMNode to check
>diff --git a/toolkit/components/microformats/tests/test_Microformats_add.html b/toolkit/components/microformats/tests/test_Microformats_add.html
>--- a/toolkit/components/microformats/tests/test_Microformats_add.html
>+++ b/toolkit/components/microformats/tests/test_Microformats_add.html
>@@ -225,13 +225,15 @@
> 
>     ok(mf.length, 1, "Check that test1 is a valid microformat");
> 
>     // Verify that the version 2 microformat is now also considered valid
>     var mf2 = Microformats.get("hTest", document.getElementById("test2"), {});
> 
>     ok(mf2.length, 1, "Check that the mfTest microformat version 2 is now valid");
>     doTest3_4_and5(true);
>+
>+    Microformats.remove("hTest");
>   }
> 

could you put the above in another bug/patch.


>diff --git a/toolkit/components/passwordmgr/jar.mn b/toolkit/components/passwordmgr/jar.mn
>--- a/toolkit/components/passwordmgr/jar.mn
>+++ b/toolkit/components/passwordmgr/jar.mn
>@@ -1,7 +1,8 @@
> toolkit.jar:
> %   content passwordmgr %content/passwordmgr/
> *   content/passwordmgr/passwordManager.xul            (content/passwordManager.xul)
> *   content/passwordmgr/passwordManager.js             (content/passwordManager.js)
> *   content/passwordmgr/passwordManagerExceptions.js   (content/passwordManagerExceptions.js)
> *   content/passwordmgr/passwordManagerExceptions.xul  (content/passwordManagerExceptions.xul)
> *   content/passwordmgr/passwordManagerCommon.js       (content/passwordManagerCommon.js)
>+*   content/passwordmgr/child.js              (src/child.js)

White space.  Line up (src/child.js)


>+ * The Initial Developer of the Original Code is Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2007
>+ * the Initial Developer. All Rights Reserved.

2010 (and other places in this patch)

>+    /* ---------- private members ---------- */
>+
>+
>+    get _formFillService() {

one space

>+    },
>+
>+
>+    /* ---------- Utility objects ---------- */
>+
>+
>+    /*

one whitespace please.  and throughout this file.

There is lots and lots of copy and paste going on (from a .js file to the module).  hard to know what is really new code and what is just moved.

Comment 14

[Benjamin Stover (:stechz)]

Attachment: WIP patch that fixes tests

Unfortunately preference changes and submits may not happen in the correct order, as try server has informed me.  This seems to fix the problem by checking the preference in the parent process.   It's running on try now.

Dolske: this needs to be done more cleanly, but are you okay with this hack?  We could add a TODO comment and file a bug to figure out exactly why preference changes are being received before the submit.

Comment 15

[Benjamin Stover (:stechz)]

^ So I've run it on try server, but I'm still seeing:

6317 INFO TEST-PASS | /tests/toolkit/components/satchel/test/test_form_submission.html | form 20 submitted
6318 INFO TEST-PASS | /tests/toolkit/components/satchel/test/test_form_submission.html | checking for empty storage
6319 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_submission.html | checking if saved form data was expected
6320 INFO TEST-PASS | /tests/toolkit/components/satchel/test/test_form_submission.html | submitting iframe test 21
6321 INFO TEST-PASS | /tests/toolkit/components/satchel/test/test_form_submission.html | form 21 submitted
6322 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_submission.html | checking for empty storage
6323 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/satchel/test/test_form_submission.html | checking if saved form data was expected

(http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1281037785.1281038501.3280.gz)

It fixes my local build, but not try for some crazy reason :(

Comment 16

[Benjamin Stover (:stechz)]

Comment on attachment 463004 [details] [diff] [review]
WIP patch that fixes tests

Ugh, posted this on the wrong bug :(

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 461086 [details] [diff] [review]
Patch with test fix

I'm skimming over a lot of the code that looks like it was probably cut moved from one place to another, will take a closer look in the next patch to ensure I didn't overlook any subtle changes.


>+++ b/toolkit/components/microformats/src/Microformats.js
...
>+++ b/toolkit/components/microformats/tests/test_Microformats_add.html

Unrelated, remove.


>+++ b/toolkit/components/passwordmgr/jar.mn
...
>+*   content/passwordmgr/child.js              (src/child.js)

As with form manager, this needs to be a less generic name.


>+++ b/toolkit/components/passwordmgr/src/PasswordUtils.jsm

I don't really understand why stuff is being spun off into this jsm... Except for a small about of code that seems reasonable to share between the chrome and content process (eg .enabled and .log), I'd expect all of this to live exclusively in the content process.

How is .fillForm even working? It's not remoting anything from chrome to content. (Are CPOWs back and working? Thought they got removed?)


>+    _notifyFoundLogins : function (didntFillReason, usernameField,
>+                                   passwordField, foundLogins, selectedLogin) {
...
>+        formInfo.setPropertyAsInterface("usernameField", usernameField);
>+        formInfo.setPropertyAsInterface("passwordField", passwordField);

Ugh. We're probably going to need to rethink this to avoid passing DOM nodes between processes.

Maybe the observer service needs a way to only send notifications to the current process? Things wanting to use these notifications would probably want to be frame scripts, anyway.


>+    _attachToInput : function (element, inputListener) {
...
>+        this._formFillService.markAsLoginManagerField(element);

This seems like it will be a multiprocess headache too. Oh, I bet this doesn't work on Fennec but you don't notice because it's not using the usual autocomplete stuff?


>+++ b/toolkit/components/passwordmgr/src/nsLoginManager.js


>     QueryInterface : XPCOMUtils.generateQI([Ci.nsILoginManager,
>+                                            Ci.nsIDOMEventListener,
>+                                            Ci.nsIObserver,
>                                             Ci.nsISupportsWeakReference]),

Seems this object is no longer an observer, so nsIObserver and nsISWR can go away. I think nsIDOMEventListener technically isn't needed, might as well pass an explicit


>+    receiveMessage: function (message) {

This method really needs to be split up, say getRemoteLogins() + onRemoteFormSubmit().


>+                // Since prompt services ask for window, we try to get the document
>+                // window before resorting to the chrome window.  When Firefox is
>+                // e10sified, prompt service will need to take browser elements.
>+                var win = message.target.contentWindow || message.target.ownerDocument.defaultView;

I don't quite understand the comment, or why you'd need an || conditional there. All the prompt stuff wants the content DOM window, never a chrome window.

>-            } else if (topic == "xpcom-shutdown") {
>-                for (let i in this._pwmgr) {
>-                  try {
>-                    this._pwmgr[i] = null;
>-                  } catch(ex) {}
>-                }
>-                this._pwmgr = null;
>-            }

If this is being removed, do the tests pass without leaks on tryserver? (At one time this was needed, though I wouldn't be surprised if it's ok with it now.)


>+    handleEvent : function (event) {
...
>+        switch (event.type) {
>+            case "DOMAutoComplete":
>+            case "blur":
>+                var acInputField = event.target;
>+                var acForm = acInputField.form;

Shouldn't this all be in the content process instead?


>     /*
>+     * fillForm
>+     *
>+     * Fill the form with login information if we can find it.
>+     */
>+    fillForm : function (form) {

Hmm. Seems like in an E10S world this needs to be handled by the content process, presumably to be invoked by another frame script. Sure would be handy to have the notion of content-process-only components. Can we do anything like that? Or perhaps, some sort of hack with this-process-only notifications and implement this API as an observer in the content process?


>     autoCompleteSearch : function (aSearchString, aPreviousResult, aElement) {
...
>         } else {
>             this.log("Creating new autocomplete search result.");
> 
>             var doc = aElement.ownerDocument;
>-            var origin = this._getPasswordOrigin(doc.documentURI);
>-            var actionOrigin = this._getActionOrigin(aElement.form);
>+            var origin = passwordUtils.getPasswordOrigin(doc.documentURI);
>+            var actionOrigin = passwordUtils.getActionOrigin(aElement.form);

Sigh. The more I think about it, the deeper the autocomplete rabbit hole goes.

Seems, again, that the content process should be responsible for creating the mapping between a DOM element and the interesting information contained in it (here, any existing username/password, page/form origins, field names). Then chrome can be responsible for driving the UI without needing to touch the content DOM.

But then there's bug 556007, which is extending the ability of HTML to contribute results to an input autocomplete (dynamically, even), so that's harder to draw a content/chrome line across.

I need to mull this over a bit, though I suspect the result for now will be to just do what's needed to get pwmgr hooked up, and do any needed redesign of autocomplete as a separate issue.

Comment 18

[Benjamin Stover (:stechz)]

> I don't really understand why stuff is being spun off into this jsm... Except
> for a small about of code that seems reasonable to share between the chrome and
> content process (eg .enabled and .log), I'd expect all of this to live
> exclusively in the content process.

This is all because of this pesky method in nsILoginManager:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/public/nsILoginManager.idl#247

It drags a lot of the form fill stuff along with it, as well as having to listen for blur events afterwards in nsLoginManager.js.  I did this work upfront so that we wouldn't have to change the API.

Mark would rather see this shared code killed by removing fillForm from login manager's API.  I assume password managers might use this code for forms that Firefox didn't detect on load, but I don't know how useful this actually is in practice.  I assume most password managers have their own code for detecting username and password fields anyways (myVidoop's did).  If it's really necessary, extension authors could have some way of accessing this function in frame scripts.

> How is .fillForm even working? It's not remoting anything from chrome to
> content. (Are CPOWs back and working? Thought they got removed?)

See above!  By sharing code with a module, both processes can have the ability to fill forms.

> Ugh. We're probably going to need to rethink this to avoid passing DOM nodes
> between processes.
> 
> Maybe the observer service needs a way to only send notifications to the
> current process? Things wanting to use these notifications would probably want
> to be frame scripts, anyway.

Followup bug?

> This seems like it will be a multiprocess headache too. Oh, I bet this doesn't
> work on Fennec but you don't notice because it's not using the usual
> autocomplete stuff?

Ah, you are right!  Since nsLoginManager can only be found in the parent process, it will never be able to do the autocomplete request.  It happens to work in Firefox this way because it is the same process still.  And it works in Fennec because we request the autocomplete stuff ourselves in the parent process.

I'm afraid this part will need more work in nsFormFillController.  Perhaps it too could be rewritten in JS and use a frame script?  Is this something that could be done in another followup bug since Firefox and Fennec happen to work regardless?

> >+                // Since prompt services ask for window, we try to get the document
> >+                // window before resorting to the chrome window.  When Firefox is
> >+                // e10sified, prompt service will need to take browser elements.
> >+                var win = message.target.contentWindow || message.target.ownerDocument.defaultView;
> 
> I don't quite understand the comment, or why you'd need an || conditional
> there. All the prompt stuff wants the content DOM window, never a chrome
> window.

If contentWindow is null (ie, it is not in this process), then this passes along the chrome window.  It might as well be null, actually.

This is another part that needs to be redone, though it shouldn't take much work.  All of these prompt services need to pass around browser elements now instead of windows.  Fennec currently doesn't attach password prompts to the correct tab (can't find the bug # presently).

> If this is being removed, do the tests pass without leaks on tryserver? (At one
> time this was needed, though I wouldn't be surprised if it's ok with it now.)

I spent a long time figuring out what exactly needed to be nulled.  Besides the circular references for the login manager component with event listeners and such, there was also a circular reference between the login manager component and the form filler component.  I fixed these problems instead of applying your hammer approach.  I've run this through try and this code did indeed not leak.

> Hmm. Seems like in an E10S world this needs to be handled by the content
> process, presumably to be invoked by another frame script. Sure would be handy
> to have the notion of content-process-only components. Can we do anything like
> that? Or perhaps, some sort of hack with this-process-only notifications and
> implement this API as an observer in the content process?

See above.

> >     autoCompleteSearch : function (aSearchString, aPreviousResult, aElement) {
> ...
> >         } else {
> >             this.log("Creating new autocomplete search result.");
> > 
> >             var doc = aElement.ownerDocument;
> >-            var origin = this._getPasswordOrigin(doc.documentURI);
> >-            var actionOrigin = this._getActionOrigin(aElement.form);
> >+            var origin = passwordUtils.getPasswordOrigin(doc.documentURI);
> >+            var actionOrigin = passwordUtils.getActionOrigin(aElement.form);
> 
> Sigh. The more I think about it, the deeper the autocomplete rabbit hole goes.
> 
> Seems, again, that the content process should be responsible for creating the
> mapping between a DOM element and the interesting information contained in it
> (here, any existing username/password, page/form origins, field names). Then
> chrome can be responsible for driving the UI without needing to touch the
> content DOM.
> 
> But then there's bug 556007, which is extending the ability of HTML to
> contribute results to an input autocomplete (dynamically, even), so that's
> harder to draw a content/chrome line across.
> 
> I need to mull this over a bit, though I suspect the result for now will be to
> just do what's needed to get pwmgr hooked up, and do any needed redesign of
> autocomplete as a separate issue.

Yeah, this is what I was talking about above.  Hopefully we can get something working now and address these issues later.  I would be happy to contribute since Fennec is getting you into this mess!  :)

Comment 19

[Benjamin Stover (:stechz)]

Attachment: WIP for Fennec

Mark,

For some reason I am not properly overriding nsLoginManager in Fennec.  Do you know what I'm doing wrong?

Comment 20

[Benjamin Stover (:stechz)]

Attachment: Fennec patch

Comment 21

[Benjamin Stover (:stechz)]

Attachment: Fennec patch

Now removes observers on unload.

Comment 22

[Sudarsana Nagineni]

(In reply to comment #21)
> Created attachment 465404 [details] [diff] [review]
> Fennec patch

I am having problem login to the page with this patch in the following case.

1. Start Fennec
2. Open www.gmail.com
3. Close current tab
4. Open www.gmail.com
5. Enter credentials and press 'Sign in' button

Result: Confirm dialog displays, but nothing happens after tap on any option in the dialog.

Comment 23

[Benjamin Stover (:stechz)]

Attachment: Stop using pref branches

This removes the chrome document I was passing in to prompt service (with a note to fix the prompt service problem in general) and does not use pref branches anymore.

Comment 24

[Benjamin Stover (:stechz)]

Comment on attachment 465404 [details] [diff] [review]
Fennec patch

Sorry for bug spam, not actually obsolete :)

Comment 25

[Justin Dolske [:Dolske]]

Comment on attachment 466394 [details] [diff] [review]
Stop using pref branches

Would be good to port this to mozilla-central right now, so that it's one less blob of diffs to carry around in the mobile port of this code.

Comment 26

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

Comment on attachment 465404 [details] [diff] [review]
Fennec patch

This patch is good enough for Fennec a1, but not quite good enough for Firefox. Let's keep it on mobile-browser for now.

The main thing we should fix before adding to toolkit is the notificationBox logic. Currently, the patch falls back use dialogs

Comment 27

[Benjamin Stover (:stechz)]

Pushed to Fennec http://hg.mozilla.org/mobile-browser/rev/e740685842e1