Revert "Implement ClearSiteDataOnChangedSetsForContext method for FPS."
This reverts commit 6604d5dac85d24d233791d4cf889fa040004a169.
Reason for revert: Suspected culprit for multiple ASAN failures on linux bots. Example: https://ci.chromium.org/ui/p/chromium/builders/ci/linux-chromeos-dbg/30359/overview
Original change's description:
> Implement ClearSiteDataOnChangedSetsForContext method for FPS.
>
> This CL:
> - triggers site state clearing and update DB
> - adds metric to record whether clearing is successful.
>
> Bug: 1219656
> Change-Id: I406be199255b08b962f423b21ca5ca2a4e17bf4c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3857668
> Reviewed-by: Chris Fredrickson <cfredric@chromium.org>
> Commit-Queue: Shuran Huang <shuuran@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1055983}
Bug: 1219656, 1372171
Change-Id: Ic5bb143d285536cacb413a3bba1484083ecf3963
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3937711
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Meredith Lane <meredithl@chromium.org>
Commit-Queue: Meredith Lane <meredithl@chromium.org>
Owners-Override: Meredith Lane <meredithl@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1056092}
diff --git a/content/browser/first_party_sets/first_party_sets_handler_impl.cc b/content/browser/first_party_sets/first_party_sets_handler_impl.cc
index 8122aa6..5ba79be 100644
--- a/content/browser/first_party_sets/first_party_sets_handler_impl.cc
+++ b/content/browser/first_party_sets/first_party_sets_handler_impl.cc
@@ -10,15 +10,12 @@
#include "base/bind.h"
#include "base/files/file.h"
#include "base/logging.h"
-#include "base/metrics/histogram_functions.h"
#include "base/task/thread_pool.h"
#include "base/types/optional_util.h"
#include "base/values.h"
#include "content/browser/first_party_sets/first_party_set_parser.h"
#include "content/browser/first_party_sets/first_party_sets_loader.h"
-#include "content/browser/first_party_sets/first_party_sets_site_data_remover.h"
#include "content/browser/first_party_sets/local_set_declaration.h"
-#include "content/public/browser/browser_context.h"
#include "content/public/browser/content_browser_client.h"
#include "content/public/browser/first_party_sets_handler.h"
#include "content/public/common/content_client.h"
@@ -33,30 +30,6 @@
constexpr base::FilePath::CharType kFirstPartySetsDatabase[] =
FILE_PATH_LITERAL("first_party_sets.db");
-using ClearSiteDataOutcomeType =
- FirstPartySetsHandlerImpl::ClearSiteDataOutcomeType;
-
-// `failed_data_types` is a bitmask used to indicate data types from
-// BrowsingDataRemover::DataType enum that were failed to remove.
-ClearSiteDataOutcomeType ComputeClearSiteDataOutcome(
- uint64_t failed_data_types) {
- ClearSiteDataOutcomeType outcome = ClearSiteDataOutcomeType::kSuccess;
- if (failed_data_types & BrowsingDataRemover::DATA_TYPE_COOKIES) {
- outcome = ClearSiteDataOutcomeType::kCookieFailed;
- }
- if (failed_data_types & BrowsingDataRemover::DATA_TYPE_DOM_STORAGE) {
- outcome = outcome == ClearSiteDataOutcomeType::kCookieFailed
- ? ClearSiteDataOutcomeType::kCookieAndStorageFailed
- : ClearSiteDataOutcomeType::kStorageFailed;
- }
- return outcome;
-}
-
-void RecordClearSiteDataOutcome(ClearSiteDataOutcomeType outcome) {
- base::UmaHistogramEnumeration(
- "FirstPartySets.Initialization.ClearSiteDataOutcomeType", outcome);
-}
-
} // namespace
// static
@@ -307,81 +280,14 @@
DCHECK(!browser_context_id.empty());
DCHECK(enabled_);
- if (db_helper_.is_null()) {
- VLOG(1) << "Invalid First-Party Sets database. Failed to clear site data "
- "for browser_context_id="
- << browser_context_id;
- std::move(callback).Run(std::move(context_config));
- return;
+ if (!db_helper_.is_null()) {
+ // TODO(crbug.com/1219656): Call site state clearing.
+ // TODO(https://crbug.com/1219656): don't invoke `callback` until site state
+ // clearing is complete.
+ db_helper_.AsyncCall(&FirstPartySetsHandlerDatabaseHelper::PersistSets)
+ .WithArgs(browser_context_id, version_, global_sets_->Clone(),
+ context_config.Clone());
}
- // Extract the callback into a variable and pass it into DB async call args,
- // to prevent the case that `context_config` gets used after it's moved. This
- // is because C++ does not have a defined evaluation order for function
- // parameters.
- base::OnceCallback<void(const std::vector<net::SchemefulSite>&)>
- on_get_sites_to_clear = base::BindOnce(
- &FirstPartySetsHandlerImpl::OnGetSitesToClear,
- // base::Unretained(this) is safe here because this
- // is a static singleton.
- base::Unretained(this), browser_context_getter, browser_context_id,
- context_config.Clone(), std::move(callback));
-
- db_helper_
- .AsyncCall(&FirstPartySetsHandlerDatabaseHelper::
- UpdateAndGetSitesToClearForContext)
- .WithArgs(browser_context_id, global_sets_->Clone(),
- std::move(context_config))
- .Then(std::move(on_get_sites_to_clear));
-}
-
-void FirstPartySetsHandlerImpl::OnGetSitesToClear(
- base::RepeatingCallback<BrowserContext*()> browser_context_getter,
- const std::string& browser_context_id,
- net::FirstPartySetsContextConfig context_config,
- base::OnceCallback<void(net::FirstPartySetsContextConfig)> callback,
- const std::vector<net::SchemefulSite>& sites_to_clear) const {
- DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
- BrowserContext* browser_context = browser_context_getter.Run();
- if (!browser_context) {
- DVLOG(1) << "Invalid Browser Context. Failed to clear site data for "
- "browser_context_id="
- << browser_context_id;
-
- std::move(callback).Run(std::move(context_config));
- return;
- }
-
- FirstPartySetsSiteDataRemover::RemoveSiteData(
- *browser_context->GetBrowsingDataRemover(), sites_to_clear,
- base::BindOnce(
- &FirstPartySetsHandlerImpl::DidClearSiteDataOnChangedSetsForContext,
- // base::Unretained(this) is safe here because
- // this is a static singleton.
- base::Unretained(this), browser_context_id, std::move(context_config),
- std::move(callback)));
-}
-
-void FirstPartySetsHandlerImpl::DidClearSiteDataOnChangedSetsForContext(
- const std::string& browser_context_id,
- net::FirstPartySetsContextConfig context_config,
- base::OnceCallback<void(net::FirstPartySetsContextConfig)> callback,
- uint64_t failed_data_types) const {
- DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
- DCHECK(!db_helper_.is_null());
-
- ClearSiteDataOutcomeType outcome =
- ComputeClearSiteDataOutcome(failed_data_types);
- RecordClearSiteDataOutcome(outcome);
- if (outcome == ClearSiteDataOutcomeType::kSuccess) {
- db_helper_
- .AsyncCall(
- &FirstPartySetsHandlerDatabaseHelper::UpdateClearStatusForContext)
- .WithArgs(browser_context_id);
- }
- db_helper_.AsyncCall(&FirstPartySetsHandlerDatabaseHelper::PersistSets)
- .WithArgs(browser_context_id, version_, global_sets_->Clone(),
- context_config.Clone());
std::move(callback).Run(std::move(context_config));
}
diff --git a/content/browser/first_party_sets/first_party_sets_handler_impl.h b/content/browser/first_party_sets/first_party_sets_handler_impl.h
index db1fab84..20c0a7e 100644
--- a/content/browser/first_party_sets/first_party_sets_handler_impl.h
+++ b/content/browser/first_party_sets/first_party_sets_handler_impl.h
@@ -41,28 +41,6 @@
// the current First-Party Sets data to disk.
class CONTENT_EXPORT FirstPartySetsHandlerImpl : public FirstPartySetsHandler {
public:
- // The outcome types of clearing site data for data types covered in
- // FirstPartySetsSiteDataRemover. We only clear
- // `BrowsingDataRemover::DATA_TYPE_COOKIES` and
- // `BrowsingDataRemover::DATA_TYPE_DOM_STORAGE` for now. Cache is "cleared"
- // with a different approach.
- //
- // These values are persisted to logs. Entries should not be renumbered and
- // numeric values should never be reused.
- enum class ClearSiteDataOutcomeType {
- kSuccess = 0,
- // Failed to clear data type of `BrowsingDataRemover::DATA_TYPE_COOKIES`.
- kCookieFailed = 1,
- // Failed to clear data type of
- // `BrowsingDataRemover::DATA_TYPE_DOM_STORAGE`.
- kStorageFailed = 2,
- // Failed to clear both `BrowsingDataRemover::DATA_TYPE_COOKIES` and
- // `BrowsingDataRemover::DATA_TYPE_DOM_STORAGE` data types.
- kCookieAndStorageFailed = 3,
-
- kMaxValue = kCookieAndStorageFailed,
- };
-
using SetsReadyOnceCallback =
base::OnceCallback<void(net::GlobalFirstPartySets)>;
@@ -109,6 +87,7 @@
const base::Value::Dict* policy,
base::OnceCallback<void(net::FirstPartySetsContextConfig)> callback)
override;
+ // TODO(shuuran@chromium.org): Implement the code to clear site state.
void ClearSiteDataOnChangedSetsForContext(
base::RepeatingCallback<BrowserContext*()> browser_context_getter,
const std::string& browser_context_id,
@@ -174,22 +153,6 @@
net::FirstPartySetsContextConfig GetContextConfigForPolicyInternal(
const base::Value::Dict& policy) const;
- void OnGetSitesToClear(
- base::RepeatingCallback<BrowserContext*()> browser_context_getter,
- const std::string& browser_context_id,
- net::FirstPartySetsContextConfig context_config,
- base::OnceCallback<void(net::FirstPartySetsContextConfig)> callback,
- const std::vector<net::SchemefulSite>& sites_to_clear) const;
-
- // `failed_data_types` is a bitmask used to indicate data types from
- // BrowsingDataRemover::DataType enum that were failed to remove. 0 indicates
- // success.
- void DidClearSiteDataOnChangedSetsForContext(
- const std::string& browser_context_id,
- net::FirstPartySetsContextConfig context_config,
- base::OnceCallback<void(net::FirstPartySetsContextConfig)> callback,
- uint64_t failed_data_types) const;
-
// Whether Init has been called already or not.
bool initialized_ = false;
diff --git a/content/browser/first_party_sets/first_party_sets_handler_impl_unittest.cc b/content/browser/first_party_sets/first_party_sets_handler_impl_unittest.cc
index ca6e01a..6d77cc5 100644
--- a/content/browser/first_party_sets/first_party_sets_handler_impl_unittest.cc
+++ b/content/browser/first_party_sets/first_party_sets_handler_impl_unittest.cc
@@ -12,15 +12,12 @@
#include "base/files/scoped_temp_dir.h"
#include "base/json/json_reader.h"
#include "base/run_loop.h"
-#include "base/test/metrics/histogram_tester.h"
#include "base/test/task_environment.h"
#include "base/test/test_future.h"
#include "base/version.h"
#include "content/browser/first_party_sets/first_party_set_parser.h"
#include "content/browser/first_party_sets/local_set_declaration.h"
#include "content/public/browser/first_party_sets_handler.h"
-#include "content/public/test/browser_task_environment.h"
-#include "content/public/test/test_browser_context.h"
#include "net/base/schemeful_site.h"
#include "net/first_party_sets/first_party_set_entry.h"
#include "net/first_party_sets/first_party_sets_context_config.h"
@@ -50,9 +47,6 @@
const char* kPrimaryField = "primary";
const char* kCctldsField = "ccTLDs";
-const char* kFirstPartySetsClearSiteDataOutcomeHistogram =
- "FirstPartySets.Initialization.ClearSiteDataOutcomeType";
-
BrowserContext* FakeBrowserContextGetter() {
return nullptr;
}
@@ -188,12 +182,9 @@
FirstPartySetsHandlerImpl::GetInstance()->ResetForTesting();
}
- BrowserContext* context() { return &context_; }
-
protected:
base::ScopedTempDir scoped_dir_;
- BrowserTaskEnvironment env_;
- TestBrowserContext context_;
+ base::test::TaskEnvironment env_;
};
class FirstPartySetsHandlerImplEnabledTest
@@ -227,7 +218,6 @@
TEST_F(FirstPartySetsHandlerImplEnabledTest,
ClearSiteDataOnChangedSetsForContext_Successful) {
- base::HistogramTester histogram;
net::SchemefulSite foo(GURL("https://foo.test"));
net::SchemefulSite associated(GURL("https://associatedsite.test"));
@@ -251,13 +241,11 @@
Pair(associated, net::FirstPartySetEntry(
foo, net::SiteType::kAssociated, 0))));
- // Should not yet be recorded.
- histogram.ExpectTotalCount(kFirstPartySetsClearSiteDataOutcomeHistogram, 0);
base::RunLoop run_loop;
FirstPartySetsHandlerImpl::GetInstance()
->ClearSiteDataOnChangedSetsForContext(
- base::BindLambdaForTesting([&]() { return context(); }),
- browser_context_id, net::FirstPartySetsContextConfig(),
+ base::BindRepeating(&FakeBrowserContextGetter), browser_context_id,
+ net::FirstPartySetsContextConfig(),
base::BindLambdaForTesting(
[&](net::FirstPartySetsContextConfig) { run_loop.Quit(); }));
run_loop.Run();
@@ -271,15 +259,10 @@
Pair(associated,
net::FirstPartySetEntry(foo, net::SiteType::kAssociated,
absl::nullopt))));
-
- histogram.ExpectUniqueSample(
- kFirstPartySetsClearSiteDataOutcomeHistogram,
- FirstPartySetsHandlerImpl::ClearSiteDataOutcomeType::kSuccess, 1);
}
TEST_F(FirstPartySetsHandlerImplEnabledTest,
ClearSiteDataOnChangedSetsForContext_EmptyDBPath) {
- base::HistogramTester histogram;
net::SchemefulSite foo(GURL("https://foo.test"));
net::SchemefulSite associated(GURL("https://associatedsite.test"));
@@ -313,8 +296,6 @@
run_loop.Run();
EXPECT_EQ(GetPersistedGlobalSetsAndWait(browser_context_id), absl::nullopt);
- // Should not be recorded.
- histogram.ExpectTotalCount(kFirstPartySetsClearSiteDataOutcomeHistogram, 0);
}
TEST_F(FirstPartySetsHandlerImplEnabledTest,
@@ -329,9 +310,8 @@
base::test::TestFuture<net::FirstPartySetsContextConfig> future;
FirstPartySetsHandlerImpl::GetInstance()
->ClearSiteDataOnChangedSetsForContext(
- base::BindLambdaForTesting([&]() { return context(); }),
- browser_context_id, net::FirstPartySetsContextConfig(),
- future.GetCallback());
+ base::BindRepeating(&FakeBrowserContextGetter), browser_context_id,
+ net::FirstPartySetsContextConfig(), future.GetCallback());
FirstPartySetsHandlerImpl::GetInstance()->SetPublicFirstPartySets(
base::Version("0.0.1"),
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
index c3264ba..e10d5ab7 100644
--- a/tools/metrics/histograms/enums.xml
+++ b/tools/metrics/histograms/enums.xml
@@ -43636,13 +43636,6 @@
<int value="4" label="User left the page before first contentful paint"/>
</enum>
-<enum name="FirstPartySetsClearSiteDataOutcomeType">
- <int value="0" label="Success"/>
- <int value="1" label="Failed to clear cookies"/>
- <int value="2" label="Failed to clear storage"/>
- <int value="3" label="Failed to clear cookies and storage"/>
-</enum>
-
<enum name="FirstPartySetsDatabaseInitStatus">
<int value="0" label="Unattempted"/>
<int value="1" label="Success"/>
diff --git a/tools/metrics/histograms/metadata/others/histograms.xml b/tools/metrics/histograms/metadata/others/histograms.xml
index f6a450f5..003ab36 100644
--- a/tools/metrics/histograms/metadata/others/histograms.xml
+++ b/tools/metrics/histograms/metadata/others/histograms.xml
@@ -6225,16 +6225,6 @@
</summary>
</histogram>
-<histogram name="FirstPartySets.Initialization.ClearSiteDataOutcomeType"
- enum="FirstPartySetsClearSiteDataOutcomeType" expires_after="2023-08-20">
- <owner>shuuran@google.com</owner>
- <owner>kaustubhag@google.com</owner>
- <summary>
- Measures the outcome of site data clearing. Recorded when First-Party Sets
- initialization triggers site data clearing for a given browser context.
- </summary>
-</histogram>
-
<histogram name="FirstRun.IOSFirebaseConfigured" enum="FirebaseConfiguredState"
expires_after="M89">
<owner>ghendel@chromium.org</owner>
