Skip to content

Commit

Permalink
Add new histograms to report errors initializing SQLite database.
Browse files Browse the repository at this point in the history
This CL adds two new histograms that report more details on failed
SQLitePersistentStore DB initialization. These new metrics are
reported for both instantiations, ReportingAndNEL and Cookie.

The first, ErrorInitializeDB is the SQLite error code upon
initialization failure and is reported on all platforms.

The second, WinGetLastErrorInitializeDB is the ::GetLastError returned
on the DB thread after attempting to load the database, and is only
reported on Windows.

A test is added to verify this behavior, along with verifying that
the new histograms are only emitted in the failure case.

UmaHistogramSqliteResult is updated to take a const std::string ref
to match the underlying UMA functions being called.

BUG=1429117

Change-Id: I1bc2c898b005232305cebfde80a0dd8d4cfafd68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4608890
Reviewed-by: Austin Sullivan <asully@chromium.org>
Reviewed-by: Chris Fredrickson <cfredric@chromium.org>
Commit-Queue: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1157192}
  • Loading branch information
wfh-chromium authored and Chromium LUCI CQ committed Jun 13, 2023
1 parent c21c033 commit b60653c
Show file tree
Hide file tree
Showing 6 changed files with 124 additions and 2 deletions.
54 changes: 54 additions & 0 deletions net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "base/task/single_thread_task_runner.h"
#include "base/task/thread_pool.h"
#include "base/test/bind.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h"
#include "base/threading/thread_restrictions.h"
#include "base/time/time.h"
Expand All @@ -46,6 +47,7 @@
#include "sql/meta_table.h"
#include "sql/statement.h"
#include "sql/transaction.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/abseil-cpp/absl/types/optional.h"
#include "url/gurl.h"
Expand Down Expand Up @@ -213,6 +215,10 @@ class SQLitePersistentCookieStoreTest : public TestWithTaskEnvironment {
void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); }

void TearDown() override {
if (!expect_init_errors_) {
EXPECT_THAT(histograms_.GetAllSamples("Cookie.ErrorInitializeDB"),
::testing::IsEmpty());
}
DestroyStore();
}

Expand All @@ -227,6 +233,8 @@ class SQLitePersistentCookieStoreTest : public TestWithTaskEnvironment {
base::ScopedTempDir temp_dir_;
scoped_refptr<SQLitePersistentCookieStore> store_;
std::unique_ptr<CookieCryptor> cookie_crypto_delegate_;
base::HistogramTester histograms_;
bool expect_init_errors_ = false;
};

TEST_F(SQLitePersistentCookieStoreTest, TestInvalidMetaTableRecovery) {
Expand Down Expand Up @@ -1484,6 +1492,7 @@ TEST_F(SQLitePersistentCookieStoreTest, OpsIfInitFailed) {
EXPECT_TRUE(set_cookie_callback.result().status.IsInclude());

// Things should commit once going out of scope.
expect_init_errors_ = true;
}

TEST_F(SQLitePersistentCookieStoreTest, Coalescing) {
Expand Down Expand Up @@ -1631,6 +1640,32 @@ TEST_P(SQLitePersistentCookieStoreExclusiveAccessTest, LockedStore) {
db_thread_event_.Signal();
}

TEST_P(SQLitePersistentCookieStoreExclusiveAccessTest, LockedStoreAlreadyOpen) {
base::HistogramTester histograms;
base::File file(
temp_dir_.GetPath().Append(kCookieFilename),
base::File::Flags::FLAG_CREATE | base::File::Flags::FLAG_READ);
ASSERT_TRUE(file.IsValid());

Create(false, false, true /* want current thread to invoke the store. */,
/* exclusive access */ ShouldBeExclusive());

base::RunLoop run_loop;
store_->Load(base::BindLambdaForTesting(
[&](CanonicalCookieVector cookies) { run_loop.Quit(); }),
NetLogWithSource());
run_loop.Run();

// Note: The non-exclusive path is verified in the TearDown for the fixture.
if (ShouldBeExclusive()) {
expect_init_errors_ = true;
histograms.ExpectUniqueSample("Cookie.ErrorInitializeDB",
sql::SqliteLoggedResultCode::kCantOpen, 1);
histograms.ExpectUniqueSample("Cookie.WinGetLastErrorInitializeDB",
ERROR_SHARING_VIOLATION, 1);
}
}

INSTANTIATE_TEST_SUITE_P(All,
SQLitePersistentCookieStoreExclusiveAccessTest,
::testing::Bool(),
Expand All @@ -1640,6 +1675,25 @@ INSTANTIATE_TEST_SUITE_P(All,

#endif // BUILDFLAG(IS_WIN)

TEST_F(SQLitePersistentCookieStoreTest, CorruptStore) {
base::HistogramTester histograms;
base::WriteFile(temp_dir_.GetPath().Append(kCookieFilename),
"SQLite format 3 foobarfoobarfoobar");

Create(false, false, true /* want current thread to invoke the store. */,
false);

base::RunLoop run_loop;
store_->Load(base::BindLambdaForTesting(
[&](CanonicalCookieVector cookies) { run_loop.Quit(); }),
NetLogWithSource());
run_loop.Run();

expect_init_errors_ = true;
histograms.ExpectUniqueSample("Cookie.ErrorInitializeDB",
sql::SqliteLoggedResultCode::kNotADatabase, 1);
}

bool CreateV10Schema(sql::Database* db) {
sql::MetaTable meta_table;
if (!meta_table.Init(db, /* version = */ 10,
Expand Down
13 changes: 13 additions & 0 deletions net/extras/sqlite/sqlite_persistent_store_backend_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@
#include "sql/database.h"
#include "sql/error_delegate_util.h"

#if BUILDFLAG(IS_WIN)
#include <windows.h>
#endif // BUILDFLAG(IS_WIN)

namespace net {

SQLitePersistentStoreBackendBase::SQLitePersistentStoreBackendBase(
Expand Down Expand Up @@ -262,6 +266,15 @@ void SQLitePersistentStoreBackendBase::DatabaseErrorCallback(

corruption_detected_ = true;

if (!initialized_) {
sql::UmaHistogramSqliteResult(histogram_tag_ + ".ErrorInitializeDB", error);

#if BUILDFLAG(IS_WIN)
base::UmaHistogramSparse(histogram_tag_ + ".WinGetLastErrorInitializeDB",
::GetLastError());
#endif // BUILDFLAG(IS_WIN)
}

// Don't just do the close/delete here, as we are being called by |db| and
// that seems dangerous.
// TODO(shess): Consider just calling RazeAndPoison() immediately.
Expand Down
3 changes: 2 additions & 1 deletion sql/sqlite_result_code.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <ostream> // Needed to compile NOTREACHED() with operator <<.
#include <set>
#include <string>
#include <utility>

#include "base/check_op.h"
Expand Down Expand Up @@ -397,7 +398,7 @@ SqliteLoggedResultCode ToSqliteLoggedResultCode(int sqlite_result_code) {
return logged_code;
}
void UmaHistogramSqliteResult(const char* histogram_name,
void UmaHistogramSqliteResult(const std::string& histogram_name,
int sqlite_result_code) {
auto logged_code = ToSqliteLoggedResultCode(sqlite_result_code);
base::UmaHistogramEnumeration(histogram_name, logged_code);
Expand Down
3 changes: 2 additions & 1 deletion sql/sqlite_result_code.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define SQL_SQLITE_RESULT_CODE_H_

#include <iosfwd>
#include <string>

#include "base/component_export.h"
#include "base/dcheck_is_on.h"
Expand Down Expand Up @@ -83,7 +84,7 @@ bool IsSqliteSuccessCode(SqliteResultCode sqlite_result_code);
// Works for all result codes, including success codes and extended error codes.
// DCHECKs if provided result code should not occur in Chrome's usage of SQLite.
COMPONENT_EXPORT(SQL)
void UmaHistogramSqliteResult(const char* histogram_name,
void UmaHistogramSqliteResult(const std::string& histogram_name,
int sqlite_result_code);

// Converts a SQLite result code into a UMA logging-friendly form.
Expand Down
27 changes: 27 additions & 0 deletions tools/metrics/histograms/metadata/cookie/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,19 @@ chromium-metrics-reviews@google.com.
</summary>
</histogram>

<histogram name="Cookie.ErrorInitializeDB" enum="SqliteLoggedResultCode"
expires_after="2023-11-12">
<owner>wfh@chromium.org</owner>
<owner>src/net/cookies/OWNERS</owner>
<summary>
The exact database error encountered if initializing the cookies database
fails catastrophically. Initializing the cookies database happens once per
network context configured with persistent cookies, so this metric could be
reported multiple times from the same client. Catastrophic errors are
defined in sql::IsErrorCatastrophic.
</summary>
</histogram>

<histogram name="Cookie.ExpirationDuration400DaysGT" units="days"
expires_after="2024-02-20">
<owner>arichiv@chromium.org</owner>
Expand Down Expand Up @@ -909,6 +922,20 @@ chromium-metrics-reviews@google.com.
<summary>For each cookie added to the store, record it's type(s).</summary>
</histogram>

<histogram name="Cookie.WinGetLastErrorInitializeDB" enum="WinGetLastError"
expires_after="2023-11-12">
<owner>wfh@chromium.org</owner>
<owner>src/net/cookies/OWNERS</owner>
<summary>
The result of calling ::GetLastError if initializing the cookies database
fails catastrophically. Initializing the cookies database happens once per
network context configured with persistent cookies, so this metric could be
reported multiple times from the same client. Catastrophic errors are
defined in sql::IsErrorCatastrophic. This metric is only reported on
Windows.
</summary>
</histogram>

</histograms>

</histogram-configuration>
26 changes: 26 additions & 0 deletions tools/metrics/histograms/metadata/others/histograms.xml
Original file line number Diff line number Diff line change
Expand Up @@ -10561,6 +10561,19 @@ chromium-metrics-reviews@google.com.
</summary>
</histogram>

<histogram name="ReportingAndNEL.ErrorInitializeDB"
enum="SqliteLoggedResultCode" expires_after="2023-11-12">
<owner>wfh@chromium.org</owner>
<owner>src/net/reporting/OWNERS</owner>
<summary>
The exact database error encountered when initializing the Reporting and NEL
database, if initialization fails catastrophically. This is recorded if the
database initialization fails catastrophically when the DB initialization
attempt is made, which typically occurs upon the first network request after
startup. Catastrophic errors are defined in sql::IsErrorCatastrophic.
</summary>
</histogram>

<histogram name="ReportingAndNEL.NumberOfLoadedNELPolicies"
units="policy count" expires_after="2023-06-25">
<owner>yhirano@chromium.org</owner>
Expand Down Expand Up @@ -10618,6 +10631,19 @@ chromium-metrics-reviews@google.com.
</summary>
</histogram>

<histogram name="ReportingAndNEL.WinGetLastErrorInitializeDB"
enum="WinGetLastError" expires_after="2023-11-12">
<owner>wfh@chromium.org</owner>
<owner>src/net/reporting/OWNERS</owner>
<summary>
The result of calling ::GetLastError() if initializing the Reporting and NEL
database fails catastrophically. This is recorded, on Windows only, if the
database initialization fails when the DB initialization attempt is made,
which typically occurs upon the first network request after startup.
Catastrophic errors are defined in sql::IsErrorCatastrophic.
</summary>
</histogram>

<histogram name="Reset.ChromeOS.PowerwashDialogShown"
enum="PowerwashDialogViewType" expires_after="M77">
<owner>merkulova@chromium.org</owner>
Expand Down

0 comments on commit b60653c

Please sign in to comment.