Skip to content

Commit

Permalink
Revert "FLEDGE: Report additional bids to devtools, too."
Browse files Browse the repository at this point in the history 
This reverts commit 124b109.

Reason for revert: frequent flaky failures of
http/tests/inspector-protocol/storage/interest-groups.js
such as in:
https://ci.chromium.org/ui/p/chromium/builders/ci/Linux%20Tests/135180
https://ci.chromium.org/ui/p/chromium/builders/ci/linux-bfcache-rel/51047/overview
https://ci.chromium.org/ui/p/chromium/builders/ci/Mac12%20Tests/12875/overview
https://ci.chromium.org/ui/p/chromium/builders/ci/WebKit%20Win10/122043/overview

Original change's description:
> FLEDGE: Report additional bids to devtools, too.
>
> This mostly involves splitting out the reporting from
> InterestGroupManagerImpl::RecordInterestGroupBids and RecordInterestGroupWin and just doing it directly (and, for bids, also at earlier time --- waiting for the reporter to notify devtools of those doesn't seem necessary).
>
> Two new enum value is also added for additional bids + them winning, to distinguish them from regular DB bids, which have separate namespace.
>
> Bug: 1475640
> Change-Id: I831d13584edd764d6f4530c42a3e4620f09b3262
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4873598
> Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
> Reviewed-by: Russ Hamilton <behamilton@google.com>
> Commit-Queue: Maks Orlovich <morlovich@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1198450}

Bug: 1475640
Change-Id: I203d509a0e6c29cb8de4597ef4090cafd4783b8e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4873434
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: David Baron <dbaron@chromium.org>
Auto-Submit: David Baron <dbaron@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1198598}
  • Loading branch information
@dbaron
dbaron authored and Chromium LUCI CQ committed Sep 19, 2023
1 parent c237a6b commit 8733d37
Show file tree
Hide file tree
Showing 12 changed files with 49 additions and 133 deletions.
6 changes: 0 additions & 6 deletions content/browser/devtools/protocol/storage_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1030,15 +1030,9 @@ void StorageHandler::OnInterestGroupAccessed(
case AccessType::kBid:
type_enum = Storage::InterestGroupAccessTypeEnum::Bid;
break;
case AccessType::kAdditionalBid:
type_enum = Storage::InterestGroupAccessTypeEnum::AdditionalBid;
break;
case AccessType::kWin:
type_enum = Storage::InterestGroupAccessTypeEnum::Win;
break;
case AccessType::kAdditionalBidWin:
type_enum = Storage::InterestGroupAccessTypeEnum::AdditionalBidWin;
break;
};
frontend_->InterestGroupAccessed(access_time.ToDoubleT(), type_enum,
owner_origin.Serialize(), name);
Expand Down
17 changes: 1 addition & 16 deletions content/browser/interest_group/auction_runner_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4752,24 +4752,9 @@ TEST_F(AuctionRunnerTest, ComponentAuctionSharedBuyer) {
ElementsAreRequests(
BuildPrivateAggregationRequest(/*bucket=*/10, /*value=*/23),
BuildPrivateAggregationRequest(/*bucket=*/30, /*value=*/43)))));

// Bid count should only be incremented by 1.
base::RunLoop run_loop;
interest_group_manager_->GetInterestGroup(
kBidder1Key,
base::BindLambdaForTesting(
[&](absl::optional<StorageInterestGroup> interest_group) {
ASSERT_TRUE(interest_group);
// MakeInterestGroup() set `bid_count` to 5, so it should be 6
// (not 7).
EXPECT_EQ(6, interest_group->bidding_browser_signals->bid_count);
run_loop.Quit();
}));
run_loop.Run();

// Both uses should get reported to the observer, however.
EXPECT_THAT(result_.interest_groups_that_bid,
testing::UnorderedElementsAre(kBidder1Key, kBidder1Key));
testing::UnorderedElementsAre(kBidder1Key));
EXPECT_EQ(R"({"renderURL":"https://component2-bid.test/"})",
result_.winning_group_ad_metadata);
// Currently an interest group participating twice in an auction is counted
Expand Down
27 changes: 7 additions & 20 deletions content/browser/interest_group/interest_group_auction.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -976,10 +976,6 @@ class InterestGroupAuction::BuyerHelper
if (bid_state->made_bid) {
interest_groups.emplace(bid_state->bidder->interest_group.owner,
bid_state->bidder->interest_group.name);
auction_->interest_group_manager_->NotifyInterestGroupAccessed(
InterestGroupManagerImpl::InterestGroupObserver::kBid,
bid_state->bidder->interest_group.owner,
bid_state->bidder->interest_group.name);
bid_count++;
}
}
Expand Down Expand Up @@ -2650,29 +2646,20 @@ void InterestGroupAuction::GetInterestGroupsThatBidAndReportBidCounts(
if (saved_response_) {
interest_groups.insert(saved_response_->bidding_groups.begin(),
saved_response_->bidding_groups.end());
for (const auto& ig_bid : interest_groups) {
interest_group_manager_->NotifyInterestGroupAccessed(
InterestGroupManagerImpl::InterestGroupObserver::
InterestGroupObserver::kBid,
ig_bid.owner, ig_bid.name);
}
return;
}

for (auto& buyer_helper : buyer_helpers_) {
buyer_helper->GetInterestGroupsThatBidAndReportBidCounts(interest_groups);
}

// Notify devtools of additional bids. These don't go into `interest_groups`,
// that's only things in the database.
for (const auto& bid_state : bid_states_for_additional_bids_) {
DCHECK(bid_state->made_bid);
interest_group_manager_->NotifyInterestGroupAccessed(
InterestGroupManagerImpl::InterestGroupObserver::InterestGroupObserver::
kAdditionalBid,
bid_state->bidder->interest_group.owner,
bid_state->bidder->interest_group.name);
}
// TODO(http://crbug.com/1464874, https://crbug.com/1475640): Report
// additional bids to devtools as well, similar to what
// BuyerHelper::GetInterestGroupsThatBidAndReportBidCounts does for things
// from interest groups. Likely will need to untangle
// InterestGroupManagerImpl::RecordInterestGroupBids doing both DB recording
// and debugging notification (and reporting is the wrong time for the debug
// info, too).

// Retrieve data from component auctions as well.
for (const auto& component_auction_info : component_auctions_) {
Expand Down
4 changes: 1 addition & 3 deletions content/browser/interest_group/interest_group_auction.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -590,11 +590,9 @@ class CONTENT_EXPORT InterestGroupAuction
// Returns all interest groups that bid in an auction. Expected to be called
// after the bidding and scoring phase completes. Returns an empty set if the
// auction failed for any reason other than the seller rejecting all bids.
// Bids from additional bids are not returned, since they do not really have
// Bids from additional bids are not included, since they do not really have
// interest groups (and we don't want to attribute them to database IGs with
// aliasing names).
//
// All bids (including additional bids) are also reported to the observer.
void GetInterestGroupsThatBidAndReportBidCounts(
blink::InterestGroupSet& interest_groups) const;

Expand Down
7 changes: 0 additions & 7 deletions content/browser/interest_group/interest_group_auction_reporter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -934,13 +934,6 @@ void InterestGroupAuctionReporter::OnNavigateToWinningAd() {
interest_group_manager_->RecordInterestGroupWin(
blink::InterestGroupKey(winning_group.owner, winning_group.name),
winning_bid_info_.ad_metadata);
interest_group_manager_->NotifyInterestGroupAccessed(
InterestGroupManagerImpl::InterestGroupObserver::kWin,
winning_group.owner, winning_group.name);
} else {
interest_group_manager_->NotifyInterestGroupAccessed(
InterestGroupManagerImpl::InterestGroupObserver::kAdditionalBidWin,
winning_group.owner, winning_group.name);
}

interest_group_manager_->RegisterAdKeysAsJoined(
Expand Down
70 changes: 27 additions & 43 deletions content/browser/interest_group/interest_group_auction_reporter_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,24 +145,14 @@ class InterestGroupAuctionReporterTest
"\"This be metadata\""}}})
.Build();

// Join the interest groups that "won" and "lost" the auction - this matters
// for tests that make sure the interest group is updated correctly.
// Join the interest group that "won" the auction - this matters for tests
// that make sure the interest group is updated correctly.
const blink::InterestGroup& interest_group =
winning_bid_info_.storage_interest_group->interest_group;
interest_group_manager_impl_->JoinInterestGroup(
interest_group,
/*joining_url=*/kWinningBidderOrigin.GetURL());

auto losing_interest_group =
blink::TestInterestGroupBuilder(kLosingBidderOrigin, kLosingBidderName)
.SetBiddingUrl(kLosingBidderScriptUrl)
// A non-empty ad list is needed by KAnonKeyForAdBid().
.SetAds({{{GURL("https://ad.render.url.test/"), "null"}}})
.Build();
interest_group_manager_impl_->JoinInterestGroup(
losing_interest_group,
/*joining_url=*/kLosingBidderOrigin.GetURL());

winning_bid_info_.render_url = (*interest_group.ads)[0].render_url;
winning_bid_info_.ad_metadata = kWinningAdMetadata;

Expand Down Expand Up @@ -357,26 +347,6 @@ class InterestGroupAuctionReporterTest
EXPECT_EQ((*prev_wins)[0]->ad_json, kWinningAdMetadata);
}

void ExpectBidsForKey(const url::Origin& origin,
const std::string& name,
int expected_bids) {
absl::optional<StorageInterestGroup> interest_group =
interest_group_manager_impl_->BlockingGetInterestGroup(origin, name);
ASSERT_TRUE(interest_group);
EXPECT_EQ(expected_bids, interest_group->bidding_browser_signals->bid_count)
<< origin << "," << name;
}

void ExpectNoBidsRecorded() {
ExpectBidsForKey(kWinningBidderOrigin, kWinningBidderName, 0);
ExpectBidsForKey(kLosingBidderOrigin, kLosingBidderName, 0);
}

void ExpectBidsRecordedOnce() {
ExpectBidsForKey(kWinningBidderOrigin, kWinningBidderName, 1);
ExpectBidsForKey(kLosingBidderOrigin, kLosingBidderName, 1);
}

// AuctionWorkletManager::Delegate implementation.
//
// Note that none of these matter for these tests, as the the mock worklet
Expand Down Expand Up @@ -450,8 +420,6 @@ class InterestGroupAuctionReporterTest
const std::string kWinningBidderName = "winning interest group name";
const std::string kWinningAdMetadata = R"({"render_url": "https://foo/"})";

const GURL kLosingBidderScriptUrl =
GURL("https://losing.bidder.origin.test/bidder_script.js");
const url::Origin kLosingBidderOrigin =
url::Origin::Create(GURL("https://losing.bidder.origin.test/"));
const std::string kLosingBidderName = "losing interest group name";
Expand All @@ -465,6 +433,10 @@ class InterestGroupAuctionReporterTest
const url::Origin kComponentSellerOrigin =
url::Origin::Create(kComponentSellerScriptUrl);

const std::vector<blink::InterestGroupKey> kExpectedInterestGroupsThatBid{
{kWinningBidderOrigin, kWinningBidderName},
{kLosingBidderOrigin, kLosingBidderName}};

// Private aggregation requests. Their values don't matter, beyond that
// they're different from each other.
const auction_worklet::mojom::PrivateAggregationRequestPtr
Expand Down Expand Up @@ -1264,47 +1236,56 @@ TEST_F(InterestGroupAuctionReporterTest, DebugReportsLateNavigation) {
TEST_F(InterestGroupAuctionReporterTest, RecordWinAndBids) {
SetUpAndStartSingleSellerAuction();
ExpectNoWinsRecorded();
ExpectNoBidsRecorded();
EXPECT_THAT(interest_group_manager_impl_->TakeInterestGroupsThatBid(),
testing::UnorderedElementsAre());

// The win and bids should be recorded immediately upon navigation.
interest_group_auction_reporter_->OnNavigateToWinningAdCallback().Run();
ExpectWinRecordedOnce();
ExpectBidsRecordedOnce();
EXPECT_THAT(
interest_group_manager_impl_->TakeInterestGroupsThatBid(),
testing::UnorderedElementsAreArray(kExpectedInterestGroupsThatBid));

WaitForReportResultAndRunCallback(kSellerScriptUrl, absl::nullopt);
WaitForReportWinAndRunCallback(absl::nullopt);
WaitForCompletion();

// The win and bids should have been recorded only once.
ExpectWinRecordedOnce();
ExpectBidsRecordedOnce();
EXPECT_THAT(interest_group_manager_impl_->TakeInterestGroupsThatBid(),
testing::UnorderedElementsAre());
}

// Check that the winning interest group and bids are reported to the
// InterestGroupManager, in the case where the fenced frame is navigated to only
// after all reporting scripts have been run.
TEST_F(InterestGroupAuctionReporterTest, RecordWinAndBidsLateNavigation) {
SetUpAndStartSingleSellerAuction();
ExpectNoBidsRecorded();
EXPECT_THAT(interest_group_manager_impl_->TakeInterestGroupsThatBid(),
testing::UnorderedElementsAre());

WaitForReportResultAndRunCallback(kSellerScriptUrl, absl::nullopt);
WaitForReportWinAndRunCallback(absl::nullopt);

// Running reporting scripts should not cause the win or any bids to be
// recorded.
ExpectNoWinsRecorded();
ExpectNoBidsRecorded();
EXPECT_THAT(interest_group_manager_impl_->TakeInterestGroupsThatBid(),
testing::UnorderedElementsAre());

// The bids should be recorded immediately upon navigation.
interest_group_auction_reporter_->OnNavigateToWinningAdCallback().Run();
ExpectWinRecordedOnce();
ExpectBidsRecordedOnce();
EXPECT_THAT(
interest_group_manager_impl_->TakeInterestGroupsThatBid(),
testing::UnorderedElementsAreArray(kExpectedInterestGroupsThatBid));

WaitForCompletion();

// The win and bids should have been recorded only once.
ExpectWinRecordedOnce();
ExpectBidsRecordedOnce();
EXPECT_THAT(interest_group_manager_impl_->TakeInterestGroupsThatBid(),
testing::UnorderedElementsAre());
}

// Check that the passed in `k_anon_keys_to_join` are reported to the
Expand Down Expand Up @@ -1715,7 +1696,8 @@ TEST_F(InterestGroupAuctionReporterTest, NoNavigation) {
EXPECT_THAT(interest_group_manager_impl_->TakeJoinedKAnonSets(),
testing::UnorderedElementsAre());
ExpectNoWinsRecorded();
ExpectNoBidsRecorded();
EXPECT_THAT(interest_group_manager_impl_->TakeInterestGroupsThatBid(),
testing::UnorderedElementsAre());
EXPECT_THAT(private_aggregation_manager_.TakePrivateAggregationRequests(),
testing::UnorderedElementsAre());
EXPECT_TRUE(
Expand Down Expand Up @@ -1782,7 +1764,9 @@ TEST_F(InterestGroupAuctionReporterTest, MultipleNavigations) {
EXPECT_THAT(interest_group_manager_impl_->TakeJoinedKAnonSets(),
testing::UnorderedElementsAreArray(k_anon_keys_to_join_));
ExpectWinRecordedOnce();
ExpectBidsRecordedOnce();
EXPECT_THAT(
interest_group_manager_impl_->TakeInterestGroupsThatBid(),
testing::UnorderedElementsAreArray(kExpectedInterestGroupsThatBid));

// Private aggregation data should have been passed along only once.
EXPECT_THAT(
Expand Down
19 changes: 1 addition & 18 deletions content/browser/interest_group/interest_group_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16440,15 +16440,12 @@ IN_PROC_BROWSER_TEST_F(InterestGroupBrowserTest,

for (bool should_win : {true, false}) {
SCOPED_TRACE(should_win);
AttachInterestGroupObserver();
ClearReceivedRequests();
ASSERT_TRUE(NavigateToURL(shell(), test_url));
std::string auction_nonce = CreateAuctionNonceAndWait();

GURL additional_bid_logic_url = https_server_->GetURL(
"b.test", "/interest_group/bidding_logic_additional_bid.js");
url::Origin additional_bid_origin =
url::Origin::Create(additional_bid_logic_url);

std::string auction_config = JsReplace(
R"({
Expand All @@ -16473,7 +16470,7 @@ IN_PROC_BROWSER_TEST_F(InterestGroupBrowserTest,
test_origin,
https_server_->GetURL("a.test", "/interest_group/decision_logic.js"),
auction_nonce, additional_bid_ad_url, additional_bid_logic_url,
additional_bid_origin, should_win ? 1.99 : 0.1);
url::Origin::Create(additional_bid_logic_url), should_win ? 1.99 : 0.1);

RunAuctionAndWaitForURLAndNavigateIframe(
auction_config, should_win ? additional_bid_ad_url : ad_url);
Expand All @@ -16483,24 +16480,10 @@ IN_PROC_BROWSER_TEST_F(InterestGroupBrowserTest,
https_server_->GetURL("a.test", "/echoall?report_bidder_additional"));
EXPECT_FALSE(HasServerSeenUrl(
https_server_->GetURL("a.test", "/echoall?report_bidder")));
WaitForAccessObserved({
{TestInterestGroupObserver::kLoaded, test_origin, "cars"},
{TestInterestGroupObserver::kBid, test_origin, "cars"},
{TestInterestGroupObserver::kAdditionalBid, additional_bid_origin,
"campaign123"},
{TestInterestGroupObserver::kAdditionalBidWin, additional_bid_origin,
"campaign123"},
});
} else {
WaitForUrl(https_server_->GetURL("a.test", "/echoall?report_bidder"));
EXPECT_FALSE(HasServerSeenUrl(https_server_->GetURL(
"a.test", "/echoall?report_bidder_additional")));
WaitForAccessObserved(
{{TestInterestGroupObserver::kLoaded, test_origin, "cars"},
{TestInterestGroupObserver::kBid, test_origin, "cars"},
{TestInterestGroupObserver::kAdditionalBid, additional_bid_origin,
"campaign123"},
{TestInterestGroupObserver::kWin, test_origin, "cars"}});
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions content/browser/interest_group/interest_group_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,19 @@ void InterestGroupManagerImpl::RecordInterestGroupBids(
if (group_keys.empty()) {
return;
}
for (const auto& group_key : group_keys) {
NotifyInterestGroupAccessed(InterestGroupObserver::kBid, group_key.owner,
group_key.name);
}
impl_.AsyncCall(&InterestGroupStorage::RecordInterestGroupBids)
.WithArgs(group_keys);
}

void InterestGroupManagerImpl::RecordInterestGroupWin(
const blink::InterestGroupKey& group_key,
const std::string& ad_json) {
NotifyInterestGroupAccessed(InterestGroupObserver::kWin, group_key.owner,
group_key.name);
impl_.AsyncCall(&InterestGroupStorage::RecordInterestGroupWin)
.WithArgs(group_key, std::move(ad_json));
}
Expand Down
19 changes: 4 additions & 15 deletions content/browser/interest_group/interest_group_manager_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,7 @@ class CONTENT_EXPORT InterestGroupManagerImpl : public InterestGroupManager {

class CONTENT_EXPORT InterestGroupObserver : public base::CheckedObserver {
public:
enum AccessType {
kJoin,
kLeave,
kUpdate,
kLoaded,
kBid,
kAdditionalBid,
kWin,
kAdditionalBidWin
};
enum AccessType { kJoin, kLeave, kUpdate, kLoaded, kBid, kWin };
virtual void OnInterestGroupAccessed(const base::Time& access_time,
AccessType type,
const url::Origin& owner_origin,
Expand Down Expand Up @@ -199,7 +190,6 @@ class CONTENT_EXPORT InterestGroupManagerImpl : public InterestGroupManager {

// Adds an entry to the bidding history for this interest group.
void RecordInterestGroupBids(const blink::InterestGroupSet& groups);

// Adds an entry to the win history for this interest group. `ad_json` is a
// piece of opaque data to identify the winning ad.
void RecordInterestGroupWin(const blink::InterestGroupKey& group_key,
Expand Down Expand Up @@ -351,10 +341,6 @@ class CONTENT_EXPORT InterestGroupManagerImpl : public InterestGroupManager {
k_anonymity_manager_ = std::move(k_anonymity_manager);
}

void NotifyInterestGroupAccessed(InterestGroupObserver::AccessType type,
const url::Origin& owner_origin,
const std::string& name);

private:
// InterestGroupUpdateManager calls private members to write updates to the
// database.
Expand Down Expand Up @@ -433,6 +419,9 @@ class CONTENT_EXPORT InterestGroupManagerImpl : public InterestGroupManager {
// To be called only by `update_manager_`.
void ReportUpdateFailed(const blink::InterestGroupKey& group_key,
bool parse_failure);
void NotifyInterestGroupAccessed(InterestGroupObserver::AccessType type,
const url::Origin& owner_origin,
const std::string& name);

void OnGetInterestGroupsComplete(
base::OnceCallback<void(std::vector<StorageInterestGroup>)> callback,
Expand Down
Loading

0 comments on commit 8733d37

Please sign in to comment.