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

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
a year 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)

Comment hidden (empty)
(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

a year ago
Created attachment 8897199 [details] [diff] [review]
Bug 1389596 - make LoginManagerContent.onUsernameInput async
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

a year ago
Created attachment 8900435 [details] [diff] [review]
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.
Attachment #8897199 - Attachment is obsolete: true
Attachment #8900435 - Flags: review?(MattN+bmo)
(Assignee)

Updated

a year 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+
(Assignee)

Comment 5

a year 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)

Comment 6

a year ago
Created attachment 8900531 [details] [diff] [review]
Bug 1389596 - use recipe cache in LoginManagerContent.onUsernameInput
Attachment #8900435 - Attachment is obsolete: true
Attachment #8900531 - Flags: review+
(Assignee)

Updated

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

Comment 7

a year 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
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.