Closed Bug 1371525 Opened 7 years ago Closed 7 years ago

Try to avoid the RemoteLogins:findRecipes sync IPC

Categories

(Toolkit :: Password Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Performance Impact high
Tracking Status
firefox57 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: perry)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

There are three call sites in LoginManagerContent.jsm, one looks pretty async, one is sending an async message shortly after so it may be possible to convert to an asynchronous message, the getFieldContext() one is trickier.

The mean time for this probe is 2.81ms which is fairly high.  It's sent by 86% of sessions.
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
Assignee: nobody → jiangperry
Status: NEW → ASSIGNED
One of the reasons this is a sync message is because the return value provided by the listener is then sent back and set as the return value of the sendSyncMessage function. e.g.:

> // --- in the parent process ---
> 
> addMessageListener("foo", function() {
>   return 3;
> });

> // --- in the child process ---
> 
> let bar = messageManager.sendSyncMessage("foo");
> // bar is 3

With an async message this is not possible.. So you need to do this manually with some msg handshake.

Looking it up on searchfox, it looks like there are three locations where we use this sync message: http://searchfox.org/mozilla-central/search?q=RemoteLogins%3AfindRecipes&path=

Matt suggested that we can convert them one by one (in separate bugs), while leaving the others untouched (still using a sync message). That way we can start by the easier or most important first, and not care too much about the others.

Here's what Matt said:

The one on _onFormSubmit happens for every form submission (unless the password manager is disabled)
The one on getFieldContext happens on every page load _if there is a saved login for that domain_
(The other one we didn't talk about)

I think we can start by the _onFormSubmit one which looks simpler, and then work on the other two.
Perry did all the hard work in the three dependencies, thanks to him for that! I'll leave this assigned to him as a result as it's kinda a meta bug.

After those three bugs, we will have removed all uses of the sync RemoteLogins:findRecipes message except for edge cases where we need a synchronous answer (e.g. _onFormSubmit) but the cache hasn't been populated asynchronously. We can watch the sync message JS telemetry to ensure that the occurrence goes down to single-digit % of sessions.
Comment on attachment 8903817 [details]
Bug 1371525 - Warn when sending a synchronous RemoteLogins:findRecipes message.

https://reviewboard.mozilla.org/r/175574/#review180678
Attachment #8903817 - Flags: review?(felipc) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 7708c1e3f5bf -d 512562477a1e: rebasing 417500:7708c1e3f5bf "Bug 1371525 - Warn when sending a synchronous RemoteLogins:findRecipes message. r=Felipe" (tip)
merging toolkit/components/passwordmgr/LoginRecipes.jsm
warning: conflicts while merging toolkit/components/passwordmgr/LoginRecipes.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/2d4783c5bf18
Warn when sending a synchronous RemoteLogins:findRecipes message. r=Felipe
https://hg.mozilla.org/mozilla-central/rev/2d4783c5bf18
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.