Skip to content

Commit

Permalink
[M110] Merge to release branch to run DIPS experiment in M110 (1/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.

Bug: 1404955

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

[DIPS] Reorder params in DIPSService::RecordBounce for consistency

The method signature of the DIPSStorage equivalent is
RecordBounce(const GURL& url, base::Time time, bool stateful), this CL
reorders DIPSService::RecordBounce to have these params in the same
order.

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

[DIPS] Migrate the DIPS db to version 2.

Version 2 of the database will stop tracking `stateless` bounce times
in favor of tracking the times of any bounce. This version also makes
the DIPS db support NULL values for timestamps, instead of using
`base::Time()` as the empty value.

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

Improve DIPSService/DIPSWebContentsObserver creation.

Only create DIPSService when dips::kFeature is enabled, and enable it by
default.

Rely on ProfileSelections to determine whether the service should be
created for a Profile. Configure it to allow regular (including
incognito) and OTR-only guest profiles.

Only create DIPSWebContentsObserver when DIPSService::Get() returns
non-null.

Bug: 1400451
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4098585
Auto-Submit: Ryan Tarpine <rtarpine@chromium.org>
Reviewed-by: Joshua Hood <jdh@chromium.org>
Commit-Queue: Joshua Hood <jdh@chromium.org>
Quick-Run: Ryan Tarpine <rtarpine@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1084439}
(cherry picked from commit 3d8f79f)

Add a scoped helper class to help test ProfileKeyedServiceFactorys.

By design, ProfileKeyedServiceFactory doesn't allow overriding
GetBrowserContextToUse(). So, to customize behavior based on a
base::Feature, the only means is to set its ProfileSelections value
based on the Feature. In production, this won't change during runtime,
so setting it once in the constructor is sufficient. But tests may need to vary it, so this CL defines a scoped helper class that
overrides the ProfileSelections and later reverts the change when the
scope ends.

(cherry picked from commit 7f9a229)

Bug: 1400451
Change-Id: I949de545b25b1c3e730857cbc13940b229af410b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4098624
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Ryan Sultanem <rsult@google.com>
Commit-Queue: Ryan Tarpine <rtarpine@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1084360}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4133655
Auto-Submit: Kirubel Aklilu <kaklilu@chromium.org>
Reviewed-by: Ryan Tarpine <rtarpine@chromium.org>
Commit-Queue: Kirubel Aklilu <kaklilu@chromium.org>
Cr-Commit-Position: refs/branch-heads/5481@{#154}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
Kirubel Aklilu authored and Chromium LUCI CQ committed Jan 6, 2023
1 parent 6246838 commit e902730
Show file tree
Hide file tree
Showing 24 changed files with 817 additions and 287 deletions.
14 changes: 14 additions & 0 deletions chrome/browser/dips/BUILD.gn
Expand Up @@ -8,19 +8,33 @@ source_set("unit_tests") {
"cookie_access_filter_unittest.cc",
"dips_bounce_detector_unittest.cc",
"dips_database_unittest.cc",
"dips_service_unittest.cc",
"dips_storage_unittest.cc",
"dips_utils_unittest.cc",
]

deps = [
":golden_dbs_bundle_data",
"//base",
"//base/test:test_support",
"//chrome/browser",
"//chrome/test:test_support",
"//components/ukm:test_support",
"//content/test:test_support",
"//sql:test_support",
"//testing/gtest",
"//url",
]

data = [ "//chrome/test/data/dips/v1.sql" ]
}

bundle_data("golden_dbs_bundle_data") {
visibility = [ ":unit_tests" ]
testonly = true
sources = [ "//chrome/test/data/dips/v1.sql" ]
outputs = [ "{{bundle_resources_dir}}/" +
"{{source_root_relative_dir}}/{{source_file_part}}" ]
}

source_set("browser_tests") {
Expand Down
38 changes: 14 additions & 24 deletions chrome/browser/dips/dips_bounce_detector.cc
Expand Up @@ -16,17 +16,12 @@
#include "chrome/browser/dips/dips_redirect_info.h"
#include "chrome/browser/dips/dips_service.h"
#include "chrome/browser/dips/dips_utils.h"
#include "chrome/browser/profiles/profile.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/navigation_handle_user_data.h"
#include "services/metrics/public/cpp/ukm_recorder.h"

#if BUILDFLAG(IS_CHROMEOS_ASH)
#include "chrome/browser/ash/profiles/profile_helper.h"
#endif

using content::NavigationHandle;

ServerBounceDetectionState::ServerBounceDetectionState() = default;
Expand Down Expand Up @@ -65,29 +60,20 @@ inline void UmaHistogramTimeToBounce(base::TimeDelta sample) {
/* static */
void DIPSWebContentsObserver::MaybeCreateForWebContents(
content::WebContents* web_contents) {
Profile* profile =
Profile::FromBrowserContext(web_contents->GetBrowserContext());

if (profile->IsSystemProfile())
return;

#if BUILDFLAG(IS_CHROMEOS_ASH)
// ChromeOS creates various irregular profiles (login, lock screen...); they
// are of type kRegular (returns true for `Profile::IsRegular()`), that aren't
// used to browse the web and users can't configure. Don't collect metrics
// about them.
if (!ash::ProfileHelper::IsUserProfile(profile))
auto* dips_service = DIPSService::Get(web_contents->GetBrowserContext());
if (!dips_service) {
return;
#endif // BUILDFLAG(IS_CHROMEOS_ASH)
}

DIPSWebContentsObserver::CreateForWebContents(web_contents);
DIPSWebContentsObserver::CreateForWebContents(web_contents, dips_service);
}

DIPSWebContentsObserver::DIPSWebContentsObserver(
content::WebContents* web_contents)
content::WebContents* web_contents,
DIPSService* dips_service)
: content::WebContentsObserver(web_contents),
content::WebContentsUserData<DIPSWebContentsObserver>(*web_contents),
dips_service_(DIPSService::Get(web_contents->GetBrowserContext())),
dips_service_(dips_service),
detector_(this,
base::DefaultTickClock::GetInstance(),
base::DefaultClock::GetInstance()) {}
Expand Down Expand Up @@ -267,8 +253,10 @@ void DIPSBounceDetector::DidStartNavigation(
base::TimeDelta(base::Seconds(kBounceThresholdSeconds)))) {
// Time between page load and client-side redirect starting is only
// tracked for stateful bounces.
if (client_detection_state_->cookie_access_type > CookieAccessType::kNone)
if (client_detection_state_->cookie_access_type >
CookieAccessType::kNone) {
UmaHistogramTimeToBounce(bounce_time);
}

client_redirect = std::make_unique<DIPSRedirectInfo>(
/*url=*/delegate_->GetLastCommittedURL(),
Expand Down Expand Up @@ -418,16 +406,18 @@ void DIPSWebContentsObserver::FrameReceivedUserActivation(
content::RenderFrameHost* render_frame_host) {
// Ignore iframe activations since we only care for its associated main-frame
// interactions on the top-level site.
if (!render_frame_host->IsInPrimaryMainFrame())
if (!render_frame_host->IsInPrimaryMainFrame()) {
return;
}

detector_.OnUserActivation();
}

void DIPSBounceDetector::OnUserActivation() {
GURL url = delegate_->GetLastCommittedURL();
if (!url.SchemeIsHTTPOrHTTPS())
if (!url.SchemeIsHTTPOrHTTPS()) {
return;
}

base::Time now = clock_->Now();
if (client_detection_state_.has_value()) {
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/dips/dips_bounce_detector.h
Expand Up @@ -214,7 +214,8 @@ class DIPSWebContentsObserver
}

private:
explicit DIPSWebContentsObserver(content::WebContents* web_contents);
DIPSWebContentsObserver(content::WebContents* web_contents,
DIPSService* dips_service);
// So WebContentsUserData::CreateForWebContents() can call the constructor.
friend class content::WebContentsUserData<DIPSWebContentsObserver>;

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/dips/dips_bounce_detector_unittest.cc
Expand Up @@ -117,7 +117,7 @@ class TestBounceDetectorDelegate : public DIPSBounceDetectorDelegate {
const std::vector<std::string>& redirects() const { return redirects_; }

private:
void RecordBounce(bool stateful, const GURL& url, base::Time time) {
void RecordBounce(const GURL& url, base::Time time, bool stateful) {
recorded_bounces_.insert(std::make_tuple(url, time, stateful));
}

Expand Down

0 comments on commit e902730

Please sign in to comment.