Skip to content

Commit

Permalink
[Topics] Proactively expire epochs older than 4 weeks
Browse files Browse the repository at this point in the history
What:
Currently, the browser stores 4 latest epochs, which cover 4 weeks
of data for active users. However, inactive users may have epochs that
are much older than 4 weeks. This contradicts our stated policy (from
Topics UX) that "Topics older than 4 weeks get auto-deleted". We need
to address this discrepancy to ensure we're fulfilling our promise.

How:
- When a new epoch is added, a task is scheduled to automatically
  delete it after 28 days.
- Upon browser restart, deletion tasks are scheduled for all
  existing epochs.

Bug: 366293462
Change-Id: I4d507f0a17b1f8cc978f2117dc968605bfc54de5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5854642
Reviewed-by: Philip Rogers <[email protected]>
Reviewed-by: Abigail Katcoff <[email protected]>
Commit-Queue: Yao Xiao <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1355400}
  • Loading branch information
yaoxiachromium authored and Chromium LUCI CQ committed Sep 13, 2024
1 parent 2d57f4f commit dbb6774
Show file tree
Hide file tree
Showing 11 changed files with 286 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -383,11 +383,15 @@ class BrowsingTopicsBrowserTest : public BrowsingTopicsBrowserTestBase {
: prerender_helper_(
base::BindRepeating(&BrowsingTopicsBrowserTest::web_contents,
base::Unretained(this))) {
scoped_feature_list_.InitWithFeatures(
// Configure a long epoch_retention_duration to prevent epochs from expiring
// during tests where expiration is irrelevant.
scoped_feature_list_.InitWithFeaturesAndParameters(
/*enabled_features=*/
{blink::features::kBrowsingTopics,
blink::features::kBrowsingTopicsBypassIPIsPubliclyRoutableCheck,
features::kPrivacySandboxAdsAPIsOverride},
{{blink::features::kBrowsingTopics, {}},
{blink::features::kBrowsingTopicsParameters,
{{"epoch_retention_duration", "3650000d"}}},
{blink::features::kBrowsingTopicsBypassIPIsPubliclyRoutableCheck, {}},
{features::kPrivacySandboxAdsAPIsOverride, {}}},
/*disabled_features=*/{});
}

Expand Down Expand Up @@ -2445,15 +2449,6 @@ IN_PROC_BROWSER_TEST_F(BrowsingTopicsBrowserTest,
// Tests that the Topics API abides by the Privacy Sandbox Enrollment framework.
class AttestationBrowsingTopicsBrowserTest : public BrowsingTopicsBrowserTest {
public:
AttestationBrowsingTopicsBrowserTest() {
scoped_feature_list_.InitWithFeatures(
/*enabled_features=*/
{blink::features::kBrowsingTopics,
blink::features::kBrowsingTopicsBypassIPIsPubliclyRoutableCheck,
features::kPrivacySandboxAdsAPIsOverride},
/*disabled_features=*/{});
}

void SetUpOnMainThread() override {
// This test suite tests Privacy Sandbox Attestations related behaviors,
// turn off the setting that makes all APIs considered attested.
Expand All @@ -2463,9 +2458,6 @@ class AttestationBrowsingTopicsBrowserTest : public BrowsingTopicsBrowserTest {
}

~AttestationBrowsingTopicsBrowserTest() override = default;

protected:
base::test::ScopedFeatureList scoped_feature_list_;
};

// Site a.test is attested for Topics, so it should receive a valid response.
Expand Down
2 changes: 2 additions & 0 deletions components/browsing_topics/browsing_topics_service_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,8 @@ void BrowsingTopicsServiceImpl::OnBrowsingTopicsStateLoaded() {

site_data_manager_->ExpireDataBefore(browsing_topics_data_sccessible_since);

browsing_topics_state_.ScheduleEpochsExpiration();

ScheduleBrowsingTopicsCalculation(
/*is_manually_triggered=*/false,
/*previous_timeout_count=*/0, decision.next_calculation_delay,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ namespace {
constexpr base::TimeDelta kOneTestDay = base::Seconds(1);
constexpr base::TimeDelta kEpoch = 7 * kOneTestDay;
constexpr base::TimeDelta kMaxEpochIntroductionDelay = 2 * kOneTestDay;

constexpr base::TimeDelta kDatabaseFetchDelay = base::Milliseconds(1);
constexpr base::TimeDelta kCalculatorDelay = base::Milliseconds(1);
constexpr base::TimeDelta kFirstTimeoutRetryDelay = base::Milliseconds(10);
Expand Down Expand Up @@ -180,6 +179,8 @@ class BrowsingTopicsServiceImplTest
BrowsingTopicsServiceImplTest()
: content::RenderViewHostTestHarness(
base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
// Configure a long epoch_retention_duration to prevent epochs from expiring
// during tests where expiration is irrelevant.
scoped_feature_list_.InitWithFeaturesAndParameters(
/*enabled_features=*/
{{blink::features::kBrowsingTopics, {}},
Expand All @@ -190,6 +191,7 @@ class BrowsingTopicsServiceImplTest
base::StrCat(
{base::NumberToString(kFirstTimeoutRetryDelay.InMilliseconds()),
"ms"})},
{"epoch_retention_duration", "3650000d"},
{"max_epoch_introduction_delay",
base::StrCat(
{base::NumberToString(kMaxEpochIntroductionDelay.InSeconds()),
Expand Down Expand Up @@ -455,6 +457,57 @@ TEST_F(BrowsingTopicsServiceImplTest, WallTimeScheduling) {
EXPECT_EQ(browsing_topics_service_->started_calculations_count(), 2u);
}

TEST_F(BrowsingTopicsServiceImplTest,
StartFromPreexistingState_ScheduleEpochsExpiration) {
scoped_feature_list_.Reset();
scoped_feature_list_.InitWithFeaturesAndParameters(
/*enabled_features=*/
{{blink::features::kBrowsingTopics, {}},
{blink::features::kBrowsingTopicsParameters,
{{"epoch_retention_duration",
base::StrCat(
{base::NumberToString(28 * kOneTestDay.InSeconds()), "s"})}}}},
/*disabled_features=*/{});

base::Time start_time = base::Time::Now();

std::vector<EpochTopics> preexisting_epochs;
preexisting_epochs.push_back(
CreateTestEpochTopics({{Topic(1), {}},
{Topic(2), {}},
{Topic(3), {}},
{Topic(4), {}},
{Topic(5), {}}},
start_time - 29 * kOneTestDay));

preexisting_epochs.push_back(
CreateTestEpochTopics({{Topic(1), {}},
{Topic(2), {}},
{Topic(3), {}},
{Topic(4), {}},
{Topic(5), {}}},
start_time - 27 * kOneTestDay));

CreateBrowsingTopicsStateFile(
std::move(preexisting_epochs),
/*next_scheduled_calculation_time=*/start_time + 2 * kOneTestDay);

InitializeBrowsingTopicsService(/*mock_calculator_results=*/{});

// Finish file loading.
task_environment()->RunUntilIdle();

// Verify that the first epoch (29 days old) has expired, leaving only one
// epoch.
EXPECT_EQ(browsing_topics_state().epochs().size(), 1u);
EXPECT_EQ(browsing_topics_state().epochs()[0].calculation_time(),
start_time - 27 * kOneTestDay);

// Fast-forward time by one day and verify the second epoch also expires.
task_environment()->FastForwardBy(kOneTestDay);
EXPECT_EQ(browsing_topics_state().epochs().size(), 0u);
}

TEST_F(BrowsingTopicsServiceImplTest,
StartFromPreexistingState_CalculateAtScheduledTime) {
base::Time start_time = base::Time::Now();
Expand Down Expand Up @@ -1461,6 +1514,7 @@ TEST_F(BrowsingTopicsServiceImplTest,
base::StrCat(
{base::NumberToString(kMaxEpochIntroductionDelay.InSeconds()),
"s"})},
{"epoch_retention_duration", "3650000d"},
{"prioritized_topics_list", "1,57"}}}},
/*disabled_features=*/{});

Expand Down Expand Up @@ -2692,6 +2746,7 @@ TEST_F(BrowsingTopicsServiceImplTest, BlockTopicWithFinch) {
base::StrCat(
{base::NumberToString(kMaxEpochIntroductionDelay.InSeconds()),
"s"})},
{"epoch_retention_duration", "3650000d"},
{"disabled_topics_list", "20,10,7"}}}},
/*disabled_features=*/{});
InitializeBrowsingTopicsService(std::move(mock_calculator_results));
Expand Down
33 changes: 33 additions & 0 deletions components/browsing_topics/browsing_topics_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,9 @@ std::optional<EpochTopics> BrowsingTopicsState::AddEpoch(
DCHECK(loaded_);

epochs_.push_back(std::move(epoch_topics));
epochs_.back().ScheduleExpiration(base::BindOnce(
&BrowsingTopicsState::OnEpochExpired, weak_ptr_factory_.GetWeakPtr(),
epochs_.back().calculation_time()));

// Remove the epoch data that is no longer useful.
std::optional<EpochTopics> removed_epoch_topics;
Expand All @@ -163,6 +166,36 @@ std::optional<EpochTopics> BrowsingTopicsState::AddEpoch(
return removed_epoch_topics;
}

void BrowsingTopicsState::ScheduleEpochsExpiration() {
base::Time expired_calculation_time =
base::Time::Now() -
blink::features::kBrowsingTopicsEpochRetentionDuration.Get();

// Remove expired epochs synchronously.
base::EraseIf(epochs_, [&expired_calculation_time](const EpochTopics& epoch) {
return epoch.calculation_time() <= expired_calculation_time;
});

for (EpochTopics& epoch : epochs_) {
epoch.ScheduleExpiration(base::BindOnce(
&BrowsingTopicsState::OnEpochExpired, weak_ptr_factory_.GetWeakPtr(),
epoch.calculation_time()));
}

ScheduleSave();
}

void BrowsingTopicsState::OnEpochExpired(base::Time calculation_time) {
// Remove all epochs associated with the given calculation_time.
// Though calculation times are typically unique, this handles potential
// duplicates.
base::EraseIf(epochs_, [&calculation_time](const EpochTopics& epoch) {
return epoch.calculation_time() == calculation_time;
});

ScheduleSave();
}

void BrowsingTopicsState::UpdateNextScheduledCalculationTime(
base::TimeDelta delay) {
DCHECK(loaded_);
Expand Down
7 changes: 7 additions & 0 deletions components/browsing_topics/browsing_topics_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ class BrowsingTopicsState
// epoch calculation. If an old EpochTopics is removed as a result, return it.
std::optional<EpochTopics> AddEpoch(EpochTopics epoch_topics);

// Remove expired epochs synchronously. For unexpired epochs, let each one
// schedule its own expiration task. Upon expiration of each epoch,
// `OnEpochExpired` will be called to remove it from `epochs_`.
void ScheduleEpochsExpiration();

// Calculates the new scheduled time by adding the provided `delay`
// to the current time (`base::Time::Now()`), and stores the result to
// `next_scheduled_calculation_time_`.
Expand Down Expand Up @@ -132,6 +137,8 @@ class BrowsingTopicsState
void DidLoadFile(base::OnceClosure loaded_callback,
std::unique_ptr<LoadResult> load_result);

void OnEpochExpired(base::Time calculation_time);

// Parse `value` and populate the state member variables.
ParseResult ParseValue(const base::Value& value);

Expand Down
106 changes: 106 additions & 0 deletions components/browsing_topics/browsing_topics_state_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,15 @@ class BrowsingTopicsStateTest : public testing::Test {
BrowsingTopicsStateTest()
: task_environment_(new base::test::TaskEnvironment(
base::test::TaskEnvironment::TimeSource::MOCK_TIME)) {
// Configure a long epoch_retention_duration to prevent epochs from expiring
// during tests where expiration is irrelevant.
feature_list_.InitWithFeaturesAndParameters(
/*enabled_features=*/
{{blink::features::kBrowsingTopics, {}},
{blink::features::kBrowsingTopicsParameters,
{{"epoch_retention_duration", "3650000d"}}}},
/*disabled_features=*/{});

OverrideHmacKeyForTesting(kTestKey);

EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
Expand Down Expand Up @@ -831,4 +840,101 @@ TEST_F(BrowsingTopicsStateTest, ShouldSaveFileDespiteShutdownWhileScheduled) {
"\"next_scheduled_calculation_time\": \"0\"}");
}

TEST_F(BrowsingTopicsStateTest, ScheduleEpochsExpiration) {
feature_list_.Reset();
feature_list_.InitWithFeaturesAndParameters(
/*enabled_features=*/
{{blink::features::kBrowsingTopics, {}},
{blink::features::kBrowsingTopicsParameters,
{{"epoch_retention_duration", "28s"}}}},
/*disabled_features=*/{});

base::Time start_time = base::Time::Now();

std::vector<EpochTopics> epochs;
epochs.emplace_back(
CreateTestEpochTopics(start_time - base::Seconds(29),
/*from_manually_triggered_calculation=*/false));
epochs.emplace_back(
CreateTestEpochTopics(start_time - base::Seconds(28),
/*from_manually_triggered_calculation=*/false));
epochs.emplace_back(
CreateTestEpochTopics(start_time - base::Seconds(27),
/*from_manually_triggered_calculation=*/false));
epochs.emplace_back(
CreateTestEpochTopics(start_time - base::Seconds(26),
/*from_manually_triggered_calculation=*/false));

CreateOrOverrideTestFile(std::move(epochs),
/*next_scheduled_calculation_time=*/kTime2,
/*hex_encoded_hmac_key=*/base::HexEncode(kTestKey));

BrowsingTopicsState state(temp_dir_.GetPath(), base::DoNothing());
task_environment_->RunUntilIdle();

EXPECT_EQ(state.epochs().size(), 4u);

state.ScheduleEpochsExpiration();

// Verify that two epochs have been removed immediately due to expiration.
EXPECT_EQ(state.epochs().size(), 2u);
EXPECT_EQ(state.epochs()[0].calculation_time(),
start_time - base::Seconds(27));
EXPECT_EQ(state.epochs()[1].calculation_time(),
start_time - base::Seconds(26));

// Process any pending tasks to ensure any asynchronous expirations are
// handled.
task_environment_->RunUntilIdle();
EXPECT_EQ(state.epochs().size(), 2u);

// Trigger another epoch expiration.
task_environment_->FastForwardBy(base::Seconds(1));
EXPECT_EQ(state.epochs().size(), 1u);
EXPECT_EQ(state.epochs()[0].calculation_time(),
start_time - base::Seconds(26));

// Trigger the final epoch expiration.
task_environment_->FastForwardBy(base::Seconds(1));
EXPECT_EQ(state.epochs().size(), 0u);
}

TEST_F(BrowsingTopicsStateTest, AddEpochAndVerifyExpiration) {
feature_list_.Reset();
feature_list_.InitWithFeaturesAndParameters(
/*enabled_features=*/
{{blink::features::kBrowsingTopics, {}},
{blink::features::kBrowsingTopicsParameters,
{{"epoch_retention_duration", "28s"}}}},
/*disabled_features=*/{});

base::Time start_time = base::Time::Now();

BrowsingTopicsState state(temp_dir_.GetPath(), base::DoNothing());
task_environment_->RunUntilIdle();

state.AddEpoch(CreateTestEpochTopics(
base::Time::Now(), /*from_manually_triggered_calculation=*/false));

task_environment_->FastForwardBy(base::Seconds(1));
state.AddEpoch(CreateTestEpochTopics(
base::Time::Now(), /*from_manually_triggered_calculation=*/false));

EXPECT_EQ(state.epochs().size(), 2u);

// Verify epochs haven't expired prematurely.
task_environment_->FastForwardBy(base::Seconds(26));
EXPECT_EQ(state.epochs().size(), 2u);

// Verify the first epoch expired at the expected expiration time.
task_environment_->FastForwardBy(base::Seconds(1));
EXPECT_EQ(state.epochs().size(), 1u);
EXPECT_EQ(state.epochs()[0].calculation_time(),
start_time + base::Seconds(1));

// Verify the second epoch has also expired at the expected expiration time.
task_environment_->FastForwardBy(base::Seconds(1));
EXPECT_EQ(state.epochs().size(), 0u);
}

} // namespace browsing_topics
11 changes: 11 additions & 0 deletions components/browsing_topics/epoch_topics.cc
Original file line number Diff line number Diff line change
Expand Up @@ -247,4 +247,15 @@ void EpochTopics::ClearContextDomain(
}
}

void EpochTopics::ScheduleExpiration(base::OnceClosure on_expiration_callback) {
CHECK(!expiration_timer_);
expiration_timer_ = std::make_unique<base::WallClockTimer>();

expiration_timer_->Start(
FROM_HERE,
calculation_time_ +
blink::features::kBrowsingTopicsEpochRetentionDuration.Get(),
std::move(on_expiration_callback));
}

} // namespace browsing_topics
Loading

0 comments on commit dbb6774

Please sign in to comment.