Skip to content

Commit

Permalink
[M110] Merge to release branch to run DIPS experiment in M110 (2/3)
Browse files Browse the repository at this point in the history 
This cherry pick combines 4 CLs that need to be merged into M110 in
order to collect metrics and experimentially do DIPS deletion.

This CL relies on https://crrev.com/c/4133655 being upstream.

Bug: 1404964

The combined CLs descriptions are listed below in descending chronological order.

[DIPS] Add new UMA and log them when the DIPS timer fires.

The added metrics measure the number of sites that would have their data cleared when the timer fires, and how long it would take to do so.

Bug: 1375302
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4060624
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Kirubel Aklilu <kaklilu@chromium.org>
Reviewed-by: Joshua Hood <jdh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1087229}
(cherry picked from commit bc84ebd)


[DIPS] Improve IsWithinOrNull to fix bad optional accesses

Bug: 1403145
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4121246
Auto-Submit: Kirubel Aklilu <kaklilu@chromium.org>
Commit-Queue: Joshua Hood <jdh@chromium.org>
Reviewed-by: Joshua Hood <jdh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1086515}
(cherry picked from commit f754ae0)


[DIPS] Update DIPS querying methods to exclude/"protect" some sites.

Since the sites returned by these querying methods will have their data
cleared, if a site isn't returned then its "protected" from DIPS.

A site can be protected in several ways:
- it's still in its grace period following a DIPS-triggering event
  (storage, bounce, or bounce with storage) when the query is run
- it received user interaction before triggering DIPS
- it received user interaction in the grace period after triggering DIPS

If a site is protected by an interaction, it maintains that status until that interaction expires (after `interaction_ttl`).

This CL also updates these methods in several ways as was discussed
offline such as:
- removing range_start in favor of scanning the whole DB
  - this is feasible since the DB will be kept small from clearing
    expired interactions and clearing rows as their data is cleared
- removing last_interaction in favor of directly inserting the
  result of `Now() - interactionTtl` to check if the interaction is
  expired
- using the new (first|last)_bounce_time columns defined in v2 of the
  DIPS database schema

Unit tests are added for the querying methods to verify that the
`grace_period` and `interaction_ttl` feature params are used by each method as intended.

Bug: 1375302
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4111381
Reviewed-by: Ryan Tarpine <rtarpine@chromium.org>
Reviewed-by: Joshua Hood <jdh@chromium.org>
Commit-Queue: Kirubel Aklilu <kaklilu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1085996}
(cherry picked from commit 4645332)


sql: Add BindTimeDelta() and ColumnTimeDelta() to sql::Statement

These helpers use the recommended approach to serialize/deserialize a
base::TimeDelta to/from int64.

These methods are added to encourage usage of built-in ways to insert
and retrieve a TimeDelta from a sql database, and discourage usage of
the `statement.BindInt64(delta.ToInternalValue())` pattern since TimeDelta::ToInternalValue is deprecated.

Bug: 1402777, 1195962

(cherry picked from commit 97f422c)

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4118621
Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: manuk hovanesian <manukh@chromium.org>
Commit-Queue: Kirubel Aklilu <kaklilu@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1085968}
Change-Id: I72174903b913087e8575f72c14c9293db03328ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4134133
Reviewed-by: Joshua Hood <jdh@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#155}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
Kirubel Aklilu authored and Chromium LUCI CQ committed Jan 6, 2023
1 parent e902730 commit a14c2c2
Show file tree
Hide file tree
Showing 17 changed files with 543 additions and 334 deletions.
129 changes: 62 additions & 67 deletions chrome/browser/dips/dips_database.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,12 +334,12 @@ sql::InitStatus DIPSDatabase::Init() {
base::UmaHistogramExactLinear("Privacy.DIPS.DatabaseInit", attempts, 3);

last_health_metrics_time_ = clock_->Now();
ComputeDatabaseMetrics();
LogDatabaseMetrics();

return status;
}

void DIPSDatabase::ComputeDatabaseMetrics() {
void DIPSDatabase::LogDatabaseMetrics() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
base::TimeTicks start_time = base::TimeTicks::Now();

Expand All @@ -355,7 +355,7 @@ void DIPSDatabase::ComputeDatabaseMetrics() {
base::TimeTicks::Now() - start_time);
}

bool DIPSDatabase::CheckDBInit() {
bool DIPSDatabase::CheckDBInit() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!db_ || !db_->is_open())
return false;
Expand All @@ -365,7 +365,7 @@ bool DIPSDatabase::CheckDBInit() {
base::Time now = clock_->Now();
if (now > last_health_metrics_time_ + kMetricsInterval) {
last_health_metrics_time_ = now;
ComputeDatabaseMetrics();
LogDatabaseMetrics();
}

return true;
Expand Down Expand Up @@ -409,7 +409,7 @@ bool DIPSDatabase::Write(const std::string& site,
return statement.Run();
}

absl::optional<StateValue> DIPSDatabase::Read(const std::string& site) {
absl::optional<StateValue> DIPSDatabase::Read(const std::string& site) const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!CheckDBInit())
return absl::nullopt;
Expand Down Expand Up @@ -454,10 +454,11 @@ absl::optional<StateValue> DIPSDatabase::Read(const std::string& site) {
ColumnOptionalTime(&statement, 8)}};
}

std::vector<std::string> DIPSDatabase::GetAllSitesForTesting() {
std::vector<std::string> DIPSDatabase::GetAllSitesForTesting() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!CheckDBInit())
if (!CheckDBInit()) {
return {};
}

static constexpr char kReadSql[] = // clang-format off
"SELECT site FROM bounces ORDER BY site";
Expand All @@ -473,28 +474,25 @@ std::vector<std::string> DIPSDatabase::GetAllSitesForTesting() {
return sites;
}

std::vector<std::string> DIPSDatabase::GetSitesThatBounced(
base::Time range_start,
base::Time last_interaction) {
std::vector<std::string> DIPSDatabase::GetSitesThatBounced() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!CheckDBInit())
if (!CheckDBInit()) {
return {};
ClearRowsWithExpiredInteractions();

static constexpr char kReadSql[] = // clang-format off
}
static constexpr char kBounceSql[] = // clang-format off
"SELECT site FROM bounces "
"WHERE (last_stateful_bounce_time > ? "
"OR last_bounce_time > ?) AND "
"last_user_interaction_time < ? "
"ORDER BY site";
// clang-format on

DCHECK(db_->IsSQLValid(kReadSql));

sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kReadSql));
statement.BindTime(0, range_start);
statement.BindTime(1, range_start);
statement.BindTime(2, last_interaction);
"WHERE first_bounce_time < ? AND "
"(last_user_interaction_time IS NULL OR "
// Only return a protected site if its protection has expired.
// Note: protected => expired ≡ (NOT protected) OR expired.
"NOT(first_user_interaction_time <= first_bounce_time + ?) OR "
"last_user_interaction_time < ?) "
"ORDER BY site"; // clang-format on
DCHECK(db_->IsSQLValid(kBounceSql));
sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kBounceSql));
statement.BindTime(0, clock_->Now() - dips::kGracePeriod.Get());
statement.BindTimeDelta(1, dips::kGracePeriod.Get());
statement.BindTime(2, clock_->Now() - dips::kInteractionTtl.Get());

std::vector<std::string> sites;
while (statement.Step()) {
Expand All @@ -503,27 +501,26 @@ std::vector<std::string> DIPSDatabase::GetSitesThatBounced(
return sites;
}

std::vector<std::string> DIPSDatabase::GetSitesThatBouncedWithState(
base::Time range_start,
base::Time last_interaction) {
std::vector<std::string> DIPSDatabase::GetSitesThatBouncedWithState() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!CheckDBInit())
if (!CheckDBInit()) {
return {};
ClearRowsWithExpiredInteractions();

static constexpr char kReadSql[] = // clang-format off
}
static constexpr char kStatefulBounceSql[] = // clang-format off
"SELECT site FROM bounces "
"WHERE last_stateful_bounce_time > ? AND "
"last_site_storage_time > ? AND "
"last_user_interaction_time < ? "
"ORDER BY site";
// clang-format on
DCHECK(db_->IsSQLValid(kReadSql));

sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kReadSql));
statement.BindTime(0, range_start);
statement.BindTime(1, range_start);
statement.BindTime(2, last_interaction);
"WHERE first_stateful_bounce_time < ? AND "
"(last_user_interaction_time IS NULL OR "
// Only return a protected site if its protection has expired.
// Note: protected => expired ≡ (NOT protected) OR expired.
"NOT(first_user_interaction_time <= first_stateful_bounce_time + ?) OR "
"last_user_interaction_time < ?) "
"ORDER BY site"; // clang-format on
DCHECK(db_->IsSQLValid(kStatefulBounceSql));
sql::Statement statement(
db_->GetCachedStatement(SQL_FROM_HERE, kStatefulBounceSql));
statement.BindTime(0, clock_->Now() - dips::kGracePeriod.Get());
statement.BindTimeDelta(1, dips::kGracePeriod.Get());
statement.BindTime(2, clock_->Now() - dips::kInteractionTtl.Get());

std::vector<std::string> sites;
while (statement.Step()) {
Expand All @@ -532,27 +529,25 @@ std::vector<std::string> DIPSDatabase::GetSitesThatBouncedWithState(
return sites;
}

std::vector<std::string> DIPSDatabase::GetSitesThatUsedStorage(
base::Time range_start,
base::Time last_interaction) {
std::vector<std::string> DIPSDatabase::GetSitesThatUsedStorage() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!CheckDBInit())
if (!CheckDBInit()) {
return {};
ClearRowsWithExpiredInteractions();

static constexpr char kReadSql[] = // clang-format off
"SELECT site FROM bounces "
"WHERE (last_site_storage_time > ? OR "
"last_stateful_bounce_time > ?) AND "
"last_user_interaction_time < ? "
"ORDER BY site";
// clang-format on
DCHECK(db_->IsSQLValid(kReadSql));

sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kReadSql));
statement.BindTime(0, range_start);
statement.BindTime(1, range_start);
statement.BindTime(2, last_interaction);
}
static constexpr char kStorageSql[] = // clang-format off
"SELECT site FROM bounces "
"WHERE first_site_storage_time < ? AND "
"(last_user_interaction_time IS NULL OR "
// Only return a protected site if its protection has expired.
// Note: protected => expired ≡ (NOT protected) OR expired.
"NOT(first_user_interaction_time <= first_site_storage_time + ?) OR "
"last_user_interaction_time < ?) "
"ORDER BY site"; // clang-format on
DCHECK(db_->IsSQLValid(kStorageSql));
sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, kStorageSql));
statement.BindTime(0, clock_->Now() - dips::kGracePeriod.Get());
statement.BindTimeDelta(1, dips::kGracePeriod.Get());
statement.BindTime(2, clock_->Now() - dips::kInteractionTtl.Get());

std::vector<std::string> sites;
while (statement.Step()) {
Expand Down Expand Up @@ -959,11 +954,10 @@ bool DIPSDatabase::RemoveEmptyRows() {
return s_clean.Run();
}

size_t DIPSDatabase::GetEntryCount() {
size_t DIPSDatabase::GetEntryCount() const {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
if (!CheckDBInit())
return 0;
ClearRowsWithExpiredInteractions();

sql::Statement s_entry_count(
db_->GetCachedStatement(SQL_FROM_HERE, "SELECT COUNT(*) FROM bounces"));
Expand All @@ -975,12 +969,13 @@ size_t DIPSDatabase::GarbageCollect() {
if (!CheckDBInit())
return 0;

size_t num_deleted = ClearRowsWithExpiredInteractions();
;
size_t num_entries = GetEntryCount();
size_t num_deleted = 0;
int purge_goal = num_entries - (max_entries_ - purge_entries_);

if (num_entries <= max_entries_)
return 0;
return num_deleted;

DCHECK_GT(purge_goal, 0);

Expand Down
55 changes: 37 additions & 18 deletions chrome/browser/dips/dips_database.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,27 +53,46 @@ class DIPSDatabase {
const TimestampRange& stateful_bounce_times,
const TimestampRange& bounce_times);

absl::optional<StateValue> Read(const std::string& site);
absl::optional<StateValue> Read(const std::string& site) const;

// Note: this doesn't clear expired interactions from the database unlike the
// other database querying methods.
std::vector<std::string> GetAllSitesForTesting();
std::vector<std::string> GetAllSitesForTesting() const;

// Returns all sites that did a bounce after |range_start| with their last
// interaction happening before |last_interaction|.
std::vector<std::string> GetSitesThatBounced(base::Time range_start,
base::Time last_interaction);
// Returns all sites which bounced the user and aren't protected from DIPS.
//
// A site can be protected in several ways:
// - it's still in its grace period after the first bounce
// - it received user interaction before the first bounce
// - it received user interaction in the grace period after the first bounce
//
// If a site is protected by an interaction, it won't be returned by this
// query until that interaction expires after interaction_ttl.
std::vector<std::string> GetSitesThatBounced() const;

// Returns all sites that did a stateful bounce after |range_start| with their
// last interaction happening before |last_interaction|.
std::vector<std::string> GetSitesThatBouncedWithState(
base::Time range_start,
base::Time last_interaction);
// Returns all sites which used storage and aren't protected from DIPS.
//
// A site can be protected in several ways:
// - it's still in its grace period after the first storage
// - it received user interaction before the first storage
// - it received user interaction in the grace period after the first storage
//
// If a site is protected by an interaction, it won't be returned by this
// query until that interaction expires after interaction_ttl.
std::vector<std::string> GetSitesThatUsedStorage() const;

// Returns all sites that wrote to storage after |range_start| with their last
// interaction happening before |last_interaction|.
std::vector<std::string> GetSitesThatUsedStorage(base::Time range_start,
base::Time last_interaction);
// Returns all sites which statefully bounced the user and aren't protected
// from DIPS.
//
// A site can be protected in several ways:
// - it's still in its grace period after the first stateful bounce
// - it received user interaction before the first stateful bounce
// - it received user interaction in the grace period after the first stateful
// bounce
//
// If a site is protected by an interaction, it won't be returned by this
// query until that interaction expires after interaction_ttl.
std::vector<std::string> GetSitesThatBouncedWithState() const;

// Deletes all rows in the database whose interactions have expired out.
//
Expand Down Expand Up @@ -105,7 +124,7 @@ class DIPSDatabase {
const DIPSEventRemovalType type);

// Returns the number of entries present in the database.
size_t GetEntryCount();
size_t GetEntryCount() const;

// If the number of entries in the database is greater than
// |GetMaxEntries()|, garbage collect. Returns the number of entries deleted
Expand All @@ -123,7 +142,7 @@ class DIPSDatabase {
}

// Checks that the internal SQLite database is initialized.
bool CheckDBInit();
bool CheckDBInit() const;

size_t GetMaxEntries() const { return max_entries_; }
size_t GetPurgeEntries() const { return purge_entries_; }
Expand Down Expand Up @@ -155,7 +174,7 @@ class DIPSDatabase {
const DIPSEventRemovalType type);
bool RemoveEmptyRows();

void ComputeDatabaseMetrics();
void LogDatabaseMetrics() const;

private:
// Callback for database errors.
Expand Down

0 comments on commit a14c2c2

Please sign in to comment.