Skip to content

Commit

Permalink
Bug 1753588 - Restrict the number of form looked up while checking wh…
Browse files Browse the repository at this point in the history
…ether autofilling a username-only form r=tgiles,sgalich

This patch adds a pref "signon.usernameOnlyForm.formThreshold" to limit
the number of form we processed while receiving "PWMGR_NUM_FORM_HAS_POSSIBLE_USERNAME_EVENT_PER_DOC"
event. This improves the performance while loading a page with multiple username-only likely form.

Differential Revision: https://phabricator.services.mozilla.com/D138121
  • Loading branch information
DimiDL committed Feb 11, 2022
1 parent 39fc8c4 commit e661e48
Show file tree
Hide file tree
Showing 6 changed files with 108 additions and 8 deletions.
1 change: 1 addition & 0 deletions modules/libpref/init/all.js
Original file line number Diff line number Diff line change
Expand Up @@ -3647,6 +3647,7 @@ pref("signon.passwordEditCapture.enabled", false);
pref("signon.privateBrowsingCapture.enabled", true);
pref("signon.storeWhenAutocompleteOff", true);
pref("signon.userInputRequiredToCapture.enabled", true);
pref("signon.usernameOnlyForm.lookupThreshold", 5);
pref("signon.debug", false);
pref("signon.recipes.path", "resource://app/defaults/settings/main/password-recipes.json");
pref("signon.recipes.remoteRecipes.enabled", true);
Expand Down
3 changes: 3 additions & 0 deletions toolkit/components/passwordmgr/LoginHelper.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,9 @@ this.LoginHelper = {
this.usernameOnlyFormEnabled = Services.prefs.getBoolPref(
"signon.usernameOnlyForm.enabled"
);
this.usernameOnlyFormLookupThreshold = Services.prefs.getIntPref(
"signon.usernameOnlyForm.lookupThreshold"
);
this.remoteRecipesEnabled = Services.prefs.getBoolPref(
"signon.recipes.remoteRecipes.enabled"
);
Expand Down
14 changes: 9 additions & 5 deletions toolkit/components/passwordmgr/LoginManagerChild.jsm
Original file line number Diff line number Diff line change
Expand Up @@ -1020,6 +1020,15 @@ this.LoginManagerChild = class LoginManagerChild extends JSWindowActorChild {
.getHistogramById("PWMGR_NUM_FORM_HAS_POSSIBLE_USERNAME_EVENT_PER_DOC")
.add(++docState.numFormHasPossibleUsernameEvent);

// Infer whether a form is a username-only form is expensive, so we restrict the
// number of form looked up per document.
if (
docState.numFormHasPossibleUsernameEvent >
LoginHelper.usernameOnlyFormLookupThreshold
) {
return;
}

if (document.visibilityState == "visible" || isMasterPasswordSet) {
this._processDOMFormHasPossibleUsernameEvent(event);
} else {
Expand Down Expand Up @@ -3131,11 +3140,6 @@ this.LoginManagerChild = class LoginManagerChild extends JSWindowActorChild {

let candidate = null;
for (let element of formElement.elements) {
// Only care input fields in the form.
if (ChromeUtils.getClassName(element) !== "HTMLInputElement") {
continue;
}

// We are looking for a username-only form, so if there is a password
// field in the form, this is NOT a username-only form.
if (element.hasBeenTypePassword) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,11 @@ async function checkChildHistogram(id, index, expected) {
}

add_task(async function setup() {
Services.prefs.setBoolPref("signon.usernameOnlyForm.enabled", true);
registerCleanupFunction(() => {
Services.prefs.clearUserPref("signon.usernameOnlyForm.enabled");
SpecialPowers.pushPrefEnv({
set: [
["signon.usernameOnlyForm.enabled", true],
["signon.usernameOnlyForm.lookupThreshold", 100], // ignore the threshold in test
],
});

// Wait 1sec to make sure all the telemetry data recorded prior to the beginning of the
Expand Down
2 changes: 2 additions & 0 deletions toolkit/components/passwordmgr/test/mochitest/mochitest.ini
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ prefs =
# signon.relatedRealms.enabled pref needed until Bug 1699698 lands
signon.relatedRealms.enabled=true
signon.usernameOnlyForm.enabled=true
signon.usernameOnlyForm.lookupThreshold=100

support-files =
../../../prompts/test/chromeScript.js
Expand Down Expand Up @@ -134,6 +135,7 @@ scheme = https
skip-if = toolkit == 'android'
[test_autofill_password-only.html]
[test_autofill_username-only.html]
[test_autofill_username-only_threshold.html]
[test_autofocus_js.html]
scheme = https
skip-if = toolkit == 'android' # autocomplete
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<!DOCTYPE HTML>
<html>
<head>
<meta charset="utf-8">
<title>Test autofill on username-form when the number of form exceeds the lookup threshold</title>
<script src="/tests/SimpleTest/SimpleTest.js"></script>
<script type="text/javascript" src="pwmgr_common.js"></script>
<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
</head>
<body>
Test not autofill on username-form when the number of form exceeds the lookup threshold

<script>
let readyPromise = registerRunTests();
let DEFAULT_ORIGIN = window.location.origin;

addLoginsInParent(
[DEFAULT_ORIGIN, "https://autofill", null, "user1", "pass1"]);

add_task(async function setup() {
await SpecialPowers.pushPrefEnv({"set": [["signon.usernameOnlyForm.lookupThreshold", 5]]});

ok(readyPromise, "check promise is available");
await readyPromise;
});

add_task(async function test_autofill_username_only_form() {
let win = window.open("about:blank");
SimpleTest.registerCleanupFunction(() => win.close());
await loadFormIntoWindow(DEFAULT_ORIGIN, `
<!-- no password field, 1 username field -->
<form id='form1' action='https://autofill'> 1
<input type='text' name='uname' autocomplete='username' value=''>
<button type='submit'>Submit</button>
<button type='reset'> Reset </button>
</form>
<form id='form2' action='https://autofill'> 2
<input type='text' name='uname' autocomplete='username' value=''>
<button type='submit'>Submit</button>
<button type='reset'> Reset </button>
</form>
<form id='form3' action='https://autofill'> 3
<input type='text' name='uname' autocomplete='username' value=''>
<button type='submit'>Submit</button>
<button type='reset'> Reset </button>
</form>
<form id='form4' action='https://autofill'> 4
<input type='text' name='uname' autocomplete='username' value=''>
<button type='submit'>Submit</button>
<button type='reset'> Reset </button>
</form>
<form id='form5' action='https://autofill'> 5
<input type='text' name='uname' autocomplete='username' value=''>
<button type='submit'>Submit</button>
<button type='reset'> Reset </button>
</form>
<form id='form6' action='https://autofill'> 6
<input type='text' name='uname' autocomplete='username' value=''>
<button type='submit'>Submit</button>
<button type='reset'> Reset </button>
</form>`, win);

await checkLoginFormInFrameWithElementValues(win, 1, "user1");
await checkLoginFormInFrameWithElementValues(win, 2, "user1");
await checkLoginFormInFrameWithElementValues(win, 3, "user1");
await checkLoginFormInFrameWithElementValues(win, 4, "user1");
await checkLoginFormInFrameWithElementValues(win, 5, "user1");
await checkUnmodifiedFormInFrame(win, 6);
});
</script>

<p id="display"></p>
<div id="content"></div>
<pre id="test"></pre>
</pre>
</body>
</html>

0 comments on commit e661e48

Please sign in to comment.