Closed Bug 1389596 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Password Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: perry, Assigned: perry)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → jiangperry
Blocks: 1371525
Depends on: 1389595
Summary: Try to avoid LoginManagerContent._onUsernameInput's RemoteLogins:findRecipes sync IPC → Try to avoid LoginManagerContent.onUsernameInput's RemoteLogins:findRecipes sync IPC
Attachment #8897199 - Flags: review?(MattN+bmo)
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.
Attachment #8897199 - Flags: review?(MattN+bmo) → feedback+
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.
Attachment #8897199 - Attachment is obsolete: true
Attachment #8900435 - Flags: review?(MattN+bmo)
Blocks: 1390347
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.
Attachment #8900435 - Flags: review?(MattN+bmo) → feedback+
Status: NEW → ASSIGNED
(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.
Attachment #8900531 - Attachment description: usernameinput.diff → Bug 1389596 - use recipe cache in LoginManagerContent.onUsernameInput
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1163bef92f20
use recipe cache in LoginManagerContent.onUsernameInput. r=MattN
https://hg.mozilla.org/mozilla-central/rev/1163bef92f20
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.