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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: perry, Assigned: perry)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → jiangperry
(Assignee)

Updated

2 years ago
Blocks: 1371525
(Assignee)

Updated

2 years ago
Blocks: 1389596
(Assignee)

Updated

2 years ago
Blocks: 1390347
(Assignee)

Comment 1

2 years ago
Attachment #8897198 - Flags: review?(MattN+bmo)
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.
Attachment #8897198 - Flags: review?(MattN+bmo) → review-
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Comment 3

2 years ago
windows are used as the _recipeCache WeakMap keys since nsIContentFrameMessageManagers couldn't be used.
Attachment #8897198 - Attachment is obsolete: true
Attachment #8900048 - Flags: review?(MattN+bmo)
(Assignee)

Comment 4

2 years ago
Attachment #8900049 - Flags: review?(MattN+bmo)
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.
Attachment #8900048 - Flags: review?(MattN+bmo) → review+
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
Attachment #8900049 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 9

2 years ago
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 ...
Attachment #8900513 - Flags: review?(MattN+bmo)
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.
Attachment #8900529 - Flags: review?(MattN+bmo) → feedback+
(Assignee)

Comment 12

2 years ago
Test order swapped in test_basic_form_autocomplete.html to avoid using invalid cache values set after loadRecipes.
Attachment #8900529 - Attachment is obsolete: true
Attachment #8900912 - Flags: review?(MattN+bmo)
(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
(Assignee)

Comment 14

2 years ago
I think the lazy loading can be preserved if the message listener is set in LoginManagerContent.jsm. Let me know if that works.
(Assignee)

Comment 15

2 years ago
Doing what comment 14 says.
Attachment #8900912 - Attachment is obsolete: true
Attachment #8900912 - Flags: review?(MattN+bmo)
Attachment #8901384 - Flags: review?(MattN+bmo)
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
Attachment #8901384 - Flags: review?(MattN+bmo) → review+

Comment 17

2 years ago
Pushed by mozilla@noorenberghe.ca:
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

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca75fef71e15
Don't access elements before they're parsed in test_onsubmit_value_change.html
You need to log in before you can comment on or make changes to this bug.