Skip to content

Commit

Permalink
[DIPS] Make all DIPSStorage accesses asynchronous
Browse files Browse the repository at this point in the history 
in preparation for migration to SQLite db for data storage.

Bug: 1342228
Change-Id: Idbeef082a3cd7765ecab8f6ca3cd1b9f554c698c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3819863
Commit-Queue: Joshua Hood <jdh@chromium.org>
Reviewed-by: Matt Reichhoff <mreichhoff@chromium.org>
Auto-Submit: Joshua Hood <jdh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033998}
  • Loading branch information
@hoodjoshua
hoodjoshua authored and Chromium LUCI CQ committed Aug 11, 2022
1 parent ab5a8ca commit b13a076
Show file tree
Hide file tree
Showing 10 changed files with 186 additions and 78 deletions.
88 changes: 28 additions & 60 deletions chrome/browser/dips/dips_helper.cc
View file
Expand Up @@ -6,38 +6,16 @@

#include <utility>

#include "base/metrics/histogram_functions.h"
#include "base/strings/strcat.h"
#include "base/time/default_clock.h"
#include "chrome/browser/dips/dips_service.h"
#include "chrome/browser/dips/dips_storage.h"
#include "chrome/browser/dips/dips_utils.h"
#include "content/public/browser/browser_context.h"
#include "content/public/browser/cookie_access_details.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/render_frame_host.h"

namespace {

inline void UmaHistogramTimeToInteraction(base::TimeDelta sample,
DIPSCookieMode mode) {
const std::string name = base::StrCat(
{"Privacy.DIPS.TimeFromStorageToInteraction", GetHistogramSuffix(mode)});

base::UmaHistogramCustomTimes(name, sample,
/*min=*/base::TimeDelta(),
/*max=*/base::Days(7), 100);
}

inline void UmaHistogramTimeToStorage(base::TimeDelta sample,
DIPSCookieMode mode) {
const std::string name = base::StrCat(
{"Privacy.DIPS.TimeFromInteractionToStorage", GetHistogramSuffix(mode)});

base::UmaHistogramCustomTimes(name, sample,
/*min=*/base::TimeDelta(),
/*max=*/base::Days(7), 100);
}

// The Clock that a new DIPSTabHelper will use internally. Exposed as a global
// so that browser tests (which don't call the DIPSTabHelper constructor
// directly) can inject a fake clock.
Expand All @@ -60,46 +38,56 @@ DIPSCookieMode DIPSTabHelper::GetCookieMode() const {
service_->ShouldBlockThirdPartyCookies());
}

DIPSState DIPSTabHelper::StateForURL(const GURL& url) {
return service_->storage()->Read(url);
void DIPSTabHelper::FlushForTesting(base::OnceClosure flushed) {
service_->storage()
->AsyncCall(&DIPSStorage::DoNothing)
.Then(std::move(flushed));
}

void DIPSTabHelper::StateForURLForTesting(const GURL& url,
StateForURLCallback callback) {
service_->storage()
->AsyncCall(&DIPSStorage::Read)
.WithArgs(url)
.Then(std::move(callback));
}

/* static */
base::Clock* DIPSTabHelper::SetClockForTesting(base::Clock* clock) {
return std::exchange(g_clock, clock);
}

void DIPSTabHelper::MaybeRecordStorage(const GURL& url) {
DIPSState state = StateForURL(url);
if (state.site_storage_time()) {
// We want the time that storage was first written, so don't overwrite the
// existing timestamp.
return;
}
void DIPSTabHelper::RecordStorage(const GURL& url) {
base::Time now = clock_->Now();
DIPSCookieMode mode = GetCookieMode();

service_->storage()
->AsyncCall(&DIPSStorage::RecordStorage)
.WithArgs(url, now, mode);
}

void DIPSTabHelper::RecordInteraction(const GURL& url) {
base::Time now = clock_->Now();
if (state.user_interaction_time()) {
// First storage, but previous interaction.
UmaHistogramTimeToStorage(now - state.user_interaction_time().value(),
GetCookieMode());
}
DIPSCookieMode mode = GetCookieMode();

state.set_site_storage_time(now);
service_->storage()
->AsyncCall(&DIPSStorage::RecordInteraction)
.WithArgs(url, now, mode);
}

void DIPSTabHelper::OnCookiesAccessed(
content::RenderFrameHost* render_frame_host,
const content::CookieAccessDetails& details) {
if (details.type == content::CookieAccessDetails::Type::kChange) {
MaybeRecordStorage(details.url);
RecordStorage(details.url);
}
}

void DIPSTabHelper::OnCookiesAccessed(
content::NavigationHandle* handle,
const content::CookieAccessDetails& details) {
if (details.type == content::CookieAccessDetails::Type::kChange) {
MaybeRecordStorage(details.url);
RecordStorage(details.url);
}
}

Expand All @@ -113,24 +101,4 @@ void DIPSTabHelper::FrameReceivedUserActivation(
RecordInteraction(url);
}

void DIPSTabHelper::RecordInteraction(const GURL& url) {
DIPSState state = StateForURL(url);

base::Time now = clock_->Now();
if (!state.user_interaction_time()) {
// First interaction on site.
if (state.site_storage_time()) {
// Site previously wrote to storage. Record metric for the time delay
// between storage and interaction.
UmaHistogramTimeToInteraction(now - state.site_storage_time().value(),
GetCookieMode());
}
}

// Unlike for storage, we want to know the time of the most recent user
// interaction, so overwrite any existing timestamp. (If interaction happened
// a long time ago, it may no longer be relevant.)
state.set_user_interaction_time(now);
}

WEB_CONTENTS_USER_DATA_KEY_IMPL(DIPSTabHelper);
6 changes: 4 additions & 2 deletions chrome/browser/dips/dips_helper.h
View file
Expand Up @@ -26,12 +26,14 @@ class DIPSTabHelper : public content::WebContentsObserver,

// Record that |url| wrote to storage, if it was the first such time (we
// currently don't care about later writes to storage.)
void MaybeRecordStorage(const GURL& url);
void RecordStorage(const GURL& url);
// Record that the user interacted on |url| .
void RecordInteraction(const GURL& url);

DIPSState StateForURL(const GURL& url);
void FlushForTesting(base::OnceClosure flushed);

using StateForURLCallback = base::OnceCallback<void(DIPSState)>;
void StateForURLForTesting(const GURL& url, StateForURLCallback callback);
static base::Clock* SetClockForTesting(base::Clock* clock);

private:
Expand Down
41 changes: 33 additions & 8 deletions chrome/browser/dips/dips_helper_browsertest.cc
View file
Expand Up @@ -5,6 +5,7 @@
#include "chrome/browser/dips/dips_helper.h"

#include "base/memory/raw_ptr.h"
#include "base/test/bind.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/simple_test_clock.h"
#include "base/time/time.h"
Expand Down Expand Up @@ -102,8 +103,32 @@ class DIPSTabHelperBrowserTest : public InProcessBrowserTest {

DIPSTabHelper* dips_helper() { return helper_; }

void BlockUntilHelperProcessesPendingRequests() {
base::RunLoop run_loop;
helper_->FlushForTesting(run_loop.QuitClosure());
run_loop.Run();
}

void SetDIPSTime(base::Time time) { test_clock_.SetNow(time); }

void CopyDIPSState(DIPSState* a, DIPSState* b) {
a->set_site_storage_time_on_load(b->site_storage_time());
a->set_user_interaction_time_on_load(b->user_interaction_time());
a->set_was_loaded_for_testing(b->was_loaded());
}

DIPSState GetDIPSState(const GURL& url) {
DIPSState state;

helper_->StateForURLForTesting(
url, base::BindLambdaForTesting([&](DIPSState loaded_state) {
CopyDIPSState(&state, &loaded_state);
}));
BlockUntilHelperProcessesPendingRequests();

return state;
}

private:
base::SimpleTestClock test_clock_;
raw_ptr<DIPSTabHelper> helper_ = nullptr;
Expand All @@ -128,8 +153,8 @@ IN_PROC_BROWSER_TEST_F(DIPSTabHelperBrowserTest,
content::WaitForHitTestData(iframe);

// Before clicking, no DIPS state for either site.
EXPECT_FALSE(dips_helper()->StateForURL(url_a).was_loaded());
EXPECT_FALSE(dips_helper()->StateForURL(url_b).was_loaded());
EXPECT_FALSE(GetDIPSState(url_a).was_loaded());
EXPECT_FALSE(GetDIPSState(url_b).was_loaded());

// Click on the b.test iframe.
SetDIPSTime(time);
Expand All @@ -138,12 +163,12 @@ IN_PROC_BROWSER_TEST_F(DIPSTabHelperBrowserTest,
observer.Wait();

// User interaction is recorded for a.test (the top-level frame).
DIPSState state_a = dips_helper()->StateForURL(url_a);
DIPSState state_a = GetDIPSState(url_a);
EXPECT_TRUE(state_a.was_loaded());
EXPECT_FALSE(state_a.site_storage_time().has_value());
EXPECT_EQ(time, state_a.user_interaction_time().value());
// User interaction is also recorded for b.test (the iframe).
DIPSState state_b = dips_helper()->StateForURL(url_b);
DIPSState state_b = GetDIPSState(url_b);
EXPECT_TRUE(state_b.was_loaded());
EXPECT_FALSE(state_b.site_storage_time().has_value());
EXPECT_EQ(time, state_b.user_interaction_time().value());
Expand Down Expand Up @@ -172,8 +197,8 @@ IN_PROC_BROWSER_TEST_F(DIPSTabHelperBrowserTest, StorageRecordedInSingleFrame) {
base::BindRepeating(&content::FrameIsChildOfMainFrame));

// Initially, no DIPS state for either site.
EXPECT_FALSE(dips_helper()->StateForURL(url_a).was_loaded());
EXPECT_FALSE(dips_helper()->StateForURL(url_b).was_loaded());
EXPECT_FALSE(GetDIPSState(url_a).was_loaded());
EXPECT_FALSE(GetDIPSState(url_b).was_loaded());

// Write a cookie in the b.test iframe.
SetDIPSTime(time);
Expand All @@ -184,10 +209,10 @@ IN_PROC_BROWSER_TEST_F(DIPSTabHelperBrowserTest, StorageRecordedInSingleFrame) {
observer.Wait();

// Nothing recorded for a.test (the top-level frame).
DIPSState state_a = dips_helper()->StateForURL(url_a);
DIPSState state_a = GetDIPSState(url_a);
EXPECT_FALSE(state_a.was_loaded());
// Site storage was recorded for b.test (the iframe).
DIPSState state_b = dips_helper()->StateForURL(url_b);
DIPSState state_b = GetDIPSState(url_b);
EXPECT_TRUE(state_b.was_loaded());
EXPECT_EQ(time, state_b.site_storage_time().value());
EXPECT_FALSE(state_b.user_interaction_time().has_value());
Expand Down
10 changes: 9 additions & 1 deletion chrome/browser/dips/dips_service.cc
View file
Expand Up @@ -4,6 +4,7 @@

#include "chrome/browser/dips/dips_service.h"

#include "base/task/thread_pool.h"
#include "chrome/browser/content_settings/cookie_settings_factory.h"
#include "chrome/browser/dips/dips_service_factory.h"
#include "chrome/browser/profiles/profile.h"
Expand All @@ -12,7 +13,8 @@
DIPSService::DIPSService(content::BrowserContext* context)
: browser_context_(context),
cookie_settings_(CookieSettingsFactory::GetForProfile(
Profile::FromBrowserContext(context))) {}
Profile::FromBrowserContext(context))),
storage_(base::SequenceBound<DIPSStorage>(CreateTaskRunner())) {}

DIPSService::~DIPSService() = default;

Expand All @@ -25,6 +27,12 @@ void DIPSService::Shutdown() {
cookie_settings_.reset();
}

scoped_refptr<base::SequencedTaskRunner> DIPSService::CreateTaskRunner() {
return base::ThreadPool::CreateSequencedTaskRunner(
{base::MayBlock(), base::TaskPriority::BEST_EFFORT,
base::ThreadPolicy::PREFER_BACKGROUND});
}

bool DIPSService::ShouldBlockThirdPartyCookies() const {
return cookie_settings_->ShouldBlockThirdPartyCookies();
}
8 changes: 6 additions & 2 deletions chrome/browser/dips/dips_service.h
View file
Expand Up @@ -7,6 +7,7 @@

#include "base/memory/raw_ptr.h"
#include "base/memory/scoped_refptr.h"
#include "base/threading/sequence_bound.h"
#include "chrome/browser/dips/dips_storage.h"
#include "components/keyed_service/core/keyed_service.h"

Expand All @@ -24,7 +25,8 @@ class DIPSService : public KeyedService {

static DIPSService* Get(content::BrowserContext* context);

DIPSStorage* storage() { return &storage_; }
base::SequenceBound<DIPSStorage>* storage() { return &storage_; }

bool ShouldBlockThirdPartyCookies() const;

private:
Expand All @@ -33,9 +35,11 @@ class DIPSService : public KeyedService {
explicit DIPSService(content::BrowserContext* context);
void Shutdown() override;

scoped_refptr<base::SequencedTaskRunner> CreateTaskRunner();

raw_ptr<content::BrowserContext> browser_context_;
scoped_refptr<content_settings::CookieSettings> cookie_settings_;
DIPSStorage storage_;
base::SequenceBound<DIPSStorage> storage_;
};

#endif // CHROME_BROWSER_DIPS_DIPS_SERVICE_H_
13 changes: 13 additions & 0 deletions chrome/browser/dips/dips_state.cc
View file
Expand Up @@ -28,6 +28,10 @@ void DIPSState::set_site_storage_time(absl::optional<base::Time> time) {
dirty_ = true;
}

void DIPSState::set_site_storage_time_on_load(absl::optional<base::Time> time) {
site_storage_time_ = time;
}

void DIPSState::set_user_interaction_time(absl::optional<base::Time> time) {
if (time == user_interaction_time_) {
return;
Expand All @@ -36,3 +40,12 @@ void DIPSState::set_user_interaction_time(absl::optional<base::Time> time) {
user_interaction_time_ = time;
dirty_ = true;
}

void DIPSState::set_user_interaction_time_on_load(
absl::optional<base::Time> time) {
user_interaction_time_ = time;
}

void DIPSState::set_was_loaded_for_testing(bool loaded) {
was_loaded_ = loaded;
}
7 changes: 7 additions & 0 deletions chrome/browser/dips/dips_state.h
View file
Expand Up @@ -34,6 +34,7 @@ class DirtyBit {
// DIPSState represents the state recorded by DIPSService itself.
class DIPSState {
public:
DIPSState() = default;
DIPSState(DIPSStorage* storage, std::string site, bool was_loaded);
DIPSState(DIPSState&&);
// Flushes changes to storage_.
Expand All @@ -43,16 +44,22 @@ class DIPSState {
// True iff this DIPSState was loaded from DIPSStorage (as opposed to being
// default-initialized for a new site).
bool was_loaded() const { return was_loaded_; }
// For testing only.
void set_was_loaded_for_testing(bool loaded);

absl::optional<base::Time> site_storage_time() const {
return site_storage_time_;
}
void set_site_storage_time(absl::optional<base::Time> time);
// For loading/copying DIPSState objects only.
void set_site_storage_time_on_load(absl::optional<base::Time> time);

absl::optional<base::Time> user_interaction_time() const {
return user_interaction_time_;
}
void set_user_interaction_time(absl::optional<base::Time> time);
// For loading/copying DIPSState objects only.
void set_user_interaction_time_on_load(absl::optional<base::Time> time);

private:
raw_ptr<DIPSStorage> storage_;
Expand Down

0 comments on commit b13a076

Please sign in to comment.