From e58f85fb10076a3f472ea1235ba16471a24959db Mon Sep 17 00:00:00 2001 From: Xiaochen Zhou Date: Tue, 13 Jun 2023 22:01:24 +0000 Subject: [PATCH] Merge to M115 of "FLEDGE: Changed to dump instead of crashing the browser process." This CL is to merge ( crrev.com/c/4595259 ) back to M115. It prevents a browser crash happening in ad auction. > FLEDGE: Changed to dump instead of crashing the browser process. > Changed to dump instead of crashing the browser when fenced frame url > mapping mismatch between the beginning of the auction and the > completion of the auction. > There has been reported crash due to the CHECK on the fenced frame url > map that is used at the beginning of the auction and at the completion > of the auction. > The assumption is that these two maps should be the same. If it does > not hold, the browser crashes. > Despite efforts to reproduce this crash, we still cannot figure out > the root cause. For now, change this to dumping to avoid crashing the > browser. > Bug: 1422301 > Change-Id (omit this): I5d320c88955568b0ab9e418bd2dc9d57924c348b > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4595259 > Reviewed-by: Shivani Sharma > Reviewed-by: Alex Moshchuk > Reviewed-by: Russ Hamilton > Commit-Queue: Xiaochen Zhou > Cr-Commit-Position: refs/heads/main@{#1155044} (cherry picked from commit 21101bbcd31493d1cdcccf2f3096f9002da52a04) Change-Id: I5d320c88955568b0ab9e418bd2dc9d57924c348b Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4609310 Bot-Commit: Rubber Stamper Commit-Queue: Krishna Govind Reviewed-by: Krishna Govind Reviewed-by: Alex Moshchuk Owners-Override: Krishna Govind Commit-Queue: Xiaochen Zhou Cr-Commit-Position: refs/branch-heads/5790@{#718} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../fenced_frame/fenced_frame_url_mapping.h | 4 +- .../interest_group/ad_auction_service_impl.cc | 15 +++- .../ad_auction_service_impl_unittest.cc | 69 +++++++++++++++++++ content/test/fenced_frame_test_utils.cc | 8 +++ content/test/fenced_frame_test_utils.h | 9 +++ 5 files changed, 102 insertions(+), 3 deletions(-) diff --git a/content/browser/fenced_frame/fenced_frame_url_mapping.h b/content/browser/fenced_frame/fenced_frame_url_mapping.h index dc7e2b4d4b0ad..4182a369c4f6f 100644 --- a/content/browser/fenced_frame/fenced_frame_url_mapping.h +++ b/content/browser/fenced_frame/fenced_frame_url_mapping.h @@ -183,7 +183,7 @@ class CONTENT_EXPORT FencedFrameURLMapping { const GURL& urn_uuid, const std::vector>& substitutions); - Id unique_id() { return unique_id_; } + Id unique_id() { return id_for_testing_.value_or(unique_id_); } private: friend class FencedFrameURLMappingTestPeer; @@ -216,6 +216,8 @@ class CONTENT_EXPORT FencedFrameURLMapping { pending_urn_uuid_to_url_map_; const Id unique_id_; + + absl::optional id_for_testing_; }; } // namespace content diff --git a/content/browser/interest_group/ad_auction_service_impl.cc b/content/browser/interest_group/ad_auction_service_impl.cc index bdbf163aecfdd..f9779f139c314 100644 --- a/content/browser/interest_group/ad_auction_service_impl.cc +++ b/content/browser/interest_group/ad_auction_service_impl.cc @@ -594,8 +594,19 @@ void AdAuctionServiceImpl::OnAuctionComplete( winning_group_key->name}; FencedFrameURLMapping& current_fenced_frame_urls_map = GetFrame()->GetPage().fenced_frame_urls_map(); - // The auction must operate on the same fenced frame mapping. - CHECK_EQ(fenced_frame_urls_map_id, current_fenced_frame_urls_map.unique_id()); + // TODO(crbug.com/1422301): The auction must operate on the same fenced frame + // mapping that was used at the beginning of the auction. If not, we fail the + // auction and dump without crashing the browser. Once the root cause is known + // and the issue fixed, convert it back to a CHECK. + if (fenced_frame_urls_map_id != current_fenced_frame_urls_map.unique_id()) { + base::debug::DumpWithoutCrashing(); + if (auction_result_metrics) { + auction_result_metrics->ReportAuctionResult( + AdAuctionResultMetrics::AuctionResult::kFailed); + } + std::move(callback).Run(manually_aborted, /*config=*/absl::nullopt); + return; + } // Set up reporting for any fenced frame that's navigated to the winning bid's // URL. Use a URLLoaderFactory that will automatically reconnect on network diff --git a/content/browser/interest_group/ad_auction_service_impl_unittest.cc b/content/browser/interest_group/ad_auction_service_impl_unittest.cc index 34eb7a6d28995..a38863648928e 100644 --- a/content/browser/interest_group/ad_auction_service_impl_unittest.cc +++ b/content/browser/interest_group/ad_auction_service_impl_unittest.cc @@ -6463,6 +6463,75 @@ function reportResult(auctionConfig, browserSignals) { EXPECT_TRUE(network_responder_->ReportSent("/seller_debug_win_2")); } +// TODO(crbug.com/1422301): The auction must operate on the same fenced frame +// mapping that was used at the beginning of the auction. If not, we fail the +// auction and dump without crashing the browser. Once the root cause is known +// and the issue fixed, remove this test. +TEST_F(AdAuctionServiceImplTest, FencedFrameUrlMappingChangedDuringAuction) { + network_responder_->RegisterDeferredScriptResponse(kBiddingUrlPath); + network_responder_->RegisterScriptResponse(kDecisionUrlPath, + BasicSellerReportScript()); + + blink::InterestGroup interest_group = CreateInterestGroup(); + interest_group.bidding_url = kUrlA.Resolve(kBiddingUrlPath); + interest_group.ads.emplace(); + blink::InterestGroup::Ad ad( + /*render_url=*/GURL("https://example.com/render"), + /*metadata=*/absl::nullopt); + interest_group.ads->emplace_back(std::move(ad)); + + JoinInterestGroupAndFlush(interest_group); + EXPECT_EQ(1, GetJoinCount(kOriginA, kInterestGroupName)); + + blink::AuctionConfig auction_config; + auction_config.seller = kOriginA; + auction_config.decision_logic_url = kUrlA.Resolve(kDecisionUrlPath); + auction_config.non_shared_params.interest_group_buyers = {kOriginA}; + + AdAuctionServiceImpl::CreateMojoService( + main_rfh(), ad_auction_service_.BindNewPipeAndPassReceiver()); + + // Start the auction. + base::RunLoop run_loop; + absl::optional maybe_config; + ad_auction_service_->RunAdAuction( + auction_config, mojo::NullReceiver(), + base::BindLambdaForTesting( + [&run_loop, &maybe_config]( + bool manually_aborted, + const absl::optional< + blink::FencedFrame::RedactedFencedFrameConfig>& config) { + EXPECT_FALSE(manually_aborted); + maybe_config = config; + run_loop.Quit(); + })); + + // Wait for the NetworkResponder to see the request for the bidding URL, then + // respond. + task_environment()->RunUntilIdle(); + EXPECT_FALSE(run_loop.AnyQuitCalled()); + network_responder_->DoDeferredScriptResponse(kBiddingUrlPath, + BasicBiddingReportScript()); + + FencedFrameURLMapping& fenced_frame_urls_map = + static_cast(main_rfh()) + ->GetPage() + .fenced_frame_urls_map(); + + FencedFrameURLMappingTestPeer test_peer(&fenced_frame_urls_map); + + // Change the id of the fenced frame url mapping used by the current auction + // to simulate the scenario we've seen in crbug.com/1422301: the fenced frame + // url mapping used at the beginning of the auction is not the same as the one + // at the end of the auction. + test_peer.SetId(test_peer.GetNextId()); + + // Complete the auction. It should fail due to the fenced frame url mapping + // mismatch. + run_loop.Run(); + EXPECT_FALSE(maybe_config.has_value()); +} + class AdAuctionServiceImplSharedStorageEnabledTest : public AdAuctionServiceImplTest { public: diff --git a/content/test/fenced_frame_test_utils.cc b/content/test/fenced_frame_test_utils.cc index d358fddbe49fc..bbc95561eb552 100644 --- a/content/test/fenced_frame_test_utils.cc +++ b/content/test/fenced_frame_test_utils.cc @@ -97,6 +97,14 @@ void FencedFrameURLMappingTestPeer::FillMap(const GURL& url) { DCHECK(fenced_frame_url_mapping_->IsFull()); } +void FencedFrameURLMappingTestPeer::SetId(FencedFrameURLMapping::Id id) { + fenced_frame_url_mapping_->id_for_testing_ = id; +} + +FencedFrameURLMapping::Id FencedFrameURLMappingTestPeer::GetNextId() const { + return FencedFrameURLMapping::GetNextId(); +} + bool PollUntilEvalToTrue(const std::string& script, RenderFrameHost* rfh) { base::Time start_time = base::Time::Now(); base::TimeDelta timeout = TestTimeouts::action_max_timeout(); diff --git a/content/test/fenced_frame_test_utils.h b/content/test/fenced_frame_test_utils.h index 38ffdfc794505..d582785aa4225 100644 --- a/content/test/fenced_frame_test_utils.h +++ b/content/test/fenced_frame_test_utils.h @@ -115,6 +115,15 @@ class FencedFrameURLMappingTestPeer { // Insert urn mappings until it reaches the limit. void FillMap(const GURL& url); + // TODO(crbug.com/1422301): This method allows setting of an arbitrary id of + // the fenced frame mapping. It is used to test that the auction fails if + // there is a mismatch between the fenced frame mapping used at the beginning + // of the auction and at the end of the auction. Once the root cause is known + // and the issue fixed, remove `SetId()` and `GetNextId()`. + void SetId(FencedFrameURLMapping::Id id); + + FencedFrameURLMapping::Id GetNextId() const; + private: raw_ptr fenced_frame_url_mapping_; };