Skip to content

Commit

Permalink
[UPM] Recording API error from GMS Core combined with Auth Error
Browse files Browse the repository at this point in the history
This way we can see how many of those errors overlap with what Chrome reports as an auth error.

(cherry picked from commit 82698aa)

Fixed: 1327668
Change-Id: Ie9e71e7662e92be060066bd1ee7c699d33540769
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3657311
Commit-Queue: Viktor Semeniuk <vsemeniuk@google.com>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1006482}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3666478
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Auto-Submit: Viktor Semeniuk <vsemeniuk@google.com>
Cr-Commit-Position: refs/branch-heads/5060@{#236}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Viktor Semeniuk authored and Chromium LUCI CQ committed May 25, 2022
1 parent 4eee3ba commit c0cb36b
Show file tree
Hide file tree
Showing 4 changed files with 137 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "components/sync/driver/sync_service.h"
#include "components/sync/driver/sync_user_settings.h"
#include "components/sync/model/proxy_model_type_controller_delegate.h"
#include "google_apis/gaia/google_service_auth_error.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace password_manager {
Expand Down Expand Up @@ -177,6 +178,23 @@ SuccessStatus GetSuccessStatusFromError(
return SuccessStatus::kError;
}

void RecordApiErrorInCombinationWithSyncStatus(
int error_code,
GoogleServiceAuthError sync_error) {
std::string histogram_suffix;
if (sync_error.IsPersistentError()) {
histogram_suffix = "PersistentAuthError";
} else if (sync_error.IsTransientError()) {
histogram_suffix = "TransientAuthError";
} else {
histogram_suffix = "NoAuthError";
}
base::UmaHistogramSparse(
"PasswordManager.PasswordStoreAndroidBackend.APIError." +
histogram_suffix,
error_code);
}

} // namespace

class PasswordStoreAndroidBackend::ClearAllLocalPasswordsMetricRecorder {
Expand Down Expand Up @@ -539,6 +557,7 @@ void PasswordStoreAndroidBackend::ClearAllLocalPasswords() {

void PasswordStoreAndroidBackend::OnSyncServiceInitialized(
syncer::SyncService* sync_service) {
sync_service_ = sync_service;
sync_service->AddObserver(sync_controller_delegate_.get());
}

Expand Down Expand Up @@ -579,7 +598,13 @@ void PasswordStoreAndroidBackend::OnError(JobId job_id,
absl::optional<JobReturnHandler> reply = GetAndEraseJob(job_id);
if (!reply.has_value())
return; // Task cleaned up after returning from background.
// TODO(crbug.com/1324588): DCHECK_EQ(api_error_code, 10) to catch dev errors.
if (error.api_error_code.has_value() && sync_service_) {
// TODO(crbug.com/1324588): DCHECK_EQ(api_error_code, 10) to catch dev
// errors.
DCHECK_EQ(AndroidBackendErrorType::kExternalError, error.type);
RecordApiErrorInCombinationWithSyncStatus(error.api_error_code.value(),
sync_service_->GetAuthError());
}
reply->RecordMetrics(std::move(error));
if (reply->Holds<LoginsOrErrorReply>()) {
main_task_runner_->PostTask(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,8 @@ class PasswordStoreAndroidBackend
// This object is the proxy to the JNI bridge that performs the API requests.
std::unique_ptr<PasswordStoreAndroidBackendBridge> bridge_;

raw_ptr<syncer::SyncService> sync_service_ = nullptr;

// Delegate to obtain sync status, and syncing account.
std::unique_ptr<SyncDelegate> sync_delegate_;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,98 @@ TEST_F(PasswordStoreAndroidBackendTest, OnExternalError) {
histogram_tester.ExpectBucketCount(kAPIErrorMetric, 11004, 1);
}

TEST_F(PasswordStoreAndroidBackendTest,
OnExternalErrorInCombinationWithNoSyncError) {
backend().InitBackend(PasswordStoreAndroidBackend::RemoteChangesReceived(),
base::RepeatingClosure(), base::DoNothing());

syncer::TestSyncService sync_service;
backend().OnSyncServiceInitialized(&sync_service);

ASSERT_FALSE(sync_service.GetAuthError().IsTransientError());
ASSERT_FALSE(sync_service.GetAuthError().IsPersistentError());

const JobId kJobId{1337};
base::HistogramTester histogram_tester;
base::MockCallback<LoginsOrErrorReply> mock_reply;
EXPECT_CALL(*bridge(), GetAllLogins).WillOnce(Return(kJobId));
backend().GetAllLoginsAsync(mock_reply.Get());
EXPECT_CALL(mock_reply,
Run(ExpectError(PasswordStoreBackendError::kUnspecified)));
AndroidBackendError error{AndroidBackendErrorType::kExternalError};
error.api_error_code = absl::optional<int>(11004);
consumer().OnError(kJobId, std::move(error));
RunUntilIdle();

constexpr char kAPIErrorMetric[] =
"PasswordManager.PasswordStoreAndroidBackend.APIError.NoAuthError";

histogram_tester.ExpectBucketCount(kAPIErrorMetric, 11004, 1);
}

TEST_F(PasswordStoreAndroidBackendTest,
OnExternalErrorInCombinationWithTransientSyncError) {
backend().InitBackend(PasswordStoreAndroidBackend::RemoteChangesReceived(),
base::RepeatingClosure(), base::DoNothing());

syncer::TestSyncService sync_service;
backend().OnSyncServiceInitialized(&sync_service);

GoogleServiceAuthError transient_error(
GoogleServiceAuthError::CONNECTION_FAILED);
ASSERT_TRUE(transient_error.IsTransientError());
sync_service.SetAuthError(transient_error);

const JobId kJobId{1337};
base::HistogramTester histogram_tester;
base::MockCallback<LoginsOrErrorReply> mock_reply;
EXPECT_CALL(*bridge(), GetAllLogins).WillOnce(Return(kJobId));
backend().GetAllLoginsAsync(mock_reply.Get());
EXPECT_CALL(mock_reply,
Run(ExpectError(PasswordStoreBackendError::kUnspecified)));
AndroidBackendError error{AndroidBackendErrorType::kExternalError};
error.api_error_code = absl::optional<int>(11004);
consumer().OnError(kJobId, std::move(error));
RunUntilIdle();

constexpr char kAPIErrorMetric[] =
"PasswordManager.PasswordStoreAndroidBackend.APIError.TransientAuthError";

histogram_tester.ExpectBucketCount(kAPIErrorMetric, 11004, 1);
}

TEST_F(PasswordStoreAndroidBackendTest,
OnExternalErrorInCombinationWithPersistentSyncError) {
backend().InitBackend(PasswordStoreAndroidBackend::RemoteChangesReceived(),
base::RepeatingClosure(), base::DoNothing());

syncer::TestSyncService sync_service;
backend().OnSyncServiceInitialized(&sync_service);

GoogleServiceAuthError persistent_error(
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS);
ASSERT_TRUE(persistent_error.IsPersistentError());
sync_service.SetAuthError(persistent_error);

const JobId kJobId{1337};
base::HistogramTester histogram_tester;
base::MockCallback<LoginsOrErrorReply> mock_reply;
EXPECT_CALL(*bridge(), GetAllLogins).WillOnce(Return(kJobId));
backend().GetAllLoginsAsync(mock_reply.Get());
EXPECT_CALL(mock_reply,
Run(ExpectError(PasswordStoreBackendError::kUnspecified)));
AndroidBackendError error{AndroidBackendErrorType::kExternalError};
error.api_error_code = absl::optional<int>(11004);
consumer().OnError(kJobId, std::move(error));
RunUntilIdle();

constexpr char kAPIErrorMetric[] =
"PasswordManager.PasswordStoreAndroidBackend.APIError."
"PersistentAuthError";

histogram_tester.ExpectBucketCount(kAPIErrorMetric, 11004, 1);
}

TEST_F(PasswordStoreAndroidBackendTest, DisableAutoSignInForOrigins) {
EnableSyncForTestAccount();
base::HistogramTester histogram_tester;
Expand Down
17 changes: 17 additions & 0 deletions tools/metrics/histograms/metadata/password/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2213,6 +2213,23 @@ chromium-metrics-reviews@google.com.
</summary>
</histogram>

<histogram
name="PasswordManager.PasswordStoreAndroidBackend.APIError.{SyncErrorStatus}"
enum="PasswordStoreAndroidBackendAPIError" expires_after="2022-06-30">
<owner>fhorschig@chromium.org</owner>
<owner>vasilii@chromium.org</owner>
<summary>
The error codes returned by the GMS Core ChromeSync 1P API when
{SyncErrorStatus} is returned by SyncService.GetAuthError(). Recorded when
the asynchronous job has returned.
</summary>
<token key="SyncErrorStatus">
<variant name="NoAuthError" summary="no error"/>
<variant name="PersistentAuthError" summary="a persistent auth error"/>
<variant name="TransientAuthError" summary="a transient auth error"/>
</token>
</histogram>

<histogram
name="PasswordManager.PasswordStoreAndroidBackend.ClearAllLocalPasswords.LoginsToRemove"
units="count" expires_after="2022-09-11">
Expand Down

0 comments on commit c0cb36b

Please sign in to comment.