Skip to content

Commit

Permalink
Merge to M115 of "FLEDGE: Changed to dump instead of crashing the
Browse files Browse the repository at this point in the history
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 <shivanisha@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Reviewed-by: Russ Hamilton <behamilton@google.com>
> Commit-Queue: Xiaochen Zhou <xiaochenzh@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1155044}
(cherry picked from commit 21101bb)

Change-Id: I5d320c88955568b0ab9e418bd2dc9d57924c348b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4609310
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Krishna Govind <govind@chromium.org>
Reviewed-by: Krishna Govind <govind@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Owners-Override: Krishna Govind <govind@chromium.org>
Commit-Queue: Xiaochen Zhou <xiaochenzh@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#718}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
xiaochen-z authored and Chromium LUCI CQ committed Jun 13, 2023
1 parent f87ffe4 commit e58f85f
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 3 deletions.
4 changes: 3 additions & 1 deletion content/browser/fenced_frame/fenced_frame_url_mapping.h
Expand Up @@ -183,7 +183,7 @@ class CONTENT_EXPORT FencedFrameURLMapping {
const GURL& urn_uuid,
const std::vector<std::pair<std::string, std::string>>& substitutions);

Id unique_id() { return unique_id_; }
Id unique_id() { return id_for_testing_.value_or(unique_id_); }

private:
friend class FencedFrameURLMappingTestPeer;
Expand Down Expand Up @@ -216,6 +216,8 @@ class CONTENT_EXPORT FencedFrameURLMapping {
pending_urn_uuid_to_url_map_;

const Id unique_id_;

absl::optional<Id> id_for_testing_;
};

} // namespace content
Expand Down
15 changes: 13 additions & 2 deletions content/browser/interest_group/ad_auction_service_impl.cc
Expand Up @@ -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
Expand Down
69 changes: 69 additions & 0 deletions content/browser/interest_group/ad_auction_service_impl_unittest.cc
Expand Up @@ -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<blink::FencedFrame::RedactedFencedFrameConfig> 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<RenderFrameHostImpl*>(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:
Expand Down
8 changes: 8 additions & 0 deletions content/test/fenced_frame_test_utils.cc
Expand Up @@ -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();
Expand Down
9 changes: 9 additions & 0 deletions content/test/fenced_frame_test_utils.h
Expand Up @@ -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<FencedFrameURLMapping> fenced_frame_url_mapping_;
};
Expand Down

0 comments on commit e58f85f

Please sign in to comment.