commit ca1d56427df8e17b48b6c85cd74aaa410ba1bc86
parent f37ec715c24cbb7e207768ae7ef1b0e45ec07d6c
Author: Johannes Salas Schmidt <joschmidt@mozilla.com>
Date: Mon, 24 Nov 2025 10:26:47 +0000
Bug 2001366 - restructure telemetry r=theidkamp,credential-management-reviewers,dimi
Differential Revision: https://phabricator.services.mozilla.com/D273430
Diffstat:
3 files changed, 229 insertions(+), 187 deletions(-)
diff --git a/toolkit/components/passwordmgr/LoginManagerRustMirror.sys.mjs b/toolkit/components/passwordmgr/LoginManagerRustMirror.sys.mjs
@@ -7,8 +7,10 @@ ChromeUtils.defineESModuleGetters(lazy, {
LoginHelper: "resource://gre/modules/LoginHelper.sys.mjs",
});
+const rustMirrorTelemetryVersion = "2";
+
/* Check if an url has punicode encoded hostname */
-function isPunycode(origin) {
+function isPunycodeOrigin(origin) {
try {
return origin && new URL(origin).hostname.startsWith("xn--");
} catch (_) {
@@ -16,64 +18,91 @@ function isPunycode(origin) {
}
}
-function recordIncompatibleFormats(runId, operation, loginInfo) {
- if (isPunycode(loginInfo.origin)) {
- Glean.pwmgr.rustIncompatibleLoginFormat.record({
- run_id: runId,
- issue: "nonAsciiOrigin",
- operation,
- });
- }
- if (isPunycode(loginInfo.formActionOrigin)) {
- Glean.pwmgr.rustIncompatibleLoginFormat.record({
- run_id: runId,
- issue: "nonAsciiFormAction",
- operation,
- });
- }
+/* Check if an origin is a single dot */
+function isSingleDot(str) {
+ return str === ".";
+}
- if (loginInfo.origin === ".") {
- Glean.pwmgr.rustIncompatibleLoginFormat.record({
- run_id: runId,
- issue: "dotOrigin",
- operation,
- });
- }
+/* Check if an origin does not contain http protocol */
+function isHttpOrigin(str) {
+ return str.startsWith("https://") || str.startsWith("http://");
+}
- if (
- loginInfo.username?.includes("\n") ||
- loginInfo.username?.includes("\r")
- ) {
- Glean.pwmgr.rustIncompatibleLoginFormat.record({
- run_id: runId,
- issue: "usernameLineBreak",
- operation,
- });
- }
+/* Check if a string contains line breaks */
+function containsLineBreaks(str) {
+ return str.includes("\n") || str.includes("\r");
+}
+
+/* Check if a string contains Nul string */
+function containsNul(str) {
+ return str.includes("\0");
}
-function recordMirrorStatus(runId, operation, status, error = null) {
+/* Normalize different errors */
+function normalizeRustStorageErrorMessage(error) {
+ const message = error?.message || String(error);
+
+ return message
+ .replace(/^reason: /, "")
+ .replace(/^Invalid login: /, "")
+ .replace(/\{[0-9a-fA-F-]{36}\}/, "{UUID}");
+}
+
+function recordMirrorFailure(runId, operation, error, login = null) {
+ // lookup poisoned status
const poisoned = Services.prefs.getBoolPref(
"signon.rustMirror.poisoned",
false
);
- let errorMessage = "";
- if (error) {
- errorMessage = error.message ?? String(error);
+ const data = {
+ metric_version: rustMirrorTelemetryVersion,
+ run_id: runId,
+ operation,
+ poisoned,
+
+ error_message: normalizeRustStorageErrorMessage(error),
+
+ is_deleted: false,
+ has_empty_password: false,
+ has_punycode_origin: false,
+ has_single_dot_origin: false,
+ has_non_http_origin: false,
+ has_username_line_break: false,
+ has_username_nul: false,
+ };
+
+ if (login) {
+ data.is_deleted = login.deleted;
+ data.has_empty_password = !login.password;
+ data.has_punycode_origin = isPunycodeOrigin(
+ login.origin || login.formActionOrigin
+ );
+ data.has_single_dot_origin = isSingleDot(
+ login.origin || login.formActionOrigin
+ );
+ data.has_non_http_origin = !isHttpOrigin(
+ login.origin || login.formActionOrigin
+ );
+ data.has_username_line_break = containsLineBreaks(login.username);
+ data.has_username_nul = containsNul(login.username);
}
+ Glean.pwmgr.rustWriteFailure.record(data);
+
+ // set poisoned status on error
+ if (!poisoned) {
+ Services.prefs.setBoolPref("signon.rustMirror.poisoned", true);
+ }
+}
+
+function recordMirrorStatus(runId, operation, status) {
Glean.pwmgr.rustMirrorStatus.record({
+ metric_version: rustMirrorTelemetryVersion,
run_id: runId,
operation,
status,
- error_message: errorMessage,
- poisoned,
});
-
- if (status === "failure" && !poisoned) {
- Services.prefs.setBoolPref("signon.rustMirror.poisoned", true);
- }
}
function recordMigrationStatus(
@@ -85,23 +114,13 @@ function recordMigrationStatus(
const had_errors = numberOfLoginsMigrated < numberOfLoginsToMigrate;
Glean.pwmgr.rustMigrationStatus.record({
+ metric_version: rustMirrorTelemetryVersion,
run_id: runId,
duration_ms: duration,
number_of_logins_to_migrate: numberOfLoginsToMigrate,
number_of_logins_migrated: numberOfLoginsMigrated,
had_errors,
});
-
- if (had_errors) {
- Services.prefs.setBoolPref("signon.rustMirror.poisoned", true);
- }
-}
-
-function recordMigrationFailure(runId, error) {
- Glean.pwmgr.rustMigrationFailure.record({
- run_id: runId,
- error_message: error.message ?? String(error),
- });
}
export class LoginManagerRustMirror {
@@ -166,7 +185,6 @@ export class LoginManagerRustMirror {
this.#logger.log("Rust Mirror is enabled.");
} catch (e) {
this.#logger.error("Login migration failed", e);
- recordMirrorStatus("migration-enable", "failure", e);
}
}
@@ -201,19 +219,20 @@ export class LoginManagerRustMirror {
const runId = Services.uuid.generateUUID();
let loginToModify;
let newLoginData;
+ let status = "success";
switch (eventName) {
case "addLogin":
this.#logger.log(`adding login ${subject.guid}...`);
try {
- recordIncompatibleFormats(runId, "add", subject);
await this.#rustStorage.addLoginsAsync([subject]);
- recordMirrorStatus(runId, "add", "success");
this.#logger.log(`added login ${subject.guid}.`);
} catch (e) {
+ status = "failure";
+ recordMirrorFailure(runId, "add", e, subject);
this.#logger.error("mirror-error:", e);
- recordMirrorStatus(runId, "add", "failure", e);
}
+ recordMirrorStatus(runId, "add", status);
break;
case "modifyLogin":
@@ -221,38 +240,40 @@ export class LoginManagerRustMirror {
newLoginData = subject.queryElementAt(1, Ci.nsILoginInfo);
this.#logger.log(`modifying login ${loginToModify.guid}...`);
try {
- recordIncompatibleFormats(runId, "modify", newLoginData);
this.#rustStorage.modifyLogin(loginToModify, newLoginData);
- recordMirrorStatus(runId, "modify", "success");
this.#logger.log(`modified login ${loginToModify.guid}.`);
} catch (e) {
+ status = "failure";
+ recordMirrorFailure(runId, "modify", e, newLoginData);
this.#logger.error("error: modifyLogin:", e);
- recordMirrorStatus(runId, "modify", "failure", e);
}
+ recordMirrorStatus(runId, "modify", status);
break;
case "removeLogin":
this.#logger.log(`removing login ${subject.guid}...`);
try {
this.#rustStorage.removeLogin(subject);
- recordMirrorStatus(runId, "remove", "success");
this.#logger.log(`removed login ${subject.guid}.`);
} catch (e) {
+ status = "failure";
+ recordMirrorFailure(runId, "remove", e, subject);
this.#logger.error("error: removeLogin:", e);
- recordMirrorStatus(runId, "remove", "failure", e);
}
+ recordMirrorStatus(runId, "remove", status);
break;
case "removeAllLogins":
this.#logger.log("removing all logins...");
try {
this.#rustStorage.removeAllLogins();
- recordMirrorStatus(runId, "remove-all", "success");
this.#logger.log("removed all logins.");
} catch (e) {
+ status = "failure";
this.#logger.error("error: removeAllLogins:", e);
- recordMirrorStatus(runId, "remove-all", "failure", e);
+ recordMirrorFailure(runId, "remove-all", e);
}
+ recordMirrorStatus(runId, "remove-all", status);
break;
// re-migrate on importLogins event
@@ -318,14 +339,15 @@ export class LoginManagerRustMirror {
Services.prefs.setBoolPref("signon.rustMirror.poisoned", false);
- const logins = await this.#jsonStorage.getAllLogins();
+ // get all logins; exclude deletion stubs
+ const logins = await this.#jsonStorage.getAllLogins(false);
numberOfLoginsToMigrate = logins.length;
const results = await this.#rustStorage.addLoginsAsync(logins, true);
- for (const { error } of results) {
+ for (const [i, { error }] of results.entries()) {
if (error) {
this.#logger.error("error during migration:", error.message);
- recordMigrationFailure(runId, error);
+ recordMirrorFailure(runId, "migration", error, logins[i]);
} else {
numberOfLoginsMigrated += 1;
}
diff --git a/toolkit/components/passwordmgr/metrics.yaml b/toolkit/components/passwordmgr/metrics.yaml
@@ -850,6 +850,9 @@ pwmgr:
- passwords-dev@mozilla.org
expires: never
extra_keys:
+ metric_version:
+ type: string
+ description: Identifier for rust mirror telemetry version
run_id:
type: string
description: Unique identifier for the login change event
@@ -861,98 +864,104 @@ pwmgr:
description: >
The result of the operation ("success" or "failure").
type: string
- error_message:
- description: >
- The error message or exception string from the failure.
- type: string
- poisoned:
- description: >
- Whether there was a previous error, meaning that this event must be viewed with caution.
- type: boolean
- rust_migration_failure:
+ rust_migration_status:
type: event
description: >
- Records errors during a migration run.
+ One record per rolling migration run, recording duration, total number
+ of logins migrated and whether there have been errors.
bugs:
- - https://bugzil.la/1991676
+ - https://bugzil.la/1985800
data_reviews:
- - https://bugzil.la/1991676
+ - https://bugzil.la/1985800
data_sensitivity:
- technical
notification_emails:
- passwords-dev@mozilla.org
expires: never
extra_keys:
+ metric_version:
+ type: string
+ description: Identifier for rust mirror telemetry version
run_id:
type: string
description: Unique identifier for the migration run.
- error_message:
- type: string
- description: The error message from the failed login entry.
+ number_of_logins_to_migrate:
+ description: >
+ Total number of logins to be migrated during this run.
+ type: quantity
+ number_of_logins_migrated:
+ description: >
+ Number of logins migrated during this run.
+ type: quantity
+ duration_ms:
+ description: >
+ Duration of the migration in milliseconds.
+ type: quantity
+ had_errors:
+ description: >
+ Whether the migration had errors.
+ type: boolean
- rust_incompatible_login_format:
+ rust_write_failure:
type: event
description: >
- Records whenever a login format is incompatible with the Rust password
- store. Includes which field was affected and during which operation.
+ Records errors during mirror or migration run.
bugs:
- - https://bugzil.la/1981814
+ - https://bugzil.la/1991676
data_reviews:
- - https://bugzil.la/1981814
+ - https://bugzil.la/1991676
data_sensitivity:
- technical
notification_emails:
- passwords-dev@mozilla.org
expires: never
extra_keys:
+ metric_version:
+ type: string
+ description: Identifier for rust mirror telemetry version
run_id:
type: string
description: Unique identifier for the login change event
- issue:
- description: >
- Which kind of incompatible format (dotOrigin, nonAsciiOrigin,
- nonAsciiFormAction, usernameLineBreak).
- type: string
operation:
description: >
- The operation being performed when the incompatible format was found
- (e.g. addLogin, modifyLogin, removeLogin).
+ The type of operation that failed (e.g. "add", "modify", "remove", "remove-all", "migration", etc.).
type: string
-
- rust_migration_status:
- type: event
- description: >
- One record per rolling migration run, recording duration, total number
- of logins migrated and whether there have been errors.
- bugs:
- - https://bugzil.la/1985800
- data_reviews:
- - https://bugzil.la/1985800
- data_sensitivity:
- - technical
- notification_emails:
- - passwords-dev@mozilla.org
- expires: never
- extra_keys:
- run_id:
+ poisoned:
+ description: >
+ Whether there was a previous error, meaning that this event must be viewed with caution.
+ type: boolean
+ error_message:
+ description: >
+ The error message or exception string from the failure.
type: string
- description: Unique identifier for the migration run.
- number_of_logins_to_migrate:
+ has_empty_password:
description: >
- Total number of logins to be migrated during this run.
- type: quantity
- number_of_logins_migrated:
+ Whether the login to add/modify/remove has an empty password
+ type: boolean
+ has_punycode_origin:
description: >
- Number of logins migrated during this run.
- type: quantity
- duration_ms:
+ Whether the login to add/modify/remove has a punycode origin or form_action_origin
+ type: boolean
+ has_single_dot_origin:
description: >
- Duration of the migration in milliseconds.
- type: quantity
- had_errors:
+ Whether the login to add/modify/remove has an origin or form_action_origin consisting of a single dot only
+ type: boolean
+ has_non_http_origin:
description: >
- Whether the migration had errors.
+ Whether the login to add/modify/remove has an origin or form_action_origin which does not start with http:// or https://
+ type: boolean
+ has_username_line_break:
+ description: >
+ Whether the login to add/modify/remove has a username containing a line break
+ type: boolean
+ has_username_nul:
+ description: >
+ Whether the login to add/modify/remove has a username containing a Nul char
+ type: boolean
+ is_deleted:
+ description: >
+ Whether the login to add/modify/remove is a deletion stub
type: boolean
saving_enabled:
diff --git a/toolkit/components/passwordmgr/test/browser/browser_rust_mirror.js b/toolkit/components/passwordmgr/test/browser/browser_rust_mirror.js
@@ -434,24 +434,25 @@ add_task(async function test_rust_migration_failure_event() {
});
await Services.logins.addLoginAsync(login_bad);
+ const waitForGleanEvent = BrowserTestUtils.waitForCondition(
+ () => Glean.pwmgr.rustWriteFailure.testGetValue()?.length == 1,
+ "event has been emitted"
+ );
+
// Trigger migration
await SpecialPowers.pushPrefEnv({
set: [["signon.rustMirror.enabled", true]],
});
- await BrowserTestUtils.waitForCondition(
- () => Glean.pwmgr.rustMigrationFailure.testGetValue()?.length == 1,
- "event has been emitted"
- );
+ await waitForGleanEvent;
- const [evt] = Glean.pwmgr.rustMigrationFailure.testGetValue();
+ const [evt] = Glean.pwmgr.rustWriteFailure.testGetValue();
Assert.ok(evt.extra?.run_id, "event has a run_id");
Assert.equal(
evt.extra?.error_message,
"simulated migration failure",
"event has the expected error message"
);
- Assert.equal(evt.name, "rust_migration_failure", "event has correct name");
sinon.restore();
LoginTestUtils.clearData();
@@ -515,6 +516,11 @@ add_task(async function test_rust_mirror_addLogin_failure() {
],
});
Services.fog.testResetFOG();
+ const waitForGleanEvent = BrowserTestUtils.waitForCondition(
+ () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 1,
+ "rust_mirror_status event has been emitted"
+ );
+
// This login will be accepted by JSON but rejected by Rust
const badLogin = LoginTestUtils.testData.formLogin({
origin: ".",
@@ -529,13 +535,9 @@ add_task(async function test_rust_mirror_addLogin_failure() {
"single dot origin login saved to JSON"
);
- await BrowserTestUtils.waitForCondition(
- () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 1,
- "event has been emitted"
- );
+ await waitForGleanEvent;
const rustStorage = new LoginManagerRustStorage();
-
const allLogins = await rustStorage.getAllLogins();
Assert.equal(
allLogins.length,
@@ -544,18 +546,34 @@ add_task(async function test_rust_mirror_addLogin_failure() {
);
const [evt] = Glean.pwmgr.rustMirrorStatus.testGetValue();
- Assert.ok(evt, "event has been emitted");
- Assert.equal(evt.extra?.operation, "add", "event has operation");
- Assert.equal(evt.extra?.status, "failure", "event has status=failure");
Assert.equal(
- evt.extra?.error_message,
- "Invalid login: Login has illegal origin",
+ evt.extra?.operation,
+ "add",
+ "rust_mirror_status event has operation"
+ );
+ Assert.equal(
+ evt.extra?.status,
+ "failure",
+ "rust_mirror_status event has status=failure"
+ );
+
+ const [evt1] = Glean.pwmgr.rustWriteFailure.testGetValue();
+ Assert.equal(
+ evt1.extra?.error_message,
+ "Login has illegal origin",
"event has error_message"
);
- Assert.equal(evt.extra?.poisoned, "false", "event is not poisoned");
- Assert.equal(evt.name, "rust_mirror_status", "event has name");
+ Assert.equal(
+ evt1.extra?.poisoned,
+ "false",
+ "rust_write_failure event is not poisoned"
+ );
// produce another failure
+ const waitForSecondGleanEvent = BrowserTestUtils.waitForCondition(
+ () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 2,
+ "two events have been emitted"
+ );
const badLogin2 = LoginTestUtils.testData.formLogin({
username: "another-bad-login",
origin: ".",
@@ -563,21 +581,22 @@ add_task(async function test_rust_mirror_addLogin_failure() {
});
await Services.logins.addLoginAsync(badLogin2);
- await BrowserTestUtils.waitForCondition(
- () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 2,
- "two events have been emitted"
- );
+ await waitForSecondGleanEvent;
// eslint-disable-next-line no-unused-vars
- const [_, evt2] = Glean.pwmgr.rustMirrorStatus.testGetValue();
- Assert.equal(evt2.extra?.poisoned, "true", "event is poisoned now");
+ const [_, evt3] = Glean.pwmgr.rustWriteFailure.testGetValue();
+ Assert.equal(
+ evt3.extra?.poisoned,
+ "true",
+ "rust_write_failure event is poisoned now"
+ );
LoginTestUtils.clearData();
await SpecialPowers.flushPrefEnv();
});
/*
- * Tests that we collect telemetry if non-ASCII origins get punycoded.
+ * Tests that we store non-ASCII origins get punycoded.
*/
add_task(async function test_punycode_origin_metric() {
// ensure mirror is on
@@ -595,13 +614,15 @@ add_task(async function test_punycode_origin_metric() {
password: "pass1",
});
- await Services.logins.addLoginAsync(login);
-
- await BrowserTestUtils.waitForCondition(
- () => Glean.pwmgr.rustIncompatibleLoginFormat.testGetValue()?.length == 1,
+ const waitForGleanEvent = BrowserTestUtils.waitForCondition(
+ () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 1,
"event has been emitted"
);
+ await Services.logins.addLoginAsync(login);
+
+ await waitForGleanEvent;
+
const rustStorage = new LoginManagerRustStorage();
const allLogins = await rustStorage.getAllLogins();
@@ -613,18 +634,13 @@ add_task(async function test_punycode_origin_metric() {
"origin has been punicoded on the Rust side"
);
- const [evt] = Glean.pwmgr.rustIncompatibleLoginFormat.testGetValue();
- Assert.equal(evt.extra?.issue, "nonAsciiOrigin");
- Assert.equal(evt.extra?.operation, "add");
- Assert.ok("run_id" in evt.extra);
-
LoginTestUtils.clearData();
rustStorage.removeAllLogins();
await SpecialPowers.flushPrefEnv();
});
/*
- * Tests that we collect telemetry if non-ASCII formorigins get punycoded.
+ * Tests that we store non-ASCII formorigins get punycoded.
*/
add_task(async function test_punycode_formActionOrigin_metric() {
// ensure mirror is on
@@ -642,15 +658,16 @@ add_task(async function test_punycode_formActionOrigin_metric() {
password: "pass1",
});
- await Services.logins.addLoginAsync(login);
-
- await BrowserTestUtils.waitForCondition(
- () => Glean.pwmgr.rustIncompatibleLoginFormat.testGetValue()?.length == 1,
+ const waitForGleanEvent = BrowserTestUtils.waitForCondition(
+ () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 1,
"event has been emitted"
);
- const rustStorage = new LoginManagerRustStorage();
+ await Services.logins.addLoginAsync(login);
+
+ await waitForGleanEvent;
+ const rustStorage = new LoginManagerRustStorage();
const allLogins = await rustStorage.getAllLogins();
Assert.equal(allLogins.length, 1, "punicode origin login saved to Rust");
const [rustLogin] = allLogins;
@@ -660,11 +677,6 @@ add_task(async function test_punycode_formActionOrigin_metric() {
"origin has been punicoded on the Rust side"
);
- const [evt] = Glean.pwmgr.rustIncompatibleLoginFormat.testGetValue();
- Assert.equal(evt.extra?.issue, "nonAsciiFormAction");
- Assert.equal(evt.extra?.operation, "add");
- Assert.ok("run_id" in evt.extra);
-
LoginTestUtils.clearData();
rustStorage.removeAllLogins();
await SpecialPowers.flushPrefEnv();
@@ -689,17 +701,15 @@ add_task(async function test_single_dot_in_origin() {
password: "pass1",
});
- await Services.logins.addLoginAsync(login);
-
- await BrowserTestUtils.waitForCondition(
- () => Glean.pwmgr.rustIncompatibleLoginFormat.testGetValue()?.length == 1,
+ const waitForGleanEvent = BrowserTestUtils.waitForCondition(
+ () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 1,
"event has been emitted"
);
+ await Services.logins.addLoginAsync(login);
+ await waitForGleanEvent;
- const [evt] = Glean.pwmgr.rustIncompatibleLoginFormat.testGetValue();
- Assert.equal(evt.extra?.issue, "dotOrigin");
- Assert.equal(evt.extra?.operation, "add");
- Assert.ok("run_id" in evt.extra);
+ const [evt] = Glean.pwmgr.rustWriteFailure.testGetValue();
+ Assert.ok(evt.extra?.has_single_dot_origin);
LoginTestUtils.clearData();
await SpecialPowers.flushPrefEnv();
@@ -723,20 +733,21 @@ add_task(async function test_username_linebreak_metric() {
password: "pass1",
});
- await Services.logins.addLoginAsync(login);
-
- await BrowserTestUtils.waitForCondition(
- () => Glean.pwmgr.rustIncompatibleLoginFormat.testGetValue()?.length == 1,
+ const waitForGleanEvent = BrowserTestUtils.waitForCondition(
+ () => Glean.pwmgr.rustMirrorStatus.testGetValue()?.length == 1,
"event has been emitted"
);
-
- const [evt] = Glean.pwmgr.rustIncompatibleLoginFormat.testGetValue();
- Assert.equal(evt.extra?.issue, "usernameLineBreak");
- Assert.equal(evt.extra?.operation, "add");
- Assert.ok("run_id" in evt.extra);
+ await Services.logins.addLoginAsync(login);
+ await waitForGleanEvent;
+ const rustStorage = new LoginManagerRustStorage();
+ const allLogins = await rustStorage.getAllLogins();
+ Assert.equal(
+ allLogins.length,
+ 1,
+ "line break username origin login saved to Rust"
+ );
LoginTestUtils.clearData();
- const rustStorage = new LoginManagerRustStorage();
rustStorage.removeAllLogins();
await SpecialPowers.flushPrefEnv();
});
