Skip to content

Commit

Permalink
FLEDGE: Changed to dump instead of crashing the browser process.
Browse files Browse the repository at this point in the history
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: 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}
  • Loading branch information
xiaochen-z authored and Chromium LUCI CQ committed Jun 8, 2023
1 parent 20ce2af commit 21101bb
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -617,8 +617,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
Original file line number Diff line number Diff line change
Expand Up @@ -6486,6 +6486,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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,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 21101bb

Please sign in to comment.