Skip to content

Commit

Permalink
[M103][UPM] Report password checkup errors for the GMS backend.
Browse files Browse the repository at this point in the history
Password checkup errors codes returned by the GMS Core 1P API are now
reported as a separate metric.

(cherry picked from commit 41071c8)

Bug: 1325225
Change-Id: Ic5a28609fa3b61c0e23e168d3c13f861d3b28a0d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3645073
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: Andrey Zaytsev <andzaytsev@google.com>
Commit-Queue: Maxim Anufriev <maxan@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1007300}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3676993
Auto-Submit: Maxim Anufriev <maxan@google.com>
Cr-Commit-Position: refs/branch-heads/5060@{#378}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Maxim Anufriev authored and Chromium LUCI CQ committed May 30, 2022
1 parent 9e31291 commit 3846007
Show file tree
Hide file tree
Showing 11 changed files with 584 additions and 47 deletions.
2 changes: 2 additions & 0 deletions chrome/browser/password_manager/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ android_library("java") {

sources = [
"java/src/org/chromium/chrome/browser/password_manager/ConfirmationDialogHelper.java",
"java/src/org/chromium/chrome/browser/password_manager/PasswordCheckupClientMetricsRecorder.java",
"java/src/org/chromium/chrome/browser/password_manager/PasswordManagerAndroidBackendUtil.java",
"java/src/org/chromium/chrome/browser/password_manager/PasswordManagerHelper.java",
"java/src/org/chromium/chrome/browser/password_manager/PasswordManagerLifecycleHelper.java",
Expand Down Expand Up @@ -158,6 +159,7 @@ junit_binary("password_manager_junit_tests") {
testonly = true

sources = [
"junit/src/org/chromium/chrome/browser/password_manager/PasswordCheckupClientMetricsRecorderTest.java",
"junit/src/org/chromium/chrome/browser/password_manager/PasswordManagerAndroidBackendUtilTest.java",
"junit/src/org/chromium/chrome/browser/password_manager/PasswordManagerHelperTest.java",
"junit/src/org/chromium/chrome/browser/password_manager/PasswordManagerLifecycleHelperTest.java",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ public interface CredentialManagerLauncher {
* in enums.xml.
*/
@IntDef({CredentialManagerError.NO_CONTEXT, CredentialManagerError.NO_ACCOUNT_NAME,
CredentialManagerError.API_ERROR, CredentialManagerError.COUNT})
CredentialManagerError.API_ERROR, CredentialManagerError.UNCATEGORIZED,
CredentialManagerError.COUNT})
@Retention(RetentionPolicy.SOURCE)
public @interface CredentialManagerError {
// There is no application context.
Expand All @@ -31,7 +32,9 @@ public interface CredentialManagerLauncher {
int NO_ACCOUNT_NAME = 1;
// Error encountered after calling the API to fetch the launch intent.
int API_ERROR = 2;
int COUNT = 3;
// Error is not categorized.
int UNCATEGORIZED = 3;
int COUNT = 4;
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
// Copyright 2022 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

package org.chromium.chrome.browser.password_manager;

import com.google.common.base.Optional;

import org.chromium.base.metrics.RecordHistogram;
import org.chromium.chrome.browser.password_manager.CredentialManagerLauncher.CredentialManagerError;
import org.chromium.chrome.browser.password_manager.PasswordManagerHelper.PasswordCheckOperation;

/**
* Records metrics for an asynchronous job or a series of jobs. The job is expected to have started
* when the {@link PasswordCheckupClientMetricsRecorder} instance is created. Histograms are
* recorded in {@link #recordMetrics(Exception) recordMetrics}.
*
* TODO(crbug.com/1326151): Report success and latency metrics.
*/
class PasswordCheckupClientMetricsRecorder {
private static final String PASSWORD_CHECKUP_HISTOGRAM_BASE = "PasswordManager.PasswordCheckup";
private static final String RUN_PASSWORD_CHECKUP_OPERATION_SUFFIX = "RunPasswordCheckup";
private static final String GET_BREACHED_CREDENTIALS_COUNT_OPERATION_SUFFIX =
"GetBreachedCredentialsCount";
private static final String GET_PASSWORD_CHECKUP_INTENT_OPERATION_SUFFIX = "GetIntent";

private final @PasswordCheckOperation int mOperation;

PasswordCheckupClientMetricsRecorder(@PasswordCheckOperation int operation) {
mOperation = operation;
}

/**
* Records the metrics depending on {@link Exception} provided.
* Error codes are reported for all errors. For GMS errors, API error code is additionally
* reported.
*
* @param exception {@link Exception} instance corresponding to the occurred error
*/
void recordMetrics(Optional<Exception> exception) {
// TODO(crbug.com/1326506): Record success and latency metrics.
if (exception.isPresent()) {
recordErrorMetrics(exception.get());
}
}

private void recordErrorMetrics(Exception exception) {
@CredentialManagerError
int error = PasswordManagerAndroidBackendUtil.getPasswordCheckupBackendError(exception);
RecordHistogram.recordEnumeratedHistogram(
getHistogramName("Error"), error, CredentialManagerError.COUNT);

if (error == CredentialManagerError.API_ERROR) {
int apiErrorCode = PasswordManagerAndroidBackendUtil.getApiErrorCode(exception);
RecordHistogram.recordSparseHistogram(getHistogramName("APIError"), apiErrorCode);
}
}

private String getHistogramName(String metric) {
return PASSWORD_CHECKUP_HISTOGRAM_BASE + "." + getSuffixForOperation(mOperation) + "."
+ metric;
}

/** Returns histogram suffix (infix in real) for a given operation. */
private static String getSuffixForOperation(@PasswordCheckOperation int operation) {
switch (operation) {
case PasswordCheckOperation.RUN_PASSWORD_CHECKUP:
return RUN_PASSWORD_CHECKUP_OPERATION_SUFFIX;
case PasswordCheckOperation.GET_BREACHED_CREDENTIALS_COUNT:
return GET_BREACHED_CREDENTIALS_COUNT_OPERATION_SUFFIX;
case PasswordCheckOperation.GET_PASSWORD_CHECKUP_INTENT:
return GET_PASSWORD_CHECKUP_INTENT_OPERATION_SUFFIX;
default:
throw new AssertionError("All operations need to be handled.");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import com.google.android.gms.common.api.ResolvableApiException;

import org.chromium.base.Log;
import org.chromium.chrome.browser.password_manager.CredentialManagerLauncher.CredentialManagerError;

/**
* Collection of utilities used by classes interacting with the password manager backend
Expand All @@ -31,6 +32,17 @@ private PasswordManagerAndroidBackendUtil() {}
return AndroidBackendErrorType.UNCATEGORIZED;
}

static @CredentialManagerError int getPasswordCheckupBackendError(Exception exception) {
if (exception instanceof PasswordCheckupClientHelper.PasswordCheckBackendException) {
return ((PasswordCheckupClientHelper.PasswordCheckBackendException) exception)
.errorCode;
}
if (exception instanceof ApiException) {
return CredentialManagerError.API_ERROR;
}
return CredentialManagerError.UNCATEGORIZED;
}

static int getApiErrorCode(Exception exception) {
if (exception instanceof ApiException) {
return ((ApiException) exception).getStatusCode();
Expand All @@ -54,4 +66,4 @@ static void handleResolvableApiException(ResolvableApiException exception) {
Log.e(TAG, "Can not launch error resolution intent", e);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import com.google.common.base.Optional;

import org.chromium.base.Callback;
import org.chromium.base.metrics.RecordHistogram;
import org.chromium.base.supplier.ObservableSupplier;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
Expand All @@ -42,6 +43,20 @@ public class PasswordManagerHelper {
// all points of entry to the passwords settings.
public static final String MANAGE_PASSWORDS_REFERRER = "manage-passwords-referrer";

// Indicates the operation that was requested from the {@link PasswordCheckupClientHelper}.
@IntDef({PasswordCheckOperation.RUN_PASSWORD_CHECKUP,
PasswordCheckOperation.GET_BREACHED_CREDENTIALS_COUNT,
PasswordCheckOperation.GET_PASSWORD_CHECKUP_INTENT})
@Retention(RetentionPolicy.SOURCE)
public @interface PasswordCheckOperation {
/** Run password checkup. */
int RUN_PASSWORD_CHECKUP = 0;
/** Obtain the number of breached credentials. */
int GET_BREACHED_CREDENTIALS_COUNT = 1;
/** Obtain pending intent for launching password checkup UI */
int GET_PASSWORD_CHECKUP_INTENT = 2;
}

private static final String UPM_VARIATION_FEATURE_PARAM = "stage";

// Loading dialog is dismissed with this delay after sending an intent to prevent
Expand Down Expand Up @@ -74,8 +89,6 @@ public class PasswordManagerHelper {
"PasswordManager.PasswordCheckup.GetIntent.Latency";
private static final String PASSWORD_CHECKUP_GET_INTENT_SUCCESS_HISTOGRAM =
"PasswordManager.PasswordCheckup.GetIntent.Success";
private static final String PASSWORD_CHECKUP_GET_INTENT_ERROR_HISTOGRAM =
"PasswordManager.PasswordCheckup.GetIntent.Error";
private static final String PASSWORD_CHECKUP_LAUNCH_CREDENTIAL_MANAGER_SUCCESS_HISTOGRAM =
"PasswordManager.PasswordCheckup.Launch.Success";

Expand All @@ -84,6 +97,8 @@ public class PasswordManagerHelper {
private static final String LOADING_DIALOG_PASSWORD_CHECKUP_HISTOGRAM =
"PasswordManager.ModalLoadingDialog.PasswordCheckup.Outcome";

private static PasswordCheckupClientMetricsRecorder sPasswordCheckupMetricsRecorderForTesting;

/**
* The identifier of the loading dialog outcome.
*
Expand Down Expand Up @@ -160,6 +175,54 @@ public static void showPasswordCheckup(Context context, @PasswordCheckReferrer i
launchPasswordCheckup(referrer, checkupClient, account, loadingDialogCoordinator);
}

/**
* Asynchronously runs Password Checkup in GMS Core and stores the result in
* PasswordSpecifics then saves it to the ChromeSync module.
*
* @param referrer the place that requested to start a check.
* @param checkupClient the {@link PasswordCheckupClientHelper} instance to launch the checkup
* with.
* @param accountName the account name that is syncing passwords. If no value was provided local
* account will be used
* @param successCallback callback called when password check finishes successfully
* @param failureCallback callback called if password check encountered an error
*/
public static void runPasswordCheckupInBackground(@PasswordCheckReferrer int referrer,
PasswordCheckupClientHelper checkupClient, Optional<String> accountName,
Callback<Void> successCallback, Callback<Exception> failureCallback) {
PasswordCheckupClientMetricsRecorder passwordCheckupMetricsRecorder =
new PasswordCheckupClientMetricsRecorder(
PasswordCheckOperation.RUN_PASSWORD_CHECKUP);
checkupClient.runPasswordCheckupInBackground(
referrer, accountName, successCallback, error -> {
passwordCheckupMetricsRecorder.recordMetrics(Optional.of(error));
failureCallback.onResult(error);
});
}

/**
* Asynchronously returns the number of breached credentials for the provided account.
*
* @param referrer the place that requested number of breached credentials.
* @param checkupClient the {@link PasswordCheckupClientHelper} instance to request the count
* with.
* @param accountName the account name that is syncing passwords. If no value was provided local
* account will be used.
* @param successCallback callback called with the number of breached passwords.
* @param failureCallback callback called if encountered an error.
*/
public static void getBreachedCredentialsCount(@PasswordCheckReferrer int referrer,
PasswordCheckupClientHelper checkupClient, Optional<String> accountName,
Callback<Integer> successCallback, Callback<Exception> failureCallback) {
PasswordCheckupClientMetricsRecorder passwordCheckupMetricsRecorder =
new PasswordCheckupClientMetricsRecorder(
PasswordCheckOperation.GET_BREACHED_CREDENTIALS_COUNT);
checkupClient.getBreachedCredentialsCount(referrer, accountName, successCallback, error -> {
passwordCheckupMetricsRecorder.recordMetrics(Optional.of(error));
failureCallback.onResult(error);
});
}

/**
* Checks whether the sync feature is enabled and the user has chosen to sync passwords.
* Note that this doesn't mean that passwords are actively syncing.
Expand Down Expand Up @@ -250,18 +313,18 @@ static void launchPasswordCheckup(@PasswordCheckReferrer int referrer,
assert checkupClient != null;

loadingDialogCoordinator.show();

PasswordCheckupClientMetricsRecorder passwordCheckupMetricsRecorder =
new PasswordCheckupClientMetricsRecorder(
(PasswordCheckOperation.GET_PASSWORD_CHECKUP_INTENT));
long startTimeMs = SystemClock.elapsedRealtime();
checkupClient.getPasswordCheckupPendingIntent(referrer, account,
checkupClient.getPasswordCheckupIntent(referrer, account,
(intent)
-> PasswordManagerHelper.launchPasswordCheckupIntent(
intent, startTimeMs, loadingDialogCoordinator),
(error) -> {
RecordHistogram.recordBooleanHistogram(
PASSWORD_CHECKUP_GET_INTENT_SUCCESS_HISTOGRAM, false);
RecordHistogram.recordEnumeratedHistogram(
PASSWORD_CHECKUP_GET_INTENT_ERROR_HISTOGRAM, error,
CredentialManagerError.COUNT);
passwordCheckupMetricsRecorder.recordMetrics(Optional.of(error));
recordLoadingDialogMetrics(LOADING_DIALOG_PASSWORD_CHECKUP_HISTOGRAM,
loadingDialogCoordinator.getState());
loadingDialogCoordinator.dismiss();
Expand Down

0 comments on commit 3846007

Please sign in to comment.