[FLEDGE] BidderWorklet reuse CL 3: Rework BidderWorklet load errors.

This CL makes BidderWorklet load errors behave like seller worklet load
errors: On error, pending callbacks are not invoked. Instead, the Mojo
pipe is reset with an error message.

Bug: 1276639
Change-Id: I746f2a3718a4f5087e76a3b6292dd78bc2744104
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3365661
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/main@{#957372}

content/browser/interest_group/auction_runner.cc

@@ -303,7 +303,7 @@ void AuctionRunner::OnBidderWorkletProcessReceived(BidState* bid_state) {
   bid_state->state = BidState::State::kGeneratingBid;
   LoadBidderWorklet(*bid_state,
                     /*disconnect_handler=*/base::BindOnce(
-                        &AuctionRunner::OnGenerateBidCrashed,
+                        &AuctionRunner::OnBidderWorkletDisconnected,
                         base::Unretained(this), bid_state));
   const blink::InterestGroup& interest_group = bid_state->bidder.interest_group;
   bid_state->bidder_worklet->GenerateBid(
@@ -318,11 +318,20 @@ void AuctionRunner::OnBidderWorkletProcessReceived(BidState* bid_state) {
                      base::Unretained(this), bid_state));
 }
 
-void AuctionRunner::OnGenerateBidCrashed(BidState* state) {
+void AuctionRunner::OnBidderWorkletDisconnected(
+    BidState* state,
+    uint32_t custom_reason,
+    const std::string& description) {
+  std::vector<std::string> errors;
+  if (description.empty()) {
+    errors.emplace_back(
+        base::StrCat({state->bidder.interest_group.bidding_url->spec(),
+                      " crashed while trying to run generateBid()."}));
+  } else {
+    errors.push_back(description);
+  }
   OnGenerateBidComplete(state, auction_worklet::mojom::BidderWorkletBidPtr(),
-                        std::vector<std::string>{base::StrCat(
-                            {state->bidder.interest_group.bidding_url->spec(),
-                             " crashed while trying to run generateBid()."})});
+                        errors);
 }
 
 void AuctionRunner::OnGenerateBidComplete(
@@ -563,12 +572,9 @@ void AuctionRunner::ReportBidWin(
 
   // Load the script for the top-scoring bidder worklet again, and invoke its
   // ReportWin() method.
-  LoadBidderWorklet(
-      *top_bidder_, /*disconnect_handler=*/base::BindOnce(
-          &AuctionRunner::FailAuction, base::Unretained(this),
-          AuctionResult::kWinningBidderWorkletCrashed,
-          base::StrCat({top_bidder_->bidder.interest_group.bidding_url->spec(),
-                        " crashed while trying to run reportWin()."})));
+  LoadBidderWorklet(*top_bidder_, /*disconnect_handler=*/base::BindOnce(
+                        &AuctionRunner::WinningBidderWorkletDisconnected,
+                        base::Unretained(this)));
   top_bidder_->bidder_worklet->ReportWin(
       top_bidder_->bidder.interest_group.name,
       auction_config_->shareable_auction_ad_config->auction_signals,
@@ -593,6 +599,24 @@ void AuctionRunner::OnReportBidWinComplete(
   ReportSuccess();
 }
 
+void AuctionRunner::WinningBidderWorkletDisconnected(
+    uint32_t custom_reason,
+    const std::string& description) {
+  std::vector<std::string> errors;
+  if (description.empty()) {
+    // Empty `description` means the process crashed.
+    FailAuction(
+        AuctionResult::kWinningBidderWorkletCrashed,
+        {base::StrCat({top_bidder_->bidder.interest_group.bidding_url->spec(),
+                       " crashed while trying to run reportWin()."})});
+  } else {
+    // Non-empty `description` means there was a different error (e.g., script
+    // failed to load). This currently does not fail the auction.
+    errors_.push_back(description);
+    ReportSuccess();
+  }
+}
+
 void AuctionRunner::ReportSuccess() {
   DCHECK(callback_);
   DCHECK(top_bidder_->bid_result);
@@ -661,8 +685,9 @@ void AuctionRunner::RecordResult(AuctionResult result) const {
   }
 }
 
-void AuctionRunner::LoadBidderWorklet(BidState& bid_state,
-                                      base::OnceClosure disconnect_handler) {
+void AuctionRunner::LoadBidderWorklet(
+    BidState& bid_state,
+    mojo::ConnectionErrorWithReasonCallback disconnect_handler) {
   const blink::InterestGroup& interest_group = bid_state.bidder.interest_group;
 
   mojo::PendingRemote<network::mojom::URLLoaderFactory> url_loader_factory;
@@ -691,7 +716,7 @@ void AuctionRunner::LoadBidderWorklet(BidState& bid_state,
       interest_group.bidding_wasm_helper_url,
       interest_group.trusted_bidding_signals_url,
       browser_signals_->top_frame_origin);
-  bid_state.bidder_worklet.set_disconnect_handler(
+  bid_state.bidder_worklet.set_disconnect_with_reason_handler(
       std::move(disconnect_handler));
 }
 

content/browser/interest_group/auction_runner.h

@@ -21,6 +21,7 @@
 #include "content/services/auction_worklet/public/mojom/auction_worklet_service.mojom.h"
 #include "content/services/auction_worklet/public/mojom/bidder_worklet.mojom.h"
 #include "content/services/auction_worklet/public/mojom/seller_worklet.mojom.h"
+#include "mojo/public/cpp/bindings/connection_error_callback.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "services/network/public/mojom/client_security_state.mojom-forward.h"
 #include "services/network/public/mojom/url_loader_factory.mojom-forward.h"
@@ -266,7 +267,9 @@ class CONTENT_EXPORT AuctionRunner {
   // bid.
   void OnBidderWorkletProcessReceived(BidState* bid_state);
 
-  void OnGenerateBidCrashed(BidState* state);
+  void OnBidderWorkletDisconnected(BidState* state,
+                                   uint32_t custom_reason,
+                                   const std::string& description);
   void OnGenerateBidComplete(BidState* state,
                              auction_worklet::mojom::BidderWorkletBidPtr bid,
                              const std::vector<std::string>& errors);
@@ -305,6 +308,11 @@ class CONTENT_EXPORT AuctionRunner {
   void OnReportBidWinComplete(const absl::optional<GURL>& bidder_report_url,
                               const std::vector<std::string>& error_msgs);
 
+  // Called when the BidderWorklet that won an auction is disconnected during
+  // the ReportWin() call.
+  void WinningBidderWorkletDisconnected(uint32_t custom_reason,
+                                        const std::string& description);
+
   // Completes the auction, invoking `callback_` and preventing any future
   // calls into `this` by closing mojo pipes and disposing of weak pointers. The
   // owner must be able to safely delete `this` when the callback is invoked.
@@ -319,8 +327,9 @@ class CONTENT_EXPORT AuctionRunner {
 
   // Loads a bidder worklet for `bid_state`, setting the provided disconnect
   // handler.
-  void LoadBidderWorklet(BidState& bid_state,
-                         base::OnceClosure disconnect_handler);
+  void LoadBidderWorklet(
+      BidState& bid_state,
+      mojo::ConnectionErrorWithReasonCallback disconnect_handler);
 
   const raw_ptr<Delegate> delegate_;
   const raw_ptr<InterestGroupManager> interest_group_manager_;

content/services/auction_worklet/auction_worklet_service_impl.cc

@@ -35,12 +35,18 @@ void AuctionWorkletServiceImpl::LoadBidderWorklet(
     const absl::optional<GURL>& wasm_helper_url,
     const absl::optional<GURL>& trusted_bidding_signals_url,
     const url::Origin& top_window_origin) {
-  bidder_worklets_.Add(
-      std::make_unique<BidderWorklet>(
-          auction_v8_helper_, pause_for_debugger_on_start,
-          std::move(pending_url_loader_factory), script_source_url,
-          wasm_helper_url, trusted_bidding_signals_url, top_window_origin),
-      std::move(bidder_worklet_receiver));
+  auto bidder_worklet = std::make_unique<BidderWorklet>(
+      auction_v8_helper_, pause_for_debugger_on_start,
+      std::move(pending_url_loader_factory), script_source_url, wasm_helper_url,
+      trusted_bidding_signals_url, top_window_origin);
+  auto* bidder_worklet_ptr = bidder_worklet.get();
+
+  mojo::ReceiverId receiver_id = bidder_worklets_.Add(
+      std::move(bidder_worklet), std::move(bidder_worklet_receiver));
+
+  bidder_worklet_ptr->set_close_pipe_callback(
+      base::BindOnce(&AuctionWorkletServiceImpl::DisconnectBidderWorklet,
+                     base::Unretained(this), receiver_id));
 }
 
 void AuctionWorkletServiceImpl::LoadSellerWorklet(
@@ -61,4 +67,11 @@ void AuctionWorkletServiceImpl::LoadSellerWorklet(
       std::move(seller_worklet_receiver));
 }
 
+void AuctionWorkletServiceImpl::DisconnectBidderWorklet(
+    mojo::ReceiverId receiver_id,
+    const std::string& reason) {
+  bidder_worklets_.RemoveWithReason(receiver_id, /*custom_reason_code=*/0,
+                                    reason);
+}
+
 }  // namespace auction_worklet

content/services/auction_worklet/auction_worklet_service_impl.h

@@ -62,6 +62,9 @@ class AuctionWorkletServiceImpl : public mojom::AuctionWorkletService {
       LoadSellerWorkletCallback load_seller_worklet_callback) override;
 
  private:
+  void DisconnectBidderWorklet(mojo::ReceiverId receiver_id,
+                               const std::string& reason);
+
   mojo::Receiver<mojom::AuctionWorkletService> receiver_;
 
   scoped_refptr<AuctionV8Helper> auction_v8_helper_;

content/services/auction_worklet/bidder_worklet.cc

@@ -179,13 +179,6 @@ BidderWorklet::BidderWorklet(
 
 BidderWorklet::~BidderWorklet() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(user_sequence_checker_);
-  // Invoke any pending callbacks, since destroying uninvoked Mojo callbacks can
-  // sometimes cause issues if the pipe they're over is still open. This is not
-  // strictly necessary here, since the BidderWorklet's received pipe is
-  // destroyed before the BidderWorklet itself is, but makes the class safer
-  // against refactors of the Mojo API.
-  FailAllPendingTasks();
-
   debug_id_->AbortDebuggerPauses();
 }
 
@@ -215,15 +208,6 @@ void BidderWorklet::GenerateBid(
   generate_bid_task->auction_start_time = auction_start_time;
   generate_bid_task->callback = std::move(generate_bid_callback);
 
-  // If worklet script failed to load, or WASM was requested and failed, fail
-  // and exit early.
-  if (CodeLoadState() == LoadState::kFailure) {
-    DeliverBidCallbackOnUserThread(generate_bid_task,
-                                   mojom::BidderWorkletBidPtr(),
-                                   /*error_msgs=*/std::vector<std::string>());
-    return;
-  }
-
   const auto& trusted_bidding_signals_keys =
       generate_bid_task->bidder_worklet_non_shared_params
           ->trusted_bidding_signals_keys;
@@ -267,18 +251,9 @@ void BidderWorklet::ReportWin(
   report_win_task->browser_signal_seller_origin = browser_signal_seller_origin;
   report_win_task->callback = std::move(report_win_callback);
 
-  // If worklet script and, if requested, WASM, aren't loaded, can't run script
-  // immediately.
-  LoadState code_load_state = CodeLoadState();
-  if (code_load_state != LoadState::kSuccess) {
-    // If required files failed to load, fail and exit early.
-    if (code_load_state == LoadState::kFailure) {
-      DeliverReportWinOnUserThread(report_win_task,
-                                   /*report_url=*/absl::optional<GURL>(),
-                                   /*errors=*/std::vector<std::string>());
-    }
+  // If not yet ready, need to wait for load to complete.
+  if (!IsCodeReady())
     return;
-  }
 
   RunReportWin(report_win_task);
 }
@@ -724,9 +699,6 @@ void BidderWorklet::Start() {
         debug_id_,
         base::BindOnce(&BidderWorklet::OnWasmDownloaded,
                        base::Unretained(this)));
-  } else {
-    // WASM helper is optional if not specified.
-    worklet_wasm_load_state_ = LoadState::kSuccess;
   }
 }
 
@@ -736,16 +708,18 @@ void BidderWorklet::OnScriptDownloaded(WorkletLoader::Result worklet_script,
 
   worklet_loader_.reset();
 
-  // Fail all pending tasks if the script failed to load.
+  // On failure, close pipe and delete `this`, as it can't do anything without a
+  // loaded script.
   if (!worklet_script.success()) {
-    worklet_js_load_state_ = LoadState::kFailure;
-    if (error_msg.has_value())
-      load_code_error_msgs_.push_back(std::move(error_msg.value()));
-    FailAllPendingTasks();
+    std::move(close_pipe_callback_)
+        .Run(error_msg ? error_msg.value() : std::string());
+    // `this` should be deleted at this point.
     return;
   }
 
-  worklet_js_load_state_ = LoadState::kSuccess;
+  if (error_msg.has_value())
+    load_code_error_msgs_.push_back(std::move(error_msg.value()));
+
   v8_runner_->PostTask(FROM_HERE,
                        base::BindOnce(&BidderWorklet::V8State::SetWorkletScript,
                                       base::Unretained(v8_state_.get()),
@@ -760,16 +734,19 @@ void BidderWorklet::OnWasmDownloaded(WorkletWasmLoader::Result wasm_helper,
 
   wasm_loader_.reset();
 
-  // If the WASM helper is actually requested, its failure fails the bid.
+  // If the WASM helper is actually requested, delete `this` and inform the
+  // browser process of the failure. ReportWin() calls would theoretically still
+  // be allowed, but that adds a lot more complexity around BidderWorklet reuse.
   if (!wasm_helper.success()) {
-    worklet_wasm_load_state_ = LoadState::kFailure;
-    if (error_msg.has_value())
-      load_code_error_msgs_.push_back(std::move(error_msg.value()));
-    FailAllPendingTasks();
+    std::move(close_pipe_callback_)
+        .Run(error_msg ? error_msg.value() : std::string());
+    // `this` should be deleted at this point.
     return;
   }
 
-  worklet_wasm_load_state_ = LoadState::kSuccess;
+  if (error_msg.has_value())
+    load_code_error_msgs_.push_back(std::move(error_msg.value()));
+
   v8_runner_->PostTask(FROM_HERE,
                        base::BindOnce(&BidderWorklet::V8State::SetWasmHelper,
                                       base::Unretained(v8_state_.get()),
@@ -811,12 +788,8 @@ void BidderWorklet::OnTrustedBiddingSignalsDownloaded(
 
 void BidderWorklet::GenerateBidIfReady(GenerateBidTaskList::iterator task) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(user_sequence_checker_);
-  // Script or WASM load failure should abort all tasks before getting here.
-  LoadState code_load_state = CodeLoadState();
-  DCHECK_NE(code_load_state, LoadState::kFailure);
-  if (task->trusted_bidding_signals || code_load_state != LoadState::kSuccess) {
+  if (task->trusted_bidding_signals || !IsCodeReady())
     return;
-  }
 
   // Other than the callback field, no fields of `task` are needed after this
   // point, so can consume them instead of copying them.
@@ -855,20 +828,6 @@ void BidderWorklet::RunReportWin(ReportWinTaskList::iterator task) {
                          weak_ptr_factory_.GetWeakPtr(), task)));
 }
 
-void BidderWorklet::FailAllPendingTasks() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(user_sequence_checker_);
-  while (!generate_bid_tasks_.empty()) {
-    DeliverBidCallbackOnUserThread(generate_bid_tasks_.begin(),
-                                   mojom::BidderWorkletBidPtr(),
-                                   /*error_msgs=*/std::vector<std::string>());
-  }
-  while (!report_win_tasks_.empty()) {
-    DeliverReportWinOnUserThread(report_win_tasks_.begin(),
-                                 /*report_url=*/absl::nullopt,
-                                 /*errors=*/std::vector<std::string>());
-  }
-}
-
 void BidderWorklet::DeliverBidCallbackOnUserThread(
     GenerateBidTaskList::iterator task,
     mojom::BidderWorkletBidPtr bid,
@@ -896,19 +855,11 @@ void BidderWorklet::DeliverReportWinOnUserThread(
   report_win_tasks_.erase(task);
 }
 
-BidderWorklet::LoadState BidderWorklet::CodeLoadState() const {
-  if (worklet_js_load_state_ == LoadState::kFailure ||
-      worklet_wasm_load_state_ == LoadState::kFailure) {
-    return LoadState::kFailure;
-  }
-
-  if (worklet_js_load_state_ == LoadState::kLoading ||
-      worklet_wasm_load_state_ == LoadState::kLoading) {
-    return LoadState::kLoading;
-  }
-  DCHECK_EQ(worklet_js_load_state_, LoadState::kSuccess);
-  DCHECK_EQ(worklet_wasm_load_state_, LoadState::kSuccess);
-  return LoadState::kSuccess;
+bool BidderWorklet::IsCodeReady() const {
+  // If `paused_`, loading hasn't started yet. Otherwise, null loaders indicate
+  // the worklet script has loaded successfully, and there's no WASM helper, or
+  // it has also loaded successfully.
+  return !paused_ && !worklet_loader_ && !wasm_loader_;
 }
 
 }  // namespace auction_worklet

content/services/auction_worklet/bidder_worklet.h

@@ -53,6 +53,11 @@ namespace auction_worklet {
 // interest group in different auctions.
 class BidderWorklet : public mojom::BidderWorklet {
  public:
+  // Deletes the worklet immediately and resets the BidderWorklet's Mojo pipe
+  // with the provided description. See mojo::Receiver::ResetWithReason().
+  using ClosePipeCallback =
+      base::OnceCallback<void(const std::string& description)>;
+
   // Starts loading the worklet script on construction, as well as the trusted
   // bidding data, if necessary. Will then call the script's generateBid()
   // function and invoke the callback with the results. Callback will always be
@@ -72,6 +77,15 @@ class BidderWorklet : public mojom::BidderWorklet {
   ~BidderWorklet() override;
   BidderWorklet& operator=(const BidderWorklet&) = delete;
 
+  // Sets the callback to be invoked on errors which require closing the pipe.
+  // Callback will also immediately delete `this`. Not an argument to
+  // constructor because the Mojo ReceiverId needs to be bound to the callback,
+  // but can only get that after creating the worklet. Must be called
+  // immediately after creating a BidderWorklet.
+  void set_close_pipe_callback(ClosePipeCallback close_pipe_callback) {
+    close_pipe_callback_ = std::move(close_pipe_callback);
+  }
+
   int context_group_id_for_testing() const;
 
   // mojom::BidderWorklet implementation:
@@ -220,8 +234,6 @@ class BidderWorklet : public mojom::BidderWorklet {
     SEQUENCE_CHECKER(v8_sequence_checker_);
   };
 
-  enum class LoadState { kLoading, kSuccess, kFailure };
-
   void ResumeIfPaused();
   void Start();
 
@@ -246,10 +258,6 @@ class BidderWorklet : public mojom::BidderWorklet {
 
   void RunReportWin(ReportWinTaskList::iterator task);
 
-  // Fails all pending GenerateBid() and ReportWin() tasks, removing all tasks
-  // from both lists.
-  void FailAllPendingTasks();
-
   // Invokes the `callback` of `task` with the provided values, and removes
   // `task` from `generate_bid_tasks_`.
   void DeliverBidCallbackOnUserThread(GenerateBidTaskList::iterator task,
@@ -262,8 +270,9 @@ class BidderWorklet : public mojom::BidderWorklet {
                                     absl::optional<GURL> report_url,
                                     std::vector<std::string> errors);
 
-  // Combines `worklet_js_load_state_` and `worklet_wasm_load_state_`.
-  LoadState CodeLoadState() const;
+  // Returns true if unpaused and the script and WASM helper (if needed) have
+  // loaded.
+  bool IsCodeReady() const;
 
   scoped_refptr<base::SequencedTaskRunner> v8_runner_;
 
@@ -282,11 +291,9 @@ class BidderWorklet : public mojom::BidderWorklet {
   // Top window origin for the auctions sharing this BidderWorklet.
   const url::Origin top_window_origin_;
 
+  // These are deleted once each resource is loaded.
   std::unique_ptr<WorkletLoader> worklet_loader_;
-  LoadState worklet_js_load_state_ = LoadState::kLoading;
-
   std::unique_ptr<WorkletWasmLoader> wasm_loader_;
-  LoadState worklet_wasm_load_state_ = LoadState::kLoading;
 
   // Lives on `v8_runner_`. Since it's deleted there via DeleteSoon, tasks can
   // be safely posted from main thread to it with an Unretained pointer.
@@ -298,6 +305,8 @@ class BidderWorklet : public mojom::BidderWorklet {
   GenerateBidTaskList generate_bid_tasks_;
   ReportWinTaskList report_win_tasks_;
 
+  ClosePipeCallback close_pipe_callback_;
+
   // Errors that occurred while loading the code, if any.
   std::vector<std::string> load_code_error_msgs_;
 

content/services/auction_worklet/bidder_worklet_unittest.cc

@@ -276,12 +276,19 @@ class BidderWorkletTest : public testing::Test {
         url.is_empty() ? interest_group_bidding_url_ : url,
         interest_group_wasm_url_, interest_group_trusted_bidding_signals_url_,
         top_window_origin_);
-    if (out_bidder_worklet_impl)
-      *out_bidder_worklet_impl = bidder_worklet_impl.get();
-
+    auto* bidder_worklet_ptr = bidder_worklet_impl.get();
     mojo::Remote<mojom::BidderWorklet> bidder_worklet;
-    mojo::MakeSelfOwnedReceiver(std::move(bidder_worklet_impl),
-                                bidder_worklet.BindNewPipeAndPassReceiver());
+    mojo::ReceiverId receiver_id =
+        bidder_worklets_.Add(std::move(bidder_worklet_impl),
+                             bidder_worklet.BindNewPipeAndPassReceiver());
+    bidder_worklet_ptr->set_close_pipe_callback(
+        base::BindOnce(&BidderWorkletTest::ClosePipeCallback,
+                       base::Unretained(this), receiver_id));
+    bidder_worklet.set_disconnect_with_reason_handler(base::BindRepeating(
+        &BidderWorkletTest::OnDisconnectWithReason, base::Unretained(this)));
+
+    if (out_bidder_worklet_impl)
+      *out_bidder_worklet_impl = bidder_worklet_ptr;
     return bidder_worklet;
   }
 
@@ -294,6 +301,19 @@ class BidderWorkletTest : public testing::Test {
                        base::Unretained(this)));
   }
 
+  // Calls GenerateBid(), expecting the callback never to be invoked.
+  void GenerateBidExpectingCallbackNotInvoked(
+      mojom::BidderWorklet* bidder_worklet) {
+    bidder_worklet->GenerateBid(
+        CreateBidderWorkletNonSharedParams(), auction_signals_,
+        per_buyer_signals_, browser_signal_seller_origin_,
+        CreateBiddingBrowserSignals(), auction_start_time_,
+        base::BindOnce([](mojom::BidderWorkletBidPtr bid,
+                          const std::vector<std::string>& errors) {
+          ADD_FAILURE() << "Callback should not be invoked.";
+        }));
+  }
+
   // Create a BidderWorklet and invokes GenerateBid(), waiting for the
   // GenerateBid() callback to be invoked. Returns a null Remote on failure.
   mojo::Remote<mojom::BidderWorklet> CreateWorkletAndGenerateBid() {
@@ -318,7 +338,39 @@ class BidderWorkletTest : public testing::Test {
     load_script_run_loop_->Quit();
   }
 
+  // Waits for OnDisconnectWithReason() to be invoked, if it hasn't been
+  // already, and returns the error string it was invoked with.
+  std::string WaitForDisconnect() {
+    DCHECK(!disconnect_run_loop_);
+
+    if (!disconnect_reason_) {
+      disconnect_run_loop_ = std::make_unique<base::RunLoop>();
+      disconnect_run_loop_->Run();
+      disconnect_run_loop_.reset();
+    }
+
+    DCHECK(disconnect_reason_);
+    std::string disconnect_reason = std::move(disconnect_reason_).value();
+    disconnect_reason_.reset();
+    return disconnect_reason;
+  }
+
  protected:
+  void ClosePipeCallback(mojo::ReceiverId receiver_id,
+                         const std::string& description) {
+    bidder_worklets_.RemoveWithReason(receiver_id, /*custom_reason_code=*/0,
+                                      description);
+  }
+
+  void OnDisconnectWithReason(uint32_t custom_reason,
+                              const std::string& description) {
+    DCHECK(!disconnect_reason_);
+
+    disconnect_reason_ = description;
+    if (disconnect_run_loop_)
+      disconnect_run_loop_->Quit();
+  }
+
   std::vector<mojo::StructPtr<mojom::PreviousWin>> CloneWinList(
       const std::vector<mojo::StructPtr<mojom::PreviousWin>>& prev_win_list) {
     std::vector<mojo::StructPtr<mojom::PreviousWin>> out;
@@ -369,6 +421,15 @@ class BidderWorkletTest : public testing::Test {
 
   network::TestURLLoaderFactory url_loader_factory_;
   scoped_refptr<AuctionV8Helper> v8_helper_;
+
+  // Reuseable run loop for disconnection errors.
+  std::unique_ptr<base::RunLoop> disconnect_run_loop_;
+  absl::optional<std::string> disconnect_reason_;
+
+  // Owns all created BidderWorklets - having a ReceiverSet allows them to have
+  // a ClosePipeCallback which behaves just like the one in
+  // AuctionWorkletServiceImpl, to better match production behavior.
+  mojo::UniqueReceiverSet<mojom::BidderWorklet> bidder_worklets_;
 };
 
 // Test the case the BidderWorklet pipe is closed before invoking the
@@ -378,31 +439,34 @@ class BidderWorkletTest : public testing::Test {
 // invoking it.
 TEST_F(BidderWorkletTest, PipeClosed) {
   auto bidder_worklet = CreateWorklet();
-  GenerateBid(bidder_worklet.get());
+  GenerateBidExpectingCallbackNotInvoked(bidder_worklet.get());
   bidder_worklet.reset();
+  EXPECT_FALSE(bidder_worklets_.empty());
 
   // This should not result in a Mojo crash.
   task_environment_.RunUntilIdle();
+  EXPECT_TRUE(bidder_worklets_.empty());
 }
 
 TEST_F(BidderWorkletTest, NetworkError) {
   url_loader_factory_.AddResponse(interest_group_bidding_url_.spec(),
                                   CreateBasicGenerateBidScript(),
                                   net::HTTP_NOT_FOUND);
-  RunGenerateBidExpectingResult(
-      mojom::BidderWorkletBidPtr() /* expected_bid */,
-      {"Failed to load https://url.test/ HTTP status = 404 Not Found."});
+  auto bidder_worklet = CreateWorklet();
+  GenerateBidExpectingCallbackNotInvoked(bidder_worklet.get());
+  EXPECT_EQ("Failed to load https://url.test/ HTTP status = 404 Not Found.",
+            WaitForDisconnect());
 }
 
 TEST_F(BidderWorkletTest, CompileError) {
   AddJavascriptResponse(&url_loader_factory_, interest_group_bidding_url_,
                         "Invalid Javascript");
-  EXPECT_FALSE(CreateWorkletAndGenerateBid());
+  auto bidder_worklet = CreateWorklet();
+  GenerateBidExpectingCallbackNotInvoked(bidder_worklet.get());
 
-  EXPECT_FALSE(bid());
-  ASSERT_EQ(1u, bid_errors().size());
-  EXPECT_THAT(bid_errors()[0], StartsWith("https://url.test/:1 "));
-  EXPECT_THAT(bid_errors()[0], HasSubstr("SyntaxError"));
+  std::string error = WaitForDisconnect();
+  EXPECT_THAT(error, StartsWith("https://url.test/:1 "));
+  EXPECT_THAT(error, HasSubstr("SyntaxError"));
 }
 
 // Test parsing of return values.
@@ -927,58 +991,21 @@ TEST_F(BidderWorkletTest, GenerateBidParallel) {
 }
 
 // Test multiple GenerateBid calls on a single worklet, in parallel, in the case
-// the script fails to load. Do this twice, once before the worklet has
-// encountered a network error, and once after, to make sure both cases work.
-TEST_F(BidderWorkletTest, GenerateBidNetworkErrorParallel) {
+// the script fails to load.
+TEST_F(BidderWorkletTest, GenerateBidParallelLoadFails) {
   auto bidder_worklet = CreateWorklet();
 
-  // For the first loop iteration, call GenerateBid repeatedly and only then
-  // provide the network error. For the second loop iteration, reuse the bidder
-  // worklet from the first iteration, so the Javascript is loaded from the
-  // start.
-  for (bool generate_bid_invoked_before_network_error : {false, true}) {
-    SCOPED_TRACE(generate_bid_invoked_before_network_error);
-
-    base::RunLoop run_loop;
-    const size_t kNumGenerateBidCalls = 10;
-    size_t num_generate_bid_calls = 0;
-    for (size_t i = 0; i < kNumGenerateBidCalls; ++i) {
-      bidder_worklet->GenerateBid(
-          CreateBidderWorkletNonSharedParams(), auction_signals_,
-          per_buyer_signals_, browser_signal_seller_origin_,
-          CreateBiddingBrowserSignals(), auction_start_time_,
-          base::BindLambdaForTesting(
-              [&run_loop, &num_generate_bid_calls](
-                  mojom::BidderWorkletBidPtr bid,
-                  const std::vector<std::string>& errors) {
-                EXPECT_FALSE(bid);
-                EXPECT_EQ(errors, std::vector<std::string>{
-                                      "Failed to load https://url.test/ HTTP "
-                                      "status = 404 Not Found."});
-                ++num_generate_bid_calls;
-                if (num_generate_bid_calls == kNumGenerateBidCalls)
-                  run_loop.Quit();
-              }));
-    }
-
-    // If this is the first loop iteration, wait for all the Mojo calls to
-    // settle, and then provide the network error.
-    if (generate_bid_invoked_before_network_error == false) {
-      // Since the script hasn't loaded yet, or failed to load, no bids should
-      // be generated.
-      task_environment_.RunUntilIdle();
-      EXPECT_FALSE(run_loop.AnyQuitCalled());
-      EXPECT_EQ(0u, num_generate_bid_calls);
-
-      // Script fails to load.
-      url_loader_factory_.AddResponse(interest_group_bidding_url_.spec(),
-                                      CreateBasicGenerateBidScript(),
-                                      net::HTTP_NOT_FOUND);
-    }
-
-    run_loop.Run();
-    EXPECT_EQ(kNumGenerateBidCalls, num_generate_bid_calls);
+  for (size_t i = 0; i < 10; ++i) {
+    GenerateBidExpectingCallbackNotInvoked(bidder_worklet.get());
   }
+
+  // Script fails to load.
+  url_loader_factory_.AddResponse(interest_group_bidding_url_.spec(),
+                                  CreateBasicGenerateBidScript(),
+                                  net::HTTP_NOT_FOUND);
+
+  EXPECT_EQ("Failed to load https://url.test/ HTTP status = 404 Not Found.",
+            WaitForDisconnect());
 }
 
 // Test multiple GenerateBid calls on a single worklet, in parallel, in the case
@@ -1463,11 +1490,16 @@ TEST_F(BidderWorkletTest, GenerateBidWasm404) {
               /*charset=*/absl::nullopt, "Error 404", kAllowFledgeHeader,
               net::HTTP_NOT_FOUND);
 
-  RunGenerateBidWithJavascriptExpectingResult(
-      CreateBasicGenerateBidScript(),
-      /*expected_bid=*/mojom::BidderWorkletBidPtr(),
-      {"Failed to load https://foo.test/helper.wasm "
-       "HTTP status = 404 Not Found."});
+  // The Javascript request receives valid JS.
+  AddJavascriptResponse(&url_loader_factory_, interest_group_bidding_url_,
+                        CreateBasicGenerateBidScript());
+
+  auto bidder_worklet = CreateWorklet();
+  GenerateBidExpectingCallbackNotInvoked(bidder_worklet.get());
+  EXPECT_EQ(
+      "Failed to load https://foo.test/helper.wasm "
+      "HTTP status = 404 Not Found.",
+      WaitForDisconnect());
 }
 
 TEST_F(BidderWorkletTest, GenerateBidWasmFailure) {
@@ -1477,12 +1509,17 @@ TEST_F(BidderWorkletTest, GenerateBidWasmFailure) {
               kWasmMimeType,
               /*charset=*/absl::nullopt, CreateBasicGenerateBidScript());
 
-  RunGenerateBidWithJavascriptExpectingResult(
-      CreateBasicGenerateBidScript(),
-      /*expected_bid=*/mojom::BidderWorkletBidPtr(),
-      {"https://foo.test/helper.wasm Uncaught CompileError: "
-       "WasmModuleObject::Compile(): expected magic word 00 61 73 6d, found "
-       "0a 20 20 20 @+0."});
+  // The Javascript request receives valid JS.
+  AddJavascriptResponse(&url_loader_factory_, interest_group_bidding_url_,
+                        CreateBasicGenerateBidScript());
+
+  auto bidder_worklet = CreateWorklet();
+  GenerateBidExpectingCallbackNotInvoked(bidder_worklet.get());
+  EXPECT_EQ(
+      "https://foo.test/helper.wasm Uncaught CompileError: "
+      "WasmModuleObject::Compile(): expected magic word 00 61 73 6d, found "
+      "0a 20 20 20 @+0.",
+      WaitForDisconnect());
 }
 
 TEST_F(BidderWorkletTest, GenerateBidWasm) {
@@ -1561,8 +1598,14 @@ TEST_F(BidderWorkletTest, WasmOrdering) {
     url_loader_factory_.ClearResponses();
 
     mojo::Remote<mojom::BidderWorklet> bidder_worklet = CreateWorklet();
-    load_script_run_loop_ = std::make_unique<base::RunLoop>();
-    GenerateBid(bidder_worklet.get());
+    if (test.expect_success) {
+      // On success, callback should be invoked.
+      load_script_run_loop_ = std::make_unique<base::RunLoop>();
+      GenerateBid(bidder_worklet.get());
+    } else {
+      // On error, the pipe is closed without invoking the callback.
+      GenerateBidExpectingCallbackNotInvoked(bidder_worklet.get());
+    }
 
     for (Event ev : test.events) {
       switch (ev) {
@@ -1591,9 +1634,16 @@ TEST_F(BidderWorkletTest, WasmOrdering) {
       task_environment_.RunUntilIdle();
     }
 
-    load_script_run_loop_->Run();
-    load_script_run_loop_.reset();
-    EXPECT_EQ(bid_.is_null(), !test.expect_success);
+    if (test.expect_success) {
+      // On success, the callback is invoked.
+      load_script_run_loop_->Run();
+      load_script_run_loop_.reset();
+      EXPECT_TRUE(bid_);
+    } else {
+      // On failure, the pipe is closed with a non-empty error message, without
+      // invoking the callback.
+      EXPECT_FALSE(WaitForDisconnect().empty());
+    }
   }
 }
 
@@ -1872,54 +1922,28 @@ TEST_F(BidderWorkletTest, ReportWinParallel) {
 }
 
 // Test multiple ReportWin calls on a single worklet, in parallel, in the case
-// the worklet script fails to load. Do this twice, once before the worklet
-// script has received an error, and once after, to make sure both cases work.
-TEST_F(BidderWorkletTest, ReportWinNetworkErrorParallel) {
+// the worklet script fails to load.
+TEST_F(BidderWorkletTest, ReportWinParallelLoadFails) {
   auto bidder_worklet = CreateWorklet();
 
-  // For the first loop iteration, call ReportWin repeatedly before providing
-  // the network error script, then provide the network error. For the second
-  // loop iteration, reuse the bidder worklet from the first iteration, so the
-  // network error is present from the start.
-  for (bool report_win_invoked_before_worklet_script_loaded : {false, true}) {
-    SCOPED_TRACE(report_win_invoked_before_worklet_script_loaded);
-
-    base::RunLoop run_loop;
-    const size_t kNumReportWinCalls = 10;
-    size_t num_report_win_calls = 0;
-    for (size_t i = 0; i < kNumReportWinCalls; ++i) {
-      bidder_worklet->ReportWin(
-          interest_group_name_,
-          /*auction_signals_json=*/base::NumberToString(i), per_buyer_signals_,
-          seller_signals_, browser_signal_render_url_, browser_signal_bid_,
-          browser_signal_seller_origin_,
-          base::BindLambdaForTesting(
-              [&run_loop, &num_report_win_calls](
-                  const absl::optional<GURL>& report_url,
-                  const std::vector<std::string>& errors) {
-                EXPECT_FALSE(report_url);
-                EXPECT_EQ(errors, std::vector<std::string>{
-                                      "Failed to load https://url.test/ "
-                                      "HTTP status = 404 Not Found."});
-                ++num_report_win_calls;
-                if (num_report_win_calls == kNumReportWinCalls)
-                  run_loop.Quit();
-              }));
-    }
-
-    // If this is the first loop iteration, wait for all the Mojo calls to
-    // settle, and then provide the Javascript response body.
-    if (report_win_invoked_before_worklet_script_loaded == false) {
-      task_environment_.RunUntilIdle();
-      EXPECT_FALSE(run_loop.AnyQuitCalled());
-      url_loader_factory_.AddResponse(interest_group_bidding_url_.spec(),
-                                      CreateBasicGenerateBidScript(),
-                                      net::HTTP_NOT_FOUND);
-    }
-
-    run_loop.Run();
-    EXPECT_EQ(kNumReportWinCalls, num_report_win_calls);
+  for (size_t i = 0; i < 10; ++i) {
+    bidder_worklet->ReportWin(
+        interest_group_name_,
+        /*auction_signals_json=*/base::NumberToString(i), per_buyer_signals_,
+        seller_signals_, browser_signal_render_url_, browser_signal_bid_,
+        browser_signal_seller_origin_,
+        base::BindOnce([](const absl::optional<GURL>& report_url,
+                          const std::vector<std::string>& errors) {
+          ADD_FAILURE() << "Callback should not be invoked.";
+        }));
   }
+
+  url_loader_factory_.AddResponse(interest_group_bidding_url_.spec(),
+                                  CreateBasicGenerateBidScript(),
+                                  net::HTTP_NOT_FOUND);
+
+  EXPECT_EQ("Failed to load https://url.test/ HTTP status = 404 Not Found.",
+            WaitForDisconnect());
 }
 
 // Make sure Date() is not available when running reportWin().
@@ -2272,7 +2296,7 @@ TEST_F(BidderWorkletTest, ParseErrorV8Debug) {
   auto worklet =
       CreateWorklet(interest_group_bidding_url_,
                     /*pause_for_debugger_on_start=*/true, &worklet_impl);
-  GenerateBid(worklet.get());
+  GenerateBidExpectingCallbackNotInvoked(worklet.get());
   int id = worklet_impl->context_group_id_for_testing();
   TestChannel* channel = inspector_support.ConnectDebuggerSession(id);
 
@@ -2283,14 +2307,13 @@ TEST_F(BidderWorkletTest, ParseErrorV8Debug) {
       R"({"id":2,"method":"Debugger.enable","params":{}})");
 
   // Unpause execution.
-  load_script_run_loop_ = std::make_unique<base::RunLoop>();
   channel->RunCommandAndWaitForResult(
       3, "Runtime.runIfWaitingForDebugger",
       R"({"id":3,"method":"Runtime.runIfWaitingForDebugger","params":{}})");
-  load_script_run_loop_->Run();
-  load_script_run_loop_.reset();
 
-  EXPECT_FALSE(bid_);
+  // Worklet should disconnect with an error message.
+  EXPECT_FALSE(WaitForDisconnect().empty());
+
   // Should have gotten a parse error notification.
   TestChannel::Event parse_error =
       channel->WaitForMethodNotification("Debugger.scriptFailedToParse");

content/services/auction_worklet/public/mojom/auction_worklet_service.mojom

@@ -32,11 +32,12 @@ interface AuctionWorkletService {
   // the DevTools hooks restrict sharing to auctions within a single
   // RenderFrame.
   //
-  // There is no return value - any error loading the worklet script may only be
-  // learned from the BidderWorklet API itself, when trying to run a script.
-  // This reduces roundtrips, and allows fetching per-auction bidding signals
-  // URL in parallel with fetching the bidding worklet script. It also makes the
-  // API more robust, not having any invocation ordering requirements.
+  // All methods may be invoked immediately upon creation. If there's a pending
+  // load of the script, callbacks will be delayed until it completes.
+  //
+  // On load error, the worklet will close its pipe with a reason string. The
+  // reason string, and worklet errors messages more generally, are considered
+  // privileged and should not be passed to renderer processes.
   //
   // Arguments:
   // `bidder_worklet` The pipe to communicate with the BidderWorklet. Closing