1841471 - Calling isInferredEmailField and isInferredUsernameField on input field before running SignUpFormRuleset

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[martin.terra]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:109.0) Gecko/20100101 Firefox/115.0

Steps to reproduce:

Visiting a complex web page with fields related to user management, web page hangs for 19 seconds and according to network tabs no network activity causes this (based on started, ended, response, duration, and latency times). Pausing debugger shows processing ongoing in stack of SignUpFormRuleset.sys.mjs. Issue occurred recently, most recently confirmed in 115.0 (64-bit) Linux version.

Actual results:

Web page hangs for 19 seconds and according to network tabs no network activity causes this (based on started, ended, response, duration, and latency times). Pausing debugger shows processing ongoing in stack of SignUpFormRuleset.sys.mjs. Issue occurred recently, most recently confirmed in 115.0 (64-bit) Linux version. NOTE: Switching off "Ask to save logins and passwords for websites" removes the problem.

Expected results:

Should be no delay. NOTE: Switching off "Ask to save logins and passwords for websites" removes the problem.

Comment 1

[Sergey Galich [:serg]]

@martin.terra thanks for filing this report! Do you have an example of complex HTML form that makes Firefox hang for that long?

Comment 2

[martin.terra]

Attachment: testpage.tar.gz

Surprisingly yes it is reproducible (at least for me) using this static copy of the web page (it is filled with dummy test data so there are no privacy issues).

Comment 3

[martin.terra]

Are you able to reproduce the issue with the attached static html? Thanks.

Comment 4

[Sergey Galich [:serg]]

I didn't get chance to do it myself yet, but someone else will be looking into it soon. Thanks for providing a sample!

Comment 5

[Janika Neuberger [:janika]]

Hi @martin.terra,

thank you so much for filing the report! I was able to reproduce the issue with the provided sample, although on my machine it causes an even larger delay! But as you correctly pointed out, the SignUpFormRuleset.sys.mjs is indeed responsible for the delay.

The SignUpFormRuleset performs a series of checks on the document's <form> HTML elements in order to detect sign-up forms.
And now comes the problematic part... The webpage's <form> elements contain a huge number of HTML elements (20,598 elements), with the majority of them being inspected in multiple rules. Although we filter out many of these elements before performing expensive checks on them, rules/methods like e.g. formHasSubscriptionCheckbox still need to inspect 10,096 input elements and for each one of them, it also inspects the surrounding labels which are another >10,000 elements. In the worst case, these operations can take forever to complete.

Time for some performance improvement!

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:serg, could you have a look please?

For more information, please visit BugBot documentation.

Comment 7

[Janika Neuberger [:janika]]

Attachment: Bug 1841471 - P1. Calling isInferredEmailField and isInferredUsernameField on input field before running SignUpFormRuleset r?#credential-management-reviewers!

Comment 8

[Janika Neuberger [:janika]]

The improvements are carried out in two parts:

1. We add heuristics to restrict for which input elements the SignUpFormRuleset will run to reduce the number of times the ruleset is run for email or username unrelated input fields (knowing that this can cause false-negatives)
2. Rewrite some of the utility functions that are used by the rules in the SignUpFormRuleset to do their task more efficiently.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cda91469ef2a
Calling isInferredEmailField and isInferredUsernameField on input field before running SignUpFormRuleset r=credential-management-reviewers,dimi

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/cda91469ef2a

Comment 11

[Janika Neuberger [:janika]]

(In reply to Janika Neuberger (:janikaneuberger) from comment #8)

The improvements are carried out in two parts:

1. We add heuristics to restrict for which input elements the SignUpFormRuleset will run to reduce the number of times the ruleset is run for email or username unrelated input fields (knowing that this can cause false-negatives)
2. Rewrite some of the utility functions that are used by the rules in the SignUpFormRuleset to do their task more efficiently.

Reopening this bug, because the second task of the fix still needs to be done.

Comment 12

[Janika Neuberger [:janika]]

Closing the bug again, because the second task of the fix is tracked in Bug 1848784.

@martin.terra Sorry for any confusion, I cc'ed you on the new bug.

Comment 13

[Dimi Lee [:dimi]]

Attachment: Bug 1841471 - P1. Calling isInferredEmailField and isInferredUsernameField on input field before running SignUpFormRuleset r?#credential-management-reviewers!

Original Revision: https://phabricator.services.mozilla.com/D185903

Comment 14

[Dimi Lee [:dimi]]

Attachment: Bug 1841471 - P1. Calling isInferredEmailField and isInferredUsernameField on input field before running SignUpFormRuleset r?#credential-management-reviewers!

Original Revision: https://phabricator.services.mozilla.com/D185903

Comment 15

[Phabricator Automation]

Uplift Approval Request

• String changes made/needed: None
• Fix verified in Nightly: yes
• User impact if declined: When the FormHistory module is run for a focused input field of a form that is recognized as a signup form, the user is offered to mask their email address with a relay mask for every input field in a sign up form, not just for the username/email input fields.
• Explanation of risk level: This patch restricts for which input fields the user is offered to mask their email address. No risk is associated with this patch.
• Is Android affected?: no
• Steps to reproduce for manual QE testing: Manuel QE test not needed.
• Code covered by automated testing: no
• Needs manual QE test: no
• Risk associated with taking this patch: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9349368 [details]
Bug 1841471 - P1. Calling isInferredEmailField and isInferredUsernameField on input field before running SignUpFormRuleset r?#credential-management-reviewers!

Approved for 117.0b9.

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/5e95b87376b8