1389595 - Try to avoid LoginManagerContent._onFormSubmit's RemoteLogins:findRecipes sync IPC

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P3)

Comment 1

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1389595 - make LoginManagerContent._onFormSubmit async

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8897198 [details] [diff] [review]
Bug 1389595 - make LoginManagerContent._onFormSubmit async

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

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +52,5 @@
>    notify(formElement, aWindow, actionURI) {
>      log("observer notified for form submission.");
>  
>      // We're invoked before the content's |onsubmit| handlers, so we
>      // can grab form data before it might be modified (see bug 257781).

This is an important comment and requires the following code https://dxr.mozilla.org/mozilla-central/rev/7ff4c2f1fe11f6b98686f783294692893b1e1e8b/toolkit/components/passwordmgr/LoginManagerContent.jsm#956-966 in _onFormSubmit to be executed synchronously before the submit event but unfortunately bug 257781 didn't seem to add a test for this… It would be great if you could add a simple one but it's not absolutely required. Basically the mochitest-plain test would listen to the 'submit' event in the capturing phase and change input.value and ensure that password manager still gets the unchanged value.

Basically I think we need a different approach to not regress bug 257781:
1) Create a _getRecipes helper in this file (LMC) which synchronously requests recipes with sendSyncMessage but also caches them for synchronous use later.  We can do a cache lookup instead of sending a message if we have a hit. The recipe cache can be a private WeakMap on LMC (_recipeCache) with the 1st key being the nsIContentFrameMessageManager instance (so the lifetime is tied to the frame; you can get it like [1]) and the value can be a Map with a key of the formOrigin. The value would be the returned recipes Set which may be an empty Set in the case of no matches.

2) Since _fetchLoginsFromParentAndFillForm requests recipes when login forms are detected it should populate the recipe cache in common cases. Since we don't autofill insecure forms or forms in private browsing contexts then we should still retain the sync message fallback for when there is a cache miss. In a follow-up we can pre-fill the recipe cache for these scenarios but I don't think we should do that all at once to ease reviewing. In _fetchLoginsFromParentAndFillForm we should populate _recipeCache with the returned recipes since it uses the same API as findRecipes does.

3) In `_onFormSubmit`, use the synchronous `_getRecipes` helper from (1) and it should have a cache hit in most cases so we don't need to make _onFormSubmit async and regress bug 257781.

This recipe helper with a cache would be useful for the other bugs using this findRecipes sync message.

Apologies if my explanation is a bit confusing as I had to go back and rewrite parts as I was writing it due to a change in my understanding and may have left some confusing bits. Essentially we want to cache recipes which we normally get asynchronously via _fetchLoginsFromParentAndFillForm so we can use them later for times when we need them synchronously (like _onFormSubmit).

Let me know if you have any questions. Thanks!

[1] http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/browser/components/feeds/FeedWriter.js#981
 When we request recipes in _fillForm

@@ +65,5 @@
>        log("Caught error in onFormSubmit(", e.lineNumber, "):", e.message);
>        Cu.reportError(e);
>      }
>  
> +

Nit: revert this extra line

@@ +184,5 @@
>                 "RemoteLogins:loginsAutoCompleted" ],
>  
> +  _getRecipes(formOrigin, mm) {
> +    return new Promise(resolve => {
> +      let listener = message => {

I think this will work but is there a reason you're not using the existing _sendRequest method and adding this message to receive to `_messages` instead?

@@ +943,5 @@
>      }
>  
>      let formSubmitURL = LoginUtils._getActionOrigin(form);
>      let messageManager = messageManagerFromWindow(win);
> +    let recipes = await this._getRecipes(hostname, messageManager);

To clarify what I said above: this is the problem with the patch since it makes _getFormFields and extracting .value happen asynchronously.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +102,2 @@
>          let formHost = (new URL(data.formOrigin)).host;
> +        // Sets wont send across the process boundry with sendAsyncMessage

typo: boundary

@@ +102,4 @@
>          let formHost = (new URL(data.formOrigin)).host;
> +        // Sets wont send across the process boundry with sendAsyncMessage
> +        let recipes = [...this._recipeManager.getRecipesForHost(formHost)];
> +        mm.sendAsyncMessage("RemoteLogins:findRecipes:result", { recipes });

Are you sure about this? This should work since bug 1139718 I think. Sets support structured clone so they should send. IIRC the problem may be with the Set being nested in the object rather than just being sent as the object.

@@ +103,5 @@
> +        // Sets wont send across the process boundry with sendAsyncMessage
> +        let recipes = [...this._recipeManager.getRecipesForHost(formHost)];
> +        mm.sendAsyncMessage("RemoteLogins:findRecipes:result", { recipes });
> +        // don't break sync IPC (to be removed after 1389596 and 1390347)
> +        return new Set(recipes);

Nit: include the word "bug " before the bug numbers so that it's easier to search for bug numbers in code comments.

Comment 3

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1389595 - cache recipes in LoginManagerContent and use cache in _onFormSubmit

windows are used as the _recipeCache WeakMap keys since nsIContentFrameMessageManagers couldn't be used.

Comment 4

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1389595 - add test to check correct form inputs are captured

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8900048 [details] [diff] [review]
Bug 1389595 - cache recipes in LoginManagerContent and use cache in _onFormSubmit

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

The disadvantage of using a window is that it means the cache isn't shared between loads in the same frame which will reduce the effectiveness of the cache in certain edge cases but in the common case of the logins found message being received because autofill is on then this will work.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8900049 [details] [diff] [review]
Bug 1389595 - add test to check correct form inputs are captured

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

Thank you very much for the test!

::: toolkit/components/passwordmgr/test/mochitest/mochitest.ini
@@ +63,5 @@
>  skip-if = os == "linux" || toolkit == 'android' # Tests desktop prompts
>  [test_prompt_promptAuth_proxy.html]
>  skip-if = e10s || os == "linux" || toolkit == 'android' # Tests desktop prompts
>  [test_recipe_login_fields.html]
> +[test_onsubmit_value_change.html]

Nit: please keep this list in alphabetical order and put it in the correct place

Comment 7

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1389595 - add test to check correct form inputs are capturedtest.diff

Comment 8

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1389595 - cache recipes in LoginRecipesContent and use cache in LoginManagerContent._onFormSubmit

Comment 9

[Perry Jiang [:perry] [no longer employee, use ni?]]

Comment on attachment 8900513 [details] [diff] [review]
Bug 1389595 - cache recipes in LoginRecipesContent and use cache in LoginManagerContent._onFormSubmit

Forgot to send the message from LoginManagerParent.reset ...

Comment 10

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1389595 - cache recipes in LoginRecipesContent and use cache in LoginManagerContent._onFormSubmit

Comment 11

[Matthew N. [:MattN]]

Comment on attachment 8900529 [details] [diff] [review]
Bug 1389595 - cache recipes in LoginRecipesContent and use cache in LoginManagerContent._onFormSubmit

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

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +35,5 @@
>    let logger = LoginHelper.createLogger("LoginManagerContent");
>    return logger.log.bind(logger);
>  });
>  
> +LoginRecipesContent.init();

To keep LoginRecipes.jsm loaded lazily I think you could put this call at the bottom of LoginRecipes.jsm inside an processType check:
```js
if (Services.appinfo.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT) { 
  LoginRecipesContent.init();
}
```
though I'm not 100% sure if that works for non-e10s.

::: toolkit/components/passwordmgr/LoginRecipes.jsm
@@ +117,5 @@
> +        }).then(recipes => {
> +          return new Promise(resolve => {
> +            // -1 to account for the parent process
> +            let numCaches = Services.ppmm.childCount - 1;
> +            let cachesCleared = 0;

It seems like this may introduce race conditions making the promise never resolve so I wouldn't wait for acknowledgement.

Comment 12

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1389595 - cache recipes in LoginRecipesContent and use cache in LoginManagerContent._onFormSubmit

Test order swapped in test_basic_form_autocomplete.html to avoid using invalid cache values set after loadRecipes.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #11)
> Comment on attachment 8900529 [details] [diff] [review]
> Bug 1389595 - cache recipes in LoginRecipesContent and use cache in
> LoginManagerContent._onFormSubmit
> 
> Review of attachment 8900529 [details] [diff] [review]:
> -----------------------------,------------------------------------
> 
> ::: toolkit/components/passwordmgr/LoginManagerContent.jsm
> @@ +35,5 @@
> >    let logger = LoginHelper.createLogger("LoginManagerContent");
> >    return logger.log.bind(logger);
> >  });
> >  
> > +LoginRecipesContent.init();
> 
> To keep LoginRecipes.jsm loaded lazily I think you could put this call at
> the bottom of LoginRecipes.jsm inside an processType check:
> ```js
> if (Services.appinfo.processType == Ci.nsIXULRuntime.PROCESS_TYPE_CONTENT) { 
>   LoginRecipesContent.init();
> }
> ```

Unfortunately this would break on non-e10s. Even though it's being called from a frame script, the processType still will say parent

Comment 14

[Perry Jiang [:perry] [no longer employee, use ni?]]

I think the lazy loading can be preserved if the message listener is set in LoginManagerContent.jsm. Let me know if that works.

Comment 15

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1389595 - cache recipes in LoginRecipesContent and use cache in LoginManagerContent._onFormSubmit

Doing what comment 14 says.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8901384 [details] [diff] [review]
Bug 1389595 - cache recipes in LoginRecipesContent and use cache in LoginManagerContent._onFormSubmit

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

Thanks

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06338b8f9ae1
cache recipes in LoginRecipesContent and use cache in LoginManagerContent._onFormSubmit. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e82093e3fa3
add test to check correct form inputs are captured. r=MattN

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca75fef71e15
Don't access elements before they're parsed in test_onsubmit_value_change.html

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/06338b8f9ae1
https://hg.mozilla.org/mozilla-central/rev/0e82093e3fa3
https://hg.mozilla.org/mozilla-central/rev/ca75fef71e15