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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
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

(1 attachment, 2 obsolete attachments)

Assignee

Description

2 years ago
No description provided.
Assignee

Updated

2 years ago
Assignee: nobody → jiangperry
Assignee

Updated

2 years ago
Blocks: 1371525
Assignee

Updated

2 years ago
Depends on: 1389595
Summary: Try to avoid LoginManagerContent._onUsernameInput's RemoteLogins:findRecipes sync IPC → Try to avoid LoginManagerContent.onUsernameInput's RemoteLogins:findRecipes sync IPC
Assignee

Comment 1

2 years ago
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+
Assignee

Comment 3

2 years ago
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)
Assignee

Updated

2 years ago
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
Assignee

Comment 5

2 years ago
(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.
Assignee

Updated

2 years ago
Attachment #8900531 - Attachment description: usernameinput.diff → Bug 1389596 - use recipe cache in LoginManagerContent.onUsernameInput

Comment 7

2 years ago
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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.