1389596 - Try to avoid LoginManagerContent.onUsernameInput's RemoteLogins:findRecipes sync IPC

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement)

Comment 1

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

Attachment: Bug 1389596 - make LoginManagerContent.onUsernameInput async

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8897199 [details] [diff] [review]
Bug 1389596 - make LoginManagerContent.onUsernameInput async

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

Thanks

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +649,5 @@
>  
>      let doc = acForm.ownerDocument;
>      let messageManager = messageManagerFromWindow(doc.defaultView);
> +    let formOrigin = LoginUtils._getPasswordOrigin(doc.documentURI);
> +    let recipes = await this._getRecipes(formOrigin, messageManager);

After the bug 1389595 comment 2 approach I think you can leave this as a sync function and remove the await and it will be good.

Comment 3

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

Attachment: Bug 1389596 - use recipe cache in LoginManagerContent.onUsernameInput

I cleared the recipe cache in resetRecipes because test_form12_formless was getting an incorrect recipe that was cached but not removed in test_form11_recipes.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8900435 [details] [diff] [review]
Bug 1389596 - use recipe cache in LoginManagerContent.onUsernameInput

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

Looking at some code again makes me wonder if we should have put the recipe cache on LoginRecipesContent instead of LoginManagerContent…

::: toolkit/components/passwordmgr/test/pwmgr_common.js
@@ +302,5 @@
>  
>  function resetRecipes() {
>    info("Resetting recipes");
> +  const LoginManagerContent = SpecialPowers.Cu.import("resource://gre/modules/LoginManagerContent.jsm")
> +    .LoginManagerContent;

Nit: The following is more concise:
const {LoginManagerContent} = SpecialPowers.Cu.import("resource://gre/modules/LoginManagerContent.jsm");

@@ +303,5 @@
>  function resetRecipes() {
>    info("Resetting recipes");
> +  const LoginManagerContent = SpecialPowers.Cu.import("resource://gre/modules/LoginManagerContent.jsm")
> +    .LoginManagerContent;
> +  LoginManagerContent.clearRecipeCache();

Thinking about this more… it really seems like the recipe cache should be cleared whenever LoginRecipesParent.reset() is called, not just in tests…

It seems like we should move the recipe cache to LoginRecipesContent and have it listen for a broadcast message from LoginRecipesParent.reset() to clear the cache. I know there aren't currently any callers of .reset at runtime but if we start to use it then it would be good for it to work. If you or Felipe disagree about doing this work here then I think we should least update the jsdoc comment on .reset to indicate that it doesn't currently clear the content process recipe cache.

Comment 5

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

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #4)
> It seems like we should move the recipe cache to LoginRecipesContent and
> have it listen for a broadcast message from LoginRecipesParent.reset() to
> clear the cache. I know there aren't currently any callers of .reset at
> runtime but if we start to use it then it would be good for it to work. If
> you or Felipe disagree about doing this work here then I think we should
> least update the jsdoc comment on .reset to indicate that it doesn't
> currently clear the content process recipe cache.

I think that makes more sense. I'll make that change on bug 1389595.

Comment 6

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

Attachment: Bug 1389596 - use recipe cache in LoginManagerContent.onUsernameInput

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1163bef92f20
use recipe cache in LoginManagerContent.onUsernameInput. r=MattN

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/1163bef92f20