1258921 - [e10s] Deleting a login via autocomplete doesn't work (tries to use storage from the content process)

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Matthew N. [:MattN]]

While making test_basic_form_autocomplete.html work in e10s (bug 1258860), it became obvious that deletion from autocomplete doesn't work properly.

STR:
1) Go to a form with a saved login
2) Delete (Windows/Linux) or Shift-Delete (OS X) to delete the login

Expected result:
Login is deleted from the autocomplete popup and storage

Actual result:
Login is removed from the autocomplete popup but not storage. The following error occurs in the Browser Console (at least with signon.debug=true):
> Removing login nsLoginManager.js:312
> TypeError: this._storage is null nsLoginManager.js:313:5
> NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this._storage is null" {file: "file:///Users/matthew/repos/fx-team/obj-fx-opt/dist/Nightly.app/Contents/Resources/components/nsLoginManager.js" line: 313}]'[JavaScript Error: "this._storage is null" {file: "file:///Users/matthew/repos/fx-team/obj-fx-opt/dist/Nightly.app/Contents/Resources/components/nsLoginManager.js" line: 313}]' when calling method: [nsILoginManager::removeLogin] LoginManagerContent.jsm:1259
It's trying to use the login manager storage from the content process which is wrong.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

This was pretty straightforward. I'm assuming that bug 1258860 will add a mochitest for this case.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.1

I forgot that LoginManagerContent is used in the parent process as well.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc2eaee83428

Comment 3

[Matthew N. [:MattN]]

Sorry for the delay but I wanted to double check that the deletion won't race with the changing of results. Perhaps it may be okay because we do synchronous storage queries now but I haven't had time to audit it fully yet

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8736046 [details] [diff] [review]
Patch v1.1

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

My main concern is potential for races between result changes and removals. Is the ordering of messages guaranteed in both directions? e.g.

=> RemoteLogins:autoCompleteLogins for "ab"
<= RemoteLogins:loginsAutoCompleted for "ab"
=> RemoteLogins:autoCompleteLogins for "abc"
=> RemoteLogins:removeValueAt 0 – Will this delete index 0 from the results for "ab" or "abc"? 
<= RemoteLogins:loginsAutoCompleted for "abc"

The user will be expecting the deletion from "ab" as that's what they see but in the parent the results for "abc" may be what gets deleted from. Can you help me understand if this can't happen?

https://wiki.mozilla.org/File:PasswordManagerRelationships.svg may be useful (though somewhat out of date since it hasn't been update in ~ 1 year).

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +1177,5 @@
>    },
>  };
>  
>  // nsIAutoCompleteResult implementation
> +function UserAutoCompleteResult (aSearchString, matchingLogins, messageManager) {

Make it clear that the messageManager argument is optional: `messageManager = null`

@@ +1260,5 @@
>      if (this.defaultIndex > this.logins.length)
>        this.defaultIndex--;
>  
>      if (removeFromDB) {
> +      if (this._messageManager) {

I really wish this was handled properly by an e10s-compatible Autocomplete implementation. The current implementation seems like a band-aid over the proper architecture. I think it's okay for now as long as we aren't going to be racing. I may be touching autocomplete soon for another reason and maybe I'll find a way to simplify.

@@ +1261,5 @@
>        this.defaultIndex--;
>  
>      if (removeFromDB) {
> +      if (this._messageManager) {
> +        this._messageManager.sendAsyncMessage("RemoteLogins:removeValueAt",

Is there a reason we aren't just sending "FormAutoComplete:RemoveEntry"? It seems like we end up just forwarding to the same code. This autocomplete e10s situation is so confusing/messy that it really needs some rewriting like we talked about before.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8736046 [details] [diff] [review]
Patch v1.1

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

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +290,5 @@
>      });
>    },
>  
> +  removeAutocompletedValueAt(index) {
> +    AutoCompleteE10S.removeEntry(index);

Also, why not inline this in the `case`?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Matthew N. [:MattN] from comment #4)
> The user will be expecting the deletion from "ab" as that's what they see
> but in the parent the results for "abc" may be what gets deleted from. Can
> you help me understand if this can't happen?

It looks like this can happen. We update the autocomplete result cache in the parent when we send loginsAutoCompleted, so there's a window where the parent and child are out of sync. I'll try to write a testcase for this tomorrow. The only fix I can think of is to add another message from the child to the parent: showAutoCompleteResults that shows the popup and updates the result cache in the parent.

> I really wish this was handled properly by an e10s-compatible Autocomplete
> implementation. The current implementation seems like a band-aid over the
> proper architecture. I think it's okay for now as long as we aren't going to
> be racing. I may be touching autocomplete soon for another reason and maybe
> I'll find a way to simplify.

Me too. The problem with the current setup is that nsAutoCompleteController expects to be able to control both the input element and the autocomplete popup at the same time. I briefly looked into fixing this a while back and didn't make too much headway.

> Is there a reason we aren't just sending "FormAutoComplete:RemoveEntry"? It
> seems like we end up just forwarding to the same code. This autocomplete

We keep the results in both processes: in AutoCompleteE10S._resultCache in the parent and in nsAutoCompleteController::mResults in the child. We have to remove the result's index in both of them in order to make sure they stay in sync. Otherwise, it'd be nice to avoid having to forward to basically the same code (though in a different process).

> e10s situation is so confusing/messy that it really needs some rewriting
> like we talked about before.

No argument from me here :-).

(In reply to Matthew N. [:MattN] from comment #5)
> Also, why not inline this in the `case`?

I'll do this. I was aiming for consistency with the other cases originally.

Comment 7

[Matthew N. [:MattN]]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #6)
> (In reply to Matthew N. [:MattN] from comment #4)
> > The user will be expecting the deletion from "ab" as that's what they see
> > but in the parent the results for "abc" may be what gets deleted from. Can
> > you help me understand if this can't happen?
> 
> It looks like this can happen. We update the autocomplete result cache in
> the parent when we send loginsAutoCompleted, so there's a window where the
> parent and child are out of sync. I'll try to write a testcase for this
> tomorrow. The only fix I can think of is to add another message from the
> child to the parent: showAutoCompleteResults that shows the popup and
> updates the result cache in the parent.

What I'll be changing shortly is for autocomplete to actually know which login an autocomplete entry refers to (instead of just the username text with the origins).

The preferred fix (which I haven't looked into the feasibility of) would be to have the child send the login ID or GUID to delete instead of an index. Then we don't need to care about the result cache at all in the parent or child (assuming the ID/GUID is tied to the entry being deleted).

Comment 8

[Matthew N. [:MattN]]

If bug 1258860 lands first, toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html should be enabled on e10s.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

This patch grew a bit since I wanted to deal with nsILoginInfos outside of IPC on both sides of the IPC barrier. It's also unfortunate that guid is on nsILoginMetaData since that led to a bunch of QIs in various places.

With this patch, we do the DB deletion based on the GUID and then re-find the index in the parent (since it could have changed).

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae46fe6a1a62

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8739606 [details] [diff] [review]
Patch v2

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

Don't forget to enable the test test_basic_form_autocomplete.html for e10s.

r=me but if we can always use "RemoteLogins:removeLogin" I would prefer it.

::: toolkit/components/passwordmgr/LoginHelper.jsm
@@ +408,5 @@
> +   * sending over IPC.
> +   *
> +   * NB: All members of nsILoginInfo and nsILoginMetaInfo are strings.
> +   */
> +  loginsToVanillaObjects(logins) {

Thanks for moving the helpers here

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +1264,5 @@
> +        let vanilla = LoginHelper.loginToVanillaObject(removedLogin);
> +        this._messageManager.sendAsyncMessage("RemoteLogins:removeLogin",
> +                                              { login: vanilla });
> +      } else {
> +        Services.logins.removeLogin(removedLogin);

Why can't we always use the e10s path? Then we wouldn't need the `Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_CONTENT` code either

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Matthew N. [:MattN] (behind on reviews) from comment #10)
> Why can't we always use the e10s path? Then we wouldn't need the
> `Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_CONTENT`
> code either

I looked into doing this and it doesn't really make sense. The difference is that in e10s, when we're in UserAutoCompleteResult.removeValueAt, we have to remove the value both in the child process's array and in the parent's array (as well as from the database). In the parent, we only have to do the work once, in one place (and we'd have to manually grab the autocomplete controller and avoid recurring over and over). Once we figure out how to get rid of AutoCompleteE10S, we could do this.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04db9fe12e08

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/04db9fe12e08

Comment 14

[Matthew N. [:MattN]]

(Quoting Matthew N. [:MattN] (behind on reviews) from comment #10)
> Don't forget to enable the test test_basic_form_autocomplete.html for e10s.

Looks like you forgot after all…

https://hg.mozilla.org/integration/fx-team/rev/07e1879a0c09

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/59fe5c482a55 for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8806419&repo=fx-team

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ea364e7948e2

Comment 17

[Matthew N. [:MattN]]

That was my follow-up test patch that bounced in comment 15

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ea364e7948e2

Comment 19

[Chris Peterson [:cpeterson]]

Marking status-firefox47=wontfix unless Matt thinks we should uplift his e10s passwordmgr test fix to 47.