0

FLEDGE: Pass previous wins to bidder scripts in chronological order.

Previously, bidder scripts would receive previous wins in whatever order
they were returned by the SQLite database, which, experimentally, was
not typically the same as they order they were added to the database.

This CL adds a sort call in the out-of-process BidderWorklet code to
sort them just before passing them to the script. Where the sort is
doesn't really matter, but might as well do it off the UI thread, since
there's no benefit from doing so.

Bug: 1210757
Change-Id: I6e268da922518dc18693679cc01f74bfb468c8eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2902632
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#884503}
This commit is contained in:
Matt Menke
2021-05-19 16:04:24 +00:00
committed by Chromium LUCI CQ
parent dc5a94abfb
commit b23901fbc8
3 changed files with 59 additions and 37 deletions
content

@ -676,9 +676,11 @@ class AuctionRunnerTest : public testing::Test, public AuctionRunner::Delegate {
std::vector<auction_worklet::mojom::PreviousWinPtr> previous_wins;
previous_wins.push_back(auction_worklet::mojom::PreviousWin::New(
base::Time::Now(), R"({"winner": 0})"));
base::Time::Now() - base::TimeDelta::FromSeconds(2),
R"({"winner": 0})"));
previous_wins.push_back(auction_worklet::mojom::PreviousWin::New(
base::Time::Now(), R"({"winner": -1})"));
base::Time::Now() - base::TimeDelta::FromSeconds(1),
R"({"winner": -1})"));
previous_wins.push_back(auction_worklet::mojom::PreviousWin::New(
base::Time::Now(), R"({"winner": -2})"));

@ -4,6 +4,7 @@
#include "content/services/auction_worklet/bidder_worklet.h"
#include <algorithm>
#include <cmath>
#include <memory>
#include <string>
@ -51,6 +52,40 @@ bool AppendJsonValueOrNull(AuctionV8Helper* const v8_helper,
return true;
}
// Creates a V8 array containing information about the passed in previous wins.
// Array is sorted by time, earliest wins first. Modifies order of `prev_wins`
// input vector. This should should be harmless, since each list of previous
// wins is only used for a single bid in a single auction, and its order is
// unspecified, anyways.
v8::MaybeLocal<v8::Value> CreatePrevWinsArray(
AuctionV8Helper* v8_helper,
v8::Local<v8::Context> context,
base::Time auction_start_time,
std::vector<mojom::PreviousWinPtr>& prev_wins) {
std::sort(prev_wins.begin(), prev_wins.end(),
[](const mojom::PreviousWinPtr& prev_win1,
const mojom::PreviousWinPtr& prev_win2) {
return prev_win1->time < prev_win2->time;
});
std::vector<v8::Local<v8::Value>> prev_wins_v8;
v8::Isolate* isolate = v8_helper->isolate();
for (const auto& prev_win : prev_wins) {
int64_t time_delta = (auction_start_time - prev_win->time).InSeconds();
// Don't give negative times if clock has changed since last auction win.
if (time_delta < 0)
time_delta = 0;
v8::Local<v8::Value> win_values[2];
win_values[0] = v8::Number::New(isolate, time_delta);
if (!v8_helper->CreateValueFromJson(context, prev_win->ad_json)
.ToLocal(&win_values[1])) {
return v8::MaybeLocal<v8::Value>();
}
prev_wins_v8.push_back(
v8::Array::New(isolate, win_values, base::size(win_values)));
}
return v8::Array::New(isolate, prev_wins_v8.data(), prev_wins_v8.size());
}
} // namespace
BidderWorklet::BidderWorklet(
@ -299,27 +334,16 @@ void BidderWorklet::GenerateBidIfReady() {
return;
}
std::vector<v8::Local<v8::Value>> prev_wins_v8;
for (const auto& prev_win : bidding_interest_group_->signals->prev_wins) {
int64_t time_delta = (auction_start_time_ - prev_win->time).InSeconds();
// Don't give negative times if clock has changed since last auction win.
// Clock changes do mean times can be out of numerical order, despite being
// in chronological order.
if (time_delta < 0)
time_delta = 0;
v8::Local<v8::Value> win_values[2];
win_values[0] = v8::Number::New(isolate, time_delta);
if (!v8_helper_->CreateValueFromJson(context, prev_win->ad_json)
.ToLocal(&win_values[1])) {
InvokeBidCallbackOnError();
return;
}
prev_wins_v8.push_back(
v8::Array::New(isolate, win_values, base::size(win_values)));
v8::Local<v8::Value> prev_wins;
if (!CreatePrevWinsArray(v8_helper_, context, auction_start_time_,
bidding_interest_group_->signals->prev_wins)
.ToLocal(&prev_wins)) {
InvokeBidCallbackOnError();
return;
}
v8::Maybe<bool> result = browser_signals->Set(
context, gin::StringToV8(isolate, "prevWins"),
v8::Array::New(isolate, prev_wins_v8.data(), prev_wins_v8.size()));
context, gin::StringToV8(isolate, "prevWins"), prev_wins);
if (result.IsNothing() || !result.FromJust()) {
InvokeBidCallbackOnError();
return;

@ -851,22 +851,18 @@ TEST_F(BidderWorkletTest, GenerateBidParametersOptionalString) {
"[true,true]", 1, GURL("https://response.test/"), base::TimeDelta()));
}
// Utility methods to create vectors of PreviousWin. Needed because StructPtr's
// Utility method to create a vector of PreviousWin. Needed because StructPtrs
// don't allow copying.
std::vector<mojo::StructPtr<mojom::PreviousWin>> CreateWinList(
const mojo::StructPtr<mojom::PreviousWin>& win1) {
std::vector<mojom::PreviousWinPtr> CreateWinList(
const mojom::PreviousWinPtr& win1,
const mojom::PreviousWinPtr& win2 = mojom::PreviousWinPtr(),
const mojom::PreviousWinPtr& win3 = mojom::PreviousWinPtr()) {
std::vector<mojo::StructPtr<mojom::PreviousWin>> out;
out.emplace_back(win1.Clone());
return out;
}
std::vector<mojo::StructPtr<mojom::PreviousWin>> CreateWinList(
const mojo::StructPtr<mojom::PreviousWin>& win1,
const mojo::StructPtr<mojom::PreviousWin>& win2) {
std::vector<mojo::StructPtr<mojom::PreviousWin>> out;
out.emplace_back(win1.Clone());
out.emplace_back(win2.Clone());
if (win2)
out.emplace_back(win2.Clone());
if (win3)
out.emplace_back(win3.Clone());
return out;
}
@ -922,11 +918,11 @@ TEST_F(BidderWorkletTest, GenerateBidPrevWins) {
"browserSignals.prevWins",
R"([[0,"future_ad"]])",
},
// Out of order times.
// Out of order wins should be sorted.
{
CreateWinList(future_win, win1),
CreateWinList(win2, future_win, win1),
"browserSignals.prevWins",
R"([[0,"future_ad"],[200,"ad1"]])",
R"([[200,"ad1"],[100,["ad2"]],[0,"future_ad"]])",
},
};