Skip to content

Commit

Permalink
[privacy_budget] Reset state on UKM client ID change.
Browse files Browse the repository at this point in the history 
The set of selected surfaces should be considered itself identifiable.
Hence should not be persisted across a UKM client ID change.

This change introduces the following:

  * A new method for MetricsProvider that a metrics service can use to
    signal that the metrics ID has changed.

  * A PrivacyBudgetMetricsProvider that updates the
    IdentifiabilityStudyState in response to client ID changes.

(cherry picked from commit e2dcd57)

Bug: 973801
Fixed: 1181084
Change-Id: Idaf436b46b29b0a5b349ae1be5970206a66a926f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2710529
Commit-Queue: Asanka Herath <asanka@chromium.org>
Reviewed-by: Caitlin Fischer <caitlinfischer@google.com>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#858279}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2738719
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Asanka Herath <asanka@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/4430@{#197}
Cr-Branched-From: e5ce7dc-refs/heads/master@{#857950}
  • Loading branch information
@asankah
asankah authored and Chromium LUCI CQ committed Mar 6, 2021
1 parent 06a2ced commit fd4f7b1
Show file tree
Hide file tree
Showing 18 changed files with 417 additions and 75 deletions.
5 changes: 5 additions & 0 deletions chrome/browser/metrics/chrome_metrics_service_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
#include "chrome/browser/metrics/metrics_reporting_state.h"
#include "chrome/browser/metrics/network_quality_estimator_provider_impl.h"
#include "chrome/browser/metrics/usertype_by_devicetype_metrics_provider.h"
#include "chrome/browser/privacy_budget/privacy_budget_metrics_provider.h"
#include "chrome/browser/privacy_budget/privacy_budget_prefs.h"
#include "chrome/browser/privacy_budget/privacy_budget_ukm_entry_filter.h"
#include "chrome/browser/profiles/profile_manager.h"
Expand Down Expand Up @@ -829,6 +830,10 @@ void ChromeMetricsServiceClient::RegisterUKMProviders() {
std::make_unique<metrics::ScreenInfoMetricsProvider>());

ukm_service_->RegisterMetricsProvider(ukm::CreateFieldTrialsProviderForUkm());

ukm_service_->RegisterMetricsProvider(
std::make_unique<PrivacyBudgetMetricsProvider>(
identifiability_study_state_.get()));
}

void ChromeMetricsServiceClient::CollectFinalHistograms() {
Expand Down
15 changes: 8 additions & 7 deletions chrome/browser/metrics/chrome_metrics_service_client_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,15 +133,16 @@ TEST_F(ChromeMetricsServiceClientTest, FilterFiles) {
} // namespace

TEST_F(ChromeMetricsServiceClientTest, TestRegisterUKMProviders) {
// Test that UKM service has initialized its metrics providers.
// Currently there are 5 providers for all platform except ChromeOS.
// Test that UKM service has initialized its metrics providers. Currently
// there are 6 providers for all platform except ChromeOS.
// NetworkMetricsProvider, GPUMetricsProvider, CPUMetricsProvider
// and ScreenInfoMetricsProvider.
size_t expected_providers = 5;

// ScreenInfoMetricsProvider, FieldTrialsProvider, and
// PrivacyBudgetMetricsProvider.
#if BUILDFLAG(IS_CHROMEOS_ASH)
expected_providers++; // ChromeOSMetricsProvider
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
const size_t expected_providers = 7; // ChromeOSMetricsProvider
#else
const size_t expected_providers = 6;
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

std::unique_ptr<ChromeMetricsServiceClient> chrome_metrics_service_client =
ChromeMetricsServiceClient::Create(metrics_state_manager_.get());
Expand Down
12 changes: 12 additions & 0 deletions chrome/browser/privacy_budget/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ source_set("headers") {
sources = [
"encountered_surface_tracker.h",
"identifiability_study_state.h",
"privacy_budget_metrics_provider.h",
"privacy_budget_prefs.h",
"privacy_budget_ukm_entry_filter.h",
]

deps = [
"//base",
"//chrome/common/privacy_budget",
"//components/metrics",
"//components/prefs",
"//components/ukm",
"//third_party/blink/public/common/privacy_budget",
Expand All @@ -23,6 +25,7 @@ source_set("privacy_budget") {
sources = [
"encountered_surface_tracker.cc",
"identifiability_study_state.cc",
"privacy_budget_metrics_provider.cc",
"privacy_budget_prefs.cc",
"privacy_budget_ukm_entry_filter.cc",
]
Expand All @@ -47,6 +50,8 @@ source_set("unit_tests") {
sources = [
"encountered_surface_tracker_unittest.cc",
"identifiability_study_state_unittest.cc",
"inspectable_identifiability_study_state.h",
"privacy_budget_metrics_provider_unittest.cc",
"privacy_budget_ukm_entry_filter_unittest.cc",
]

Expand All @@ -67,11 +72,18 @@ source_set("browser_tests") {
defines = [ "HAS_OUT_OF_PROC_TEST_RUNNER" ]

deps = [
":headers",
"//chrome/browser",
"//chrome/browser:browser_process",
"//chrome/browser/metrics:test_support",
"//chrome/common/privacy_budget:test_support",
"//chrome/test:sync_integration_test_support",
"//chrome/test:test_support",
"//components/metrics_services_manager",
"//components/sync/test/fake_server",
"//components/ukm:test_support",
"//components/ukm:ukm_test_helper",
"//components/unified_consent",
"//components/variations/service:buildflags",
"//third_party/blink/public/common/privacy_budget",
]
Expand Down
51 changes: 33 additions & 18 deletions chrome/browser/privacy_budget/identifiability_study_state.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,39 @@ void IdentifiabilityStudyState::CheckInvariants() const {
void IdentifiabilityStudyState::CheckInvariants() const {}
#endif // DCHECK_IS_ON()

void IdentifiabilityStudyState::ResetPerReportState() {
surface_encounters_.Reset();
}

void IdentifiabilityStudyState::ResetEphemeralState() {
ResetPerReportState();
recent_surfaces_.Clear();
}

void IdentifiabilityStudyState::ResetClientState() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
ResetEphemeralState();
active_surfaces_.clear();
retired_surfaces_.clear();
pref_service_->ClearPref(prefs::kPrivacyBudgetActiveSurfaces);
pref_service_->ClearPref(prefs::kPrivacyBudgetRetiredSurfaces);

if (LIKELY(!IsStudyActive())) {
prng_seed_ = 0;
pref_service_->ClearPref(prefs::kPrivacyBudgetSeed);
pref_service_->ClearPref(prefs::kPrivacyBudgetGeneration);
return;
}

CheckAndResetPrngSeed(0u);
pref_service_->SetInteger(prefs::kPrivacyBudgetGeneration, generation_);

// State should be ready-to-go after resetting, even if the study is disabled.
CheckInvariants();
}

void IdentifiabilityStudyState::InitFromPrefs() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
DCHECK(active_surfaces_.empty());
DCHECK(retired_surfaces_.empty());

// None of the parameters should be relied upon if the study is not enabled.
if (LIKELY(!IsStudyActive()))
Expand All @@ -207,17 +236,6 @@ void IdentifiabilityStudyState::InitFromPrefs() {
ReconcileLoadedPrefs();
}

void IdentifiabilityStudyState::ResetClientState() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
active_surfaces_.clear();
retired_surfaces_.clear();

pref_service_->ClearPref(prefs::kPrivacyBudgetActiveSurfaces);
pref_service_->ClearPref(prefs::kPrivacyBudgetRetiredSurfaces);
CheckAndResetPrngSeed(0u);
pref_service_->SetInteger(prefs::kPrivacyBudgetGeneration, generation_);
}

void IdentifiabilityStudyState::WriteToPrefs() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
CheckInvariants();
Expand Down Expand Up @@ -331,9 +349,6 @@ bool IdentifiabilityStudyState::ShouldReportEncounteredSurface(
blink::IdentifiableSurface::Type::kMeasuredSurface)) {
return false;
}
return tracked_surfaces_.IsNewEncounter(source_id, surface.ToUkmMetricHash());
}

void IdentifiabilityStudyState::ResetRecordedSurfaces() {
tracked_surfaces_.Reset();
return surface_encounters_.IsNewEncounter(source_id,
surface.ToUkmMetricHash());
}
44 changes: 27 additions & 17 deletions chrome/browser/privacy_budget/identifiability_study_state.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,17 +75,34 @@ class IdentifiabilityStudyState {
bool ShouldReportEncounteredSurface(uint64_t source_id,
blink::IdentifiableSurface surface);

// Clear the sampled surface state from the state tracker. Ideally this would
// be called each time a UKM report generated.
void ResetRecordedSurfaces();
// Resets the state associated with a single report.
//
// It should be called each time the UKM service constructs a UKM client
// report.
void ResetPerReportState();

// Resets state that is not persisted across sessions. Currently these are
// things like MRU caches. Invoked prior to reading in or resetting persisted
// state.
//
// It should be called each time persisted state is refreshed.
void ResetEphemeralState();

// Clears all persisted and ephemeral state.
//
// It should be called when the UKM client ID changes or if the experiment
// generation changes.
void ResetClientState();

// Initializes from fields persisted in `pref_service_`.
void InitFromPrefs();

// A knob that we can use to split data sets from different versions of the
// implementation where the differences could have material effects on the
// data distribution.
//
// Should be incremented whenever a non-backwards-compatible change is made in
// the code. This value is independent of any server controlled study
// parameters.
// Increment this whenever a non-backwards-compatible change is made in the
// code. This value is independent of any server controlled study parameters.
static constexpr int kGeneratorVersion = 1;

private:
Expand All @@ -111,10 +128,7 @@ class IdentifiabilityStudyState {
// expensive. Noop otherwise.
void CheckInvariants() const;

// Initialize from fields persisted in `pref_service_`.
void InitFromPrefs();

// Write active and retired lists to `pref_service_`.
// Writes persisted state to `pref_service_`.
void WriteToPrefs();

// Checks whether `previous_value` is a valid PRNG seed. If not generates
Expand All @@ -130,10 +144,6 @@ class IdentifiabilityStudyState {
// allowed surfaces.
void ReconcileLoadedPrefs();

// Clears all client-side persisted state. This should only be invoked when
// the experiment generation changes, or if the UKM client ID has been reset.
void ResetClientState();

// Contains all the logic for determining whether a newly observed surface
// should be added to the active list or not.
bool DecideSurfaceInclusion(blink::IdentifiableSurface surface);
Expand Down Expand Up @@ -230,11 +240,11 @@ class IdentifiabilityStudyState {
//
// Invariants:
//
// * tracked_surfaces_ ∩ settings_.blocked_surfaces() = Ø.
// * surface_encounters_ ∩ settings_.blocked_surfaces() = Ø.
//
// * tracked_surfaces_.GetType() ∩ settings_.blocked_types() = Ø.
// * surface_encounters_.GetType() ∩ settings_.blocked_types() = Ø.
//
EncounteredSurfaceTracker tracked_surfaces_;
EncounteredSurfaceTracker surface_encounters_;

SEQUENCE_CHECKER(sequence_checker_);
};
Expand Down
43 changes: 18 additions & 25 deletions chrome/browser/privacy_budget/identifiability_study_state_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "chrome/browser/privacy_budget/inspectable_identifiability_study_state.h"
#include "chrome/browser/privacy_budget/privacy_budget_prefs.h"
#include "chrome/common/privacy_budget/privacy_budget_features.h"
#include "chrome/common/privacy_budget/scoped_privacy_budget_config.h"
Expand All @@ -20,31 +21,6 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/privacy_budget/identifiable_surface.h"

namespace test_utils {

// This class is a friend of IdentifiabilityStudyState and can reach into the
// internals. Use this as a last resort.
class InspectableIdentifiabilityStudyState : public IdentifiabilityStudyState {
public:
using IdentifiabilityStudyState::IdentifiableSurfaceSet;
using IdentifiabilityStudyState::IdentifiableSurfaceTypeSet;

explicit InspectableIdentifiabilityStudyState(PrefService* pref_service)
: IdentifiabilityStudyState(pref_service) {}

const IdentifiableSurfaceSet& active_surfaces() const {
return active_surfaces_;
}
const IdentifiableSurfaceSet& retired_surfaces() const {
return retired_surfaces_;
}
int max_active_surfaces() const { return max_active_surfaces_; }
int surface_selection_rate() const { return surface_selection_rate_; }
uint64_t prng_seed() const { return prng_seed_; }
};

} // namespace test_utils

namespace {

// Constants used to set up the test configuration.
Expand Down Expand Up @@ -305,3 +281,20 @@ TEST(IdentifiabilityStudyStateStandaloneTest, Disabled) {
EXPECT_FALSE(settings.ShouldRecordSurface(kRegularSurface2));
EXPECT_FALSE(settings.ShouldRecordSurface(kRegularSurface3));
}

TEST(IdentifiabilityStudyStateStandaloneTest, DisabledStudyDoesNotNukePrefs) {
auto params = test::ScopedPrivacyBudgetConfig::Parameters{};
params.enabled = false;
params.surface_selection_rate = 1;
test::ScopedPrivacyBudgetConfig config(params);

const std::string kSurfaces = "1,2,3";

TestingPrefServiceSimple pref_service;
prefs::RegisterPrivacyBudgetPrefs(pref_service.registry());
pref_service.SetString(prefs::kPrivacyBudgetActiveSurfaces, kSurfaces);
test_utils::InspectableIdentifiabilityStudyState settings(&pref_service);

EXPECT_EQ(kSurfaces,
pref_service.GetString(prefs::kPrivacyBudgetActiveSurfaces));
}
35 changes: 35 additions & 0 deletions chrome/browser/privacy_budget/inspectable_identifiability_study_state.h
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright 2021 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.

#ifndef CHROME_BROWSER_PRIVACY_BUDGET_INSPECTABLE_IDENTIFIABILITY_STUDY_STATE_H_
#define CHROME_BROWSER_PRIVACY_BUDGET_INSPECTABLE_IDENTIFIABILITY_STUDY_STATE_H_

#include "chrome/browser/privacy_budget/identifiability_study_state.h"

namespace test_utils {

// This class is a friend of IdentifiabilityStudyState and can reach into the
// internals. Use this as a last resort.
class InspectableIdentifiabilityStudyState : public IdentifiabilityStudyState {
public:
using IdentifiabilityStudyState::IdentifiableSurfaceSet;
using IdentifiabilityStudyState::IdentifiableSurfaceTypeSet;

explicit InspectableIdentifiabilityStudyState(PrefService* pref_service)
: IdentifiabilityStudyState(pref_service) {}

const IdentifiableSurfaceSet& active_surfaces() const {
return active_surfaces_;
}
const IdentifiableSurfaceSet& retired_surfaces() const {
return retired_surfaces_;
}
int max_active_surfaces() const { return max_active_surfaces_; }
int surface_selection_rate() const { return surface_selection_rate_; }
uint64_t prng_seed() const { return prng_seed_; }
};

} // namespace test_utils

#endif // CHROME_BROWSER_PRIVACY_BUDGET_INSPECTABLE_IDENTIFIABILITY_STUDY_STATE_H_

0 comments on commit fd4f7b1

Please sign in to comment.