From c3eb3450c0f373e9e66785aad465dbc7c7a61741 Mon Sep 17 00:00:00 2001
From: Huiting Yu <huitingyu@google.com>
Date: Wed, 13 May 2026 09:01:56 -0700
Subject: [PATCH] [ios]Refactor OverlayRequestCoordinatorDelegate to use OverlayRequestId

1. For the dismissal method inside OverlayRequestCoordinatorDelegate,
replace passing a raw OverlayRequest* by a safe, copied 64-bit
OverlayRequestId. This completely eliminates Use-After-Free bugs where
the OverlayRequest object has been freed before the coordinator delegate
receives the completion event, which is theoretically possible because
once the OverlayRequestQueue releases the object, OverlayPresenter only
conditionally extends the object's life, reference of ownership:
https://huitingyu.users.x20web.corp.google.com/overlay_request_flow.html.

2. Refactor of OverlayRequestCoordinator to back request ivar with
raw_ptr, so such UAF can generate crash reports. Fix the unittest
crashes introduced by the refactor.

Fixed: 509712284
Change-Id: I125123e9957a2b0bf25cff6df87b0f5f08f59682
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7842832
Reviewed-by: Chris Lu <thegreenfrog@chromium.org>
Commit-Queue: Huiting Yu <huitingyu@google.com>
Cr-Commit-Position: refs/heads/main@{#1630011}
---

diff --git a/ios/chrome/browser/overlays/ui_bundled/infobar_banner/infobar_banner_overlay_coordinator.mm b/ios/chrome/browser/overlays/ui_bundled/infobar_banner/infobar_banner_overlay_coordinator.mm
index 2afec98..f303ef9 100644
--- a/ios/chrome/browser/overlays/ui_bundled/infobar_banner/infobar_banner_overlay_coordinator.mm
+++ b/ios/chrome/browser/overlays/ui_bundled/infobar_banner/infobar_banner_overlay_coordinator.mm
@@ -215,7 +215,7 @@
   // is necessary to synchronize OverlayPresenter scheduling logic with the UI
   // layer.
   if (self.delegate) {
-    self.delegate->OverlayUIDidFinishDismissal(self.request);
+    self.delegate->OverlayUIDidFinishDismissal(self.requestId);
   }
   UpdateBannerAccessibilityForDismissal(self.baseViewController);
 }
diff --git a/ios/chrome/browser/overlays/ui_bundled/infobar_banner/translate/translate_infobar_placeholder_overlay_coordinator.mm b/ios/chrome/browser/overlays/ui_bundled/infobar_banner/translate/translate_infobar_placeholder_overlay_coordinator.mm
index 6d2cf845..cab2f44d 100644
--- a/ios/chrome/browser/overlays/ui_bundled/infobar_banner/translate/translate_infobar_placeholder_overlay_coordinator.mm
+++ b/ios/chrome/browser/overlays/ui_bundled/infobar_banner/translate/translate_infobar_placeholder_overlay_coordinator.mm
@@ -43,7 +43,7 @@
   // Notify the presentation context that the dismissal has finished.  This
   // is necessary to synchronize OverlayPresenter scheduling logic with the UI
   // layer.
-  self.delegate->OverlayUIDidFinishDismissal(self.request);
+  self.delegate->OverlayUIDidFinishDismissal(self.requestId);
 }
 
 @end
diff --git a/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator.mm b/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator.mm
index 764dda168..833fa46 100644
--- a/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator.mm
+++ b/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator.mm
@@ -115,7 +115,7 @@
   // is necessary to synchronize OverlayPresenter scheduling logic with the UI
   // layer.
   if (self.delegate) {
-    self.delegate->OverlayUIDidFinishDismissal(self.request);
+    self.delegate->OverlayUIDidFinishDismissal(self.requestId);
   }
 }
 
diff --git a/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator_unittest.mm b/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator_unittest.mm
index 596148b4..3a877cc99 100644
--- a/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator_unittest.mm
+++ b/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator_unittest.mm
@@ -136,7 +136,7 @@
 
   // Stop the coordinator, expecting OverlayUIDidFinishDismissal() to be
   // executed.
-  EXPECT_CALL(delegate_, OverlayUIDidFinishDismissal(request_.get()));
+  EXPECT_CALL(delegate_, OverlayUIDidFinishDismissal(request_->GetRequestId()));
   [coordinator_ stopAnimated:NO];
 
   // Wait for dismissal to finish.
@@ -162,7 +162,8 @@
 
   // Stop the coordinator, expecting OverlayUIDidFinishDismissal() to be
   // executed once.
-  EXPECT_CALL(delegate_, OverlayUIDidFinishDismissal(request_.get())).Times(1);
+  EXPECT_CALL(delegate_, OverlayUIDidFinishDismissal(request_->GetRequestId()))
+      .Times(1);
   [coordinator_ stopAnimated:NO];
 
   // Stop coordinator again. It should be a no-op since stop has been called
diff --git a/ios/chrome/browser/overlays/ui_bundled/overlay_presentation_context_impl.h b/ios/chrome/browser/overlays/ui_bundled/overlay_presentation_context_impl.h
index ebc8d16..b7372ff 100644
--- a/ios/chrome/browser/overlays/ui_bundled/overlay_presentation_context_impl.h
+++ b/ios/chrome/browser/overlays/ui_bundled/overlay_presentation_context_impl.h
@@ -161,7 +161,7 @@
 
     // OverlayUIDismissalDelegate:
     void OverlayUIDidFinishPresentation(OverlayRequest* request) override;
-    void OverlayUIDidFinishDismissal(OverlayRequest* request) override;
+    void OverlayUIDidFinishDismissal(OverlayRequestId request_id) override;
 
    private:
     raw_ptr<OverlayPresentationContextImpl> presentation_context_ = nullptr;
diff --git a/ios/chrome/browser/overlays/ui_bundled/overlay_presentation_context_impl.mm b/ios/chrome/browser/overlays/ui_bundled/overlay_presentation_context_impl.mm
index fb28980..57dc4b59 100644
--- a/ios/chrome/browser/overlays/ui_bundled/overlay_presentation_context_impl.mm
+++ b/ios/chrome/browser/overlays/ui_bundled/overlay_presentation_context_impl.mm
@@ -471,9 +471,7 @@
 }
 
 void OverlayPresentationContextImpl::OverlayRequestCoordinatorDelegateImpl::
-    OverlayUIDidFinishDismissal(OverlayRequest* request) {
-  DCHECK(request);
-  const OverlayRequestId request_id = OverlayRequestIdForRequest(request);
+    OverlayUIDidFinishDismissal(OverlayRequestId request_id) {
   DCHECK_EQ(presentation_context_->request_id_, request_id);
   presentation_context_->OverlayUIWasDismissed();
 }
diff --git a/ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator.h b/ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator.h
index d7c389b..fa8effe 100644
--- a/ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator.h
+++ b/ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator.h
@@ -5,11 +5,11 @@
 #ifndef IOS_CHROME_BROWSER_OVERLAYS_UI_BUNDLED_OVERLAY_REQUEST_COORDINATOR_H_
 #define IOS_CHROME_BROWSER_OVERLAYS_UI_BUNDLED_OVERLAY_REQUEST_COORDINATOR_H_
 
+#import "ios/chrome/browser/overlays/model/public/overlay_request.h"
 #import "ios/chrome/browser/shared/coordinator/chrome_coordinator/chrome_coordinator.h"
 
 class OverlayRequestSupport;
 class OverlayRequestCoordinatorDelegate;
-class OverlayRequest;
 
 // Coordinator superclass used to present UI for an OverlayRequest.
 @interface OverlayRequestCoordinator : ChromeCoordinator
@@ -43,6 +43,9 @@
 // The request used to configure the overlay UI.
 @property(nonatomic, readonly) OverlayRequest* request;
 
+// The 64-bit identifier of the request.
+@property(nonatomic, readonly) OverlayRequestId requestId;
+
 // The view controller that displays the UI for `request`.
 @property(nonatomic, readonly) UIViewController* viewController;
 
diff --git a/ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator.mm b/ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator.mm
index 8289903..0e7f3c1e 100644
--- a/ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator.mm
+++ b/ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator.mm
@@ -7,20 +7,27 @@
 #import <ostream>
 
 #import "base/check.h"
+#import "base/functional/bind.h"
+#import "base/functional/callback.h"
+#import "base/memory/raw_ptr.h"
 #import "base/notreached.h"
+#import "ios/chrome/browser/overlays/model/public/overlay_callback_manager.h"
+#import "ios/chrome/browser/overlays/model/public/overlay_request.h"
 #import "ios/chrome/browser/overlays/model/public/overlay_request_support.h"
 #import "ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator+subclassing.h"
 #import "ios/chrome/browser/overlays/ui_bundled/overlay_request_coordinator_delegate.h"
 #import "ios/chrome/browser/overlays/ui_bundled/overlay_request_mediator.h"
 
-@interface OverlayRequestCoordinator () <OverlayRequestMediatorDelegate> {
+@interface OverlayRequestCoordinator () <OverlayRequestMediatorDelegate>
+@end
+
+@implementation OverlayRequestCoordinator {
   // Subclassing properties.
   BOOL _started;
   OverlayRequestMediator* _mediator;
+  raw_ptr<OverlayRequest> _request;
+  OverlayRequestId _requestId;
 }
-@end
-
-@implementation OverlayRequestCoordinator
 
 - (void)dealloc {
   // ChromeCoordinator's `-dealloc` calls `-stop`, which defaults to an animated
@@ -48,12 +55,34 @@
   if (self) {
     _request = request;
     DCHECK(_request);
+    _requestId = request->GetRequestId();
     _delegate = delegate;
     DCHECK(_delegate);
+
+    // Register a completion callback to synchronously null-out `_request` as
+    // soon as the request is completed or destroyed. This ensures that
+    // `_request` never becomes a dangling raw_ptr.
+    __weak __typeof(self) weakSelf = self;
+    _request->GetCallbackManager()->AddCompletionCallback(
+        base::BindOnce(^(OverlayResponse*) {
+          [weakSelf requestWasCompleted];
+        }));
   }
   return self;
 }
 
+- (OverlayRequest*)request {

diff --git a/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator_unittest.mm b/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator_unittest.mm
index 596148b4..3a877cc99 100644
--- a/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator_unittest.mm
+++ b/ios/chrome/browser/overlays/ui_bundled/infobar_modal/infobar_modal_overlay_coordinator_unittest.mm
@@ -136,7 +136,7 @@
 
   // Stop the coordinator, expecting OverlayUIDidFinishDismissal() to be
   // executed.
-  EXPECT_CALL(delegate_, OverlayUIDidFinishDismissal(request_.get()));
+  EXPECT_CALL(delegate_, OverlayUIDidFinishDismissal(request_->GetRequestId()));
   [coordinator_ stopAnimated:NO];
 
   // Wait for dismissal to finish.
@@ -162,7 +162,8 @@
 
   // Stop the coordinator, expecting OverlayUIDidFinishDismissal() to be
   // executed once.
-  EXPECT_CALL(delegate_, OverlayUIDidFinishDismissal(request_.get())).Times(1);
+  EXPECT_CALL(delegate_, OverlayUIDidFinishDismissal(request_->GetRequestId()))
+      .Times(1);
   [coordinator_ stopAnimated:NO];
 
   // Stop coordinator again. It should be a no-op since stop has been called
diff --git a/ios/chrome/browser/overlays/ui_bundled/test/fake_overlay_request_coordinator_delegate.cc b/ios/chrome/browser/overlays/ui_bundled/test/fake_overlay_request_coordinator_delegate.cc
index b0d8cc2c..01249f1 100644
--- a/ios/chrome/browser/overlays/ui_bundled/test/fake_overlay_request_coordinator_delegate.cc
+++ b/ios/chrome/browser/overlays/ui_bundled/test/fake_overlay_request_coordinator_delegate.cc
@@ -12,22 +12,32 @@
 
 bool FakeOverlayRequestCoordinatorDelegate::HasUIBeenPresented(
     OverlayRequest* request) const {
-  return states_.contains(request) &&
-         states_.at(request) == PresentationState::kPresented;
+  if (!request) {
+    return false;
+  }
+  OverlayRequestId request_id = request->GetRequestId();
+  return states_.contains(request_id) &&
+         states_.at(request_id) == PresentationState::kPresented;
 }
 
 bool FakeOverlayRequestCoordinatorDelegate::HasUIBeenDismissed(
     OverlayRequest* request) const {
-  return states_.contains(request) &&
-         states_.at(request) == PresentationState::kDismissed;
+  if (!request) {
+    return false;
+  }
+  OverlayRequestId request_id = request->GetRequestId();
+  return states_.contains(request_id) &&
+         states_.at(request_id) == PresentationState::kDismissed;
 }
 
 void FakeOverlayRequestCoordinatorDelegate::OverlayUIDidFinishPresentation(
     OverlayRequest* request) {
-  states_[request] = PresentationState::kPresented;
+  if (request) {
+    states_[request->GetRequestId()] = PresentationState::kPresented;
+  }
 }
 
 void FakeOverlayRequestCoordinatorDelegate::OverlayUIDidFinishDismissal(
-    OverlayRequest* request) {
-  states_[request] = PresentationState::kDismissed;
+    OverlayRequestId request_id) {
+  states_[request_id] = PresentationState::kDismissed;
 }
diff --git a/ios/chrome/browser/overlays/ui_bundled/test/fake_overlay_request_coordinator_delegate.h b/ios/chrome/browser/overlays/ui_bundled/test/fake_overlay_request_coordinator_delegate.h
index bf6aa425..4352eab 100644
--- a/ios/chrome/browser/overlays/ui_bundled/test/fake_overlay_request_coordinator_delegate.h
+++ b/ios/chrome/browser/overlays/ui_bundled/test/fake_overlay_request_coordinator_delegate.h
@@ -24,11 +24,11 @@
 
   // OverlayRequestCoordinatorDelegate:
   void OverlayUIDidFinishPresentation(OverlayRequest* request) override;
-  void OverlayUIDidFinishDismissal(OverlayRequest* request) override;
+  void OverlayUIDidFinishDismissal(OverlayRequestId request_id) override;
 
  private:
   enum class PresentationState { kNotPresented, kPresented, kDismissed };
-  std::map<OverlayRequest*, PresentationState> states_;
+  std::map<OverlayRequestId, PresentationState> states_;
 };
 
 #endif  // IOS_CHROME_BROWSER_OVERLAYS_UI_BUNDLED_TEST_FAKE_OVERLAY_REQUEST_COORDINATOR_DELEGATE_H_
diff --git a/ios/chrome/browser/overlays/ui_bundled/test/mock_overlay_coordinator_delegate.h b/ios/chrome/browser/overlays/ui_bundled/test/mock_overlay_coordinator_delegate.h
index ec0b8ddb..157061b 100644
--- a/ios/chrome/browser/overlays/ui_bundled/test/mock_overlay_coordinator_delegate.h
+++ b/ios/chrome/browser/overlays/ui_bundled/test/mock_overlay_coordinator_delegate.h
@@ -16,7 +16,7 @@
   ~MockOverlayRequestCoordinatorDelegate() override;
 
   MOCK_METHOD1(OverlayUIDidFinishPresentation, void(OverlayRequest* request));
-  MOCK_METHOD1(OverlayUIDidFinishDismissal, void(OverlayRequest* request));
+  MOCK_METHOD1(OverlayUIDidFinishDismissal, void(OverlayRequestId request_id));
 };
 
 #endif  // IOS_CHROME_BROWSER_OVERLAYS_UI_BUNDLED_TEST_MOCK_OVERLAY_COORDINATOR_DELEGATE_H_