From 9ca5e9809c72dbbdf358787d3bafd158c9c8b96c Mon Sep 17 00:00:00 2001 From: Caleb Raitto Date: Fri, 9 Jun 2023 18:17:54 +0000 Subject: [PATCH] [M115] Add prevWinsMs browser signal The time field in prevWins was reported in seconds, but the web standard convention is to use milliseconds instead of seconds. To maintain compatibility with existing API users, both prevWins and the new prevWinsMs will be provided -- support for the deprecated prevWins can be removed once API users have switched. **NOTE**: Test expectations in bidder_worklet_unittest.cc have been changed from the version of this CL in main since crrev.com/c/4566565 and crrev.com/c/4580612 were not cherry-picked for M115. (cherry picked from commit 88997c3e7332c667737a7d6c70cc16807ce261d4) Bug: 1451033 Change-Id: I67248f92030c91d6e1f76e283ba43d57e2f2e4a0 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4590273 Reviewed-by: Russ Hamilton Commit-Queue: Caleb Raitto Cr-Original-Commit-Position: refs/heads/main@{#1153994} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4605148 Auto-Submit: Caleb Raitto Commit-Queue: Russ Hamilton Cr-Commit-Position: refs/branch-heads/5790@{#544} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../interest_group/auction_runner_unittest.cc | 10 ++++ .../auction_worklet/bidder_lazy_filler.cc | 50 ++++++++++++++++--- .../auction_worklet/bidder_lazy_filler.h | 10 ++++ .../bidder_worklet_unittest.cc | 42 ++++++++++++++++ .../context_recycler_unittest.cc | 6 ++- .../bidding_argument_validator.js | 4 +- ...nent_auction_bidding_argument_validator.js | 4 +- 7 files changed, 114 insertions(+), 12 deletions(-) diff --git a/content/browser/interest_group/auction_runner_unittest.cc b/content/browser/interest_group/auction_runner_unittest.cc index 8b6f34d9259fb..2065cefa5348f 100644 --- a/content/browser/interest_group/auction_runner_unittest.cc +++ b/content/browser/interest_group/auction_runner_unittest.cc @@ -330,6 +330,16 @@ std::string MakeBidScript(const url::Origin& seller, if (browserSignals.prevWins[i][1].winner !== -i) throw new Error("prevWin MD not what passed in"); } + if (browserSignals.prevWinsMs.length !== 3) + throw new Error("prevWinsMs"); + for (let i = 0; i < browserSignals.prevWinsMs.length; ++i) { + if (!(browserSignals.prevWinsMs[i] instanceof Array)) + throw new Error("prevWinsMs entry not an array"); + if (typeof browserSignals.prevWinsMs[i][0] != "number") + throw new Error("Not a Number in prevWin?"); + if (browserSignals.prevWinsMs[i][1].winner !== -i) + throw new Error("prevWin MD not what passed in"); + } if (debugLossReportUrl) { if (reportPostAuctionSignals) debugLossReportUrl += postAuctionSignalsPlaceholder; diff --git a/content/services/auction_worklet/bidder_lazy_filler.cc b/content/services/auction_worklet/bidder_lazy_filler.cc index 3f2b56a1e2d68..9261005a6d9e3 100644 --- a/content/services/auction_worklet/bidder_lazy_filler.cc +++ b/content/services/auction_worklet/bidder_lazy_filler.cc @@ -21,6 +21,7 @@ namespace { // wins is only used for a single bid in a single auction, and its order is // unspecified, anyways. v8::MaybeLocal CreatePrevWinsArray( + PrevWinsType prev_wins_type, AuctionV8Helper* v8_helper, v8::Local context, base::Time auction_start_time, @@ -33,12 +34,24 @@ v8::MaybeLocal CreatePrevWinsArray( std::vector> 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(); + base::TimeDelta time_delta = auction_start_time - prev_win->time; + int time_delta_int; + switch (prev_wins_type) { + case PrevWinsType::kSeconds: + time_delta_int = time_delta.InSeconds(); + break; + case PrevWinsType::kMilliseconds: + // Truncate to the nearest second, rather than providing the result in + // millisecond granularity. + time_delta_int = time_delta.InSeconds() * 1000; + break; + } // Don't give negative times if clock has changed since last auction win. - if (time_delta < 0) - time_delta = 0; + if (time_delta_int < 0) { + time_delta_int = 0; + } v8::Local win_values[2]; - win_values[0] = v8::Number::New(isolate, time_delta); + win_values[0] = v8::Number::New(isolate, time_delta_int); if (!v8_helper->CreateValueFromJson(context, prev_win->ad_json) .ToLocal(&win_values[1])) { return v8::MaybeLocal(); @@ -169,8 +182,12 @@ void BiddingBrowserSignalsLazyFiller::ReInitialize( bool BiddingBrowserSignalsLazyFiller::FillInObject( v8::Local object) { - if (!DefineLazyAttribute(object, "prevWins", &HandlePrevWins)) + if (!DefineLazyAttribute(object, "prevWins", &HandlePrevWins)) { + return false; + } + if (!DefineLazyAttribute(object, "prevWinsMs", &HandlePrevWinsMs)) { return false; + } return true; } @@ -182,15 +199,32 @@ void BiddingBrowserSignalsLazyFiller::Reset() { void BiddingBrowserSignalsLazyFiller::HandlePrevWins( v8::Local name, const v8::PropertyCallbackInfo& info) { + HandlePrevWinsInternal(name, info, PrevWinsType::kSeconds); +} + +// static +void BiddingBrowserSignalsLazyFiller::HandlePrevWinsMs( + v8::Local name, + const v8::PropertyCallbackInfo& info) { + HandlePrevWinsInternal(name, info, PrevWinsType::kMilliseconds); +} + +// TODO(crbug.com/1451034): Clean up support for deprecated seconds-based +// version after API users migrate, and remove this indirection function. +// static +void BiddingBrowserSignalsLazyFiller::HandlePrevWinsInternal( + v8::Local name, + const v8::PropertyCallbackInfo& info, + PrevWinsType prev_wins_type) { BiddingBrowserSignalsLazyFiller* self = GetSelf(info); AuctionV8Helper* v8_helper = self->v8_helper(); v8::Isolate* isolate = v8_helper->isolate(); v8::Local value; if (self->bidder_browser_signals_ && - CreatePrevWinsArray(v8_helper, isolate->GetCurrentContext(), - self->auction_start_time_, - self->bidder_browser_signals_->prev_wins) + CreatePrevWinsArray( + prev_wins_type, v8_helper, isolate->GetCurrentContext(), + self->auction_start_time_, self->bidder_browser_signals_->prev_wins) .ToLocal(&value)) { SetResult(info, value); } else { diff --git a/content/services/auction_worklet/bidder_lazy_filler.h b/content/services/auction_worklet/bidder_lazy_filler.h index dba2328b86a28..59a586e14c384 100644 --- a/content/services/auction_worklet/bidder_lazy_filler.h +++ b/content/services/auction_worklet/bidder_lazy_filler.h @@ -40,6 +40,10 @@ class CONTENT_EXPORT InterestGroupLazyFiller : public LazyFiller { bidder_worklet_non_shared_params_ = nullptr; }; +// TODO(crbug.com/1451034): Clean up support for deprecated seconds-based +// version after API users migrate. +enum class PrevWinsType { kSeconds, kMilliseconds }; + class CONTENT_EXPORT BiddingBrowserSignalsLazyFiller : public LazyFiller { public: explicit BiddingBrowserSignalsLazyFiller(AuctionV8Helper* v8_helper); @@ -53,6 +57,12 @@ class CONTENT_EXPORT BiddingBrowserSignalsLazyFiller : public LazyFiller { private: static void HandlePrevWins(v8::Local name, const v8::PropertyCallbackInfo& info); + static void HandlePrevWinsMs(v8::Local name, + const v8::PropertyCallbackInfo& info); + static void HandlePrevWinsInternal( + v8::Local name, + const v8::PropertyCallbackInfo& info, + PrevWinsType prev_wins_type); raw_ptr bidder_browser_signals_ = nullptr; base::Time auction_start_time_; diff --git a/content/services/auction_worklet/bidder_worklet_unittest.cc b/content/services/auction_worklet/bidder_worklet_unittest.cc index 491f4a9e98ab6..142c456d54207 100644 --- a/content/services/auction_worklet/bidder_worklet_unittest.cc +++ b/content/services/auction_worklet/bidder_worklet_unittest.cc @@ -3751,6 +3751,48 @@ TEST_F(BidderWorkletTest, GenerateBidPrevWins) { "browserSignals.prevWins", R"([[200,"ad1"],[100,["ad2"]],[0,"future_ad"]])", }, + // Same as above, but for prevWinsMs. + { + {}, + "browserSignals.prevWinsMs", + "[]", + }, + { + CreateWinList(win1), + "browserSignals.prevWinsMs", + R"([[200000,"ad1"]])", + }, + // Make sure it's passed on as an object and not a string. + { + CreateWinList(win1), + "browserSignals.prevWinsMs[0]", + R"([200000,"ad1"])", + }, + // Test rounding. + { + CreateWinList(win2), + "browserSignals.prevWinsMs", + R"([[100000,["ad2"]]])", + }, + // Multiple previous wins. + { + CreateWinList(win1, win2), + "browserSignals.prevWinsMs", + R"([[200000,"ad1"],[100000,["ad2"]]])", + }, + // Times are trimmed at 0. + { + CreateWinList(future_win), + "browserSignals.prevWinsMs", + R"([[0,"future_ad"]])", + }, + // Out of order wins should be sorted. + { + CreateWinList(win2, future_win, win1), + "browserSignals.prevWinsMs", + R"([[200000,"ad1"],[100000,["ad2"]],[0,"future_ad"]])", + }, + }; for (auto& test_case : test_cases) { diff --git a/content/services/auction_worklet/context_recycler_unittest.cc b/content/services/auction_worklet/context_recycler_unittest.cc index 44913446b6789..3e1f5f048b958 100644 --- a/content/services/auction_worklet/context_recycler_unittest.cc +++ b/content/services/auction_worklet/context_recycler_unittest.cc @@ -860,7 +860,8 @@ TEST_F(ContextRecyclerTest, BidderLazyFiller) { "{\"userBiddingSignals\":{\"k\":2}," "\"trustedBiddingSignalsKeys\":[\"c\",\"d\"]," "\"priorityVector\":{\"e\":12}," - "\"prevWins\":[[240,[\"d\"]],[180,[\"c\"]]]}", + "\"prevWins\":[[240,[\"d\"]],[180,[\"c\"]]]," + "\"prevWinsMs\":[[240000,[\"d\"]],[180000,[\"c\"]]]}", str_result); } } @@ -953,7 +954,8 @@ TEST_F(ContextRecyclerTest, BidderLazyFiller2) { "{\"userBiddingSignals\":null," "\"trustedBiddingSignalsKeys\":null," "\"priorityVector\":null," - "\"prevWins\":[]}", + "\"prevWins\":[]," + "\"prevWinsMs\":[]}", str_result); } } diff --git a/content/test/data/interest_group/bidding_argument_validator.js b/content/test/data/interest_group/bidding_argument_validator.js index 6728742eabce0..b41255fdfe2ee 100644 --- a/content/test/data/interest_group/bidding_argument_validator.js +++ b/content/test/data/interest_group/bidding_argument_validator.js @@ -198,7 +198,7 @@ function validateBrowserSignals(browserSignals, isGenerateBid) { throw 'Wrong topLevelSeller ' + browserSignals.topLevelSeller; if (isGenerateBid) { - if (Object.keys(browserSignals).length !== 5) { + if (Object.keys(browserSignals).length !== 6) { throw 'Wrong number of browser signals fields ' + JSON.stringify(browserSignals); } @@ -208,6 +208,8 @@ function validateBrowserSignals(browserSignals, isGenerateBid) { throw 'Wrong bidCount ' + bidCount; if (browserSignals.prevWins.length !== 0) throw 'Wrong prevWins ' + JSON.stringify(browserSignals.prevWins); + if (browserSignals.prevWinsMs.length !== 0) + throw 'Wrong prevWinsMs ' + JSON.stringify(browserSignals.prevWinsMs); } else { if (Object.keys(browserSignals).length !== 15) { throw 'Wrong number of browser signals fields ' + diff --git a/content/test/data/interest_group/component_auction_bidding_argument_validator.js b/content/test/data/interest_group/component_auction_bidding_argument_validator.js index 40d79396817e3..fcf9e7580279a 100644 --- a/content/test/data/interest_group/component_auction_bidding_argument_validator.js +++ b/content/test/data/interest_group/component_auction_bidding_argument_validator.js @@ -180,7 +180,7 @@ function validateBrowserSignals(browserSignals, isGenerateBid) { throw 'Wrong topLevelSeller ' + browserSignals.topLevelSeller; if (isGenerateBid) { - if (Object.keys(browserSignals).length !== 6) { + if (Object.keys(browserSignals).length !== 7) { throw 'Wrong number of browser signals fields ' + JSON.stringify(browserSignals); } @@ -190,6 +190,8 @@ function validateBrowserSignals(browserSignals, isGenerateBid) { throw 'Wrong bidCount ' + browserSignals.bidCount; if (browserSignals.prevWins.length !== 0) throw 'Wrong prevWins ' + JSON.stringify(browserSignals.prevWins); + if (browserSignals.prevWinsMs.length !== 0) + throw 'Wrong prevWinsMs ' + JSON.stringify(browserSignals.prevWinsMs); } else { if (Object.keys(browserSignals).length !== 16) { throw 'Wrong number of browser signals fields ' +