[ios] Fix Overlay Presenter dismiss logic for detached WebStates

When a WebState is detached from a WebStateList, the Overlay Presenter
removes itself as the OverlayRequestQueue's delegate. If an overlay
was being presented, the queue then removes the request. However,
the OverlayWasDismissed() callback of the request is still executed,
causing bad behavior given that the request is now null.

Two new codepaths are introduced to managed this newly discovered scenario
best reproduced with using the Crash Restore Infobar:

1) The presenter remains the delegate of a queue when its WebState
is detached and an overlay is being presented. That way if the queue is
destroyed and the requests are cancelled,
| removed_request_awaiting_dismissal_| can be used to extend its lifetime
until dismissal callbacks are finished.

2) If the presenter is replaced as the queue's delegate, there is no
way to guarantee the validity of |presenting_request_| from then on.
This can technically also occur separately from WebState detachment.
Thus, upon replacement, the presenter will do early DidHideOverlay()
calls, reset |presenting_request_| and mark
|detached_queue_replaced_delegate_| as true so that
it knows to early return in OverlayWasDismissed since the request
is likely to be invalid at that point.


Bug: 1071914
Change-Id: Ie7d7d7e67849323ac482b83680f7e35853b8c284
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2207509
Commit-Queue: Chris Lu <thegreenfrog@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774292}

ios/chrome/browser/overlays/overlay_presenter_impl.h

@@ -106,8 +106,8 @@ class OverlayPresenterImpl : public BrowserObserver,
 
   // Sets up and tears down observation and delegation for |web_state|'s request
   // queue when it is added or removed from the Browser.
-  void StartObservingWebState(web::WebState* web_state);
-  void StopObservingWebState(web::WebState* web_state);
+  void WebStateAddedToBrowser(web::WebState* web_state);
+  void WebStateRemovedFromBrowser(web::WebState* web_state);
 
   // BrowserObserver:
   void BrowserDestroyed(Browser* browser) override;
@@ -116,6 +116,8 @@ class OverlayPresenterImpl : public BrowserObserver,
   void OverlayRequestRemoved(OverlayRequestQueueImpl* queue,
                              std::unique_ptr<OverlayRequest> request,
                              bool cancelled) override;
+  void OverlayRequestQueueWillReplaceDelegate(
+      OverlayRequestQueueImpl* queue) override;
 
   // OverlayRequestQueueImpl::Observer:
   void RequestAddedToQueue(OverlayRequestQueueImpl* queue,
@@ -156,14 +158,23 @@ class OverlayPresenterImpl : public BrowserObserver,
   // true from the beginning of the presentation until the end of the
   // dismissal.
   bool presenting_ = false;
+  // Whether |detached_presenting_request_queue_| has replaced this
+  // presenter as its delegate. This property will help manage a situation where
+  // the WebState replaces another presenter with this presenter while an
+  // overlay request is still presenting, requiring this presenter to cleanup
+  // references to the request before the request is dismissed.
+  bool detached_queue_replaced_delegate_ = false;
+  // The OverlayRequestQueue owning |presented_request_| has recently been
+  // detached.
+  OverlayRequestQueueImpl* detached_presenting_request_queue_ = nullptr;
   // The request whose overlay UI is currently being presented.  The value is
   // set when |presenting_| is set to true, and is reset to nullptr when
   // |presenting_| is reset to false.  May be different from GetActiveRequest()
   // if the front request of the active WebState's request queue is updated
   // while overlay UI is be presented.
   OverlayRequest* presented_request_ = nullptr;
-  // Whether the active WebState is being detached.
-  bool detaching_active_web_state_ = false;
+  // Whether the WebState that owns |presented_request_| is being detached.
+  bool detaching_presenting_web_state_ = false;
   // Used to extend the lifetime of an OverlayRequest after being removed from
   // a queue until the completion of its dismissal flow.
   std::unique_ptr<OverlayRequest> removed_request_awaiting_dismissal_;

ios/chrome/browser/overlays/overlay_presenter_impl.mm

@@ -58,7 +58,7 @@ OverlayPresenterImpl::OverlayPresenterImpl(Browser* browser,
   DCHECK(web_state_list_);
   web_state_list_->AddObserver(this);
   for (int i = 0; i < web_state_list_->count(); ++i) {
-    StartObservingWebState(web_state_list_->GetWebStateAt(i));
+    WebStateAddedToBrowser(web_state_list_->GetWebStateAt(i));
   }
   SetActiveWebState(web_state_list_->GetActiveWebState(),
                     ActiveWebStateChangeReason::Activated);
@@ -134,14 +134,14 @@ void OverlayPresenterImpl::SetActiveWebState(
   // The UI should be cancelled instead of hidden if the presenter does not
   // expect to show any more overlay UI for previously active WebState in the UI
   // delegate's presentation context.  This occurs:
-  // - when the active WebState is replaced, and
-  // - when the active WebState is detached from the WebStateList.
+  // - when the presenting WebState is replaced, and
+  // - when the presenting WebState is detached from the WebStateList.
   const bool should_cancel_ui =
       (reason == ActiveWebStateChangeReason::Replaced) ||
-      detaching_active_web_state_;
+      detaching_presenting_web_state_;
 
   active_web_state_ = web_state;
-  detaching_active_web_state_ = false;
+  detaching_presenting_web_state_ = false;
 
   // Early return if there's no UI delegate, since presentation cannot occur.
   if (!presentation_context_)
@@ -256,6 +256,21 @@ void OverlayPresenterImpl::OverlayWasDismissed(
   if (presentation_context_ != presentation_context)
     return;
 
+  // When the presenter has been replaced as the delegate of the active
+  // OverlayRequestQueue, observers are notified of DidHideOverlay() and
+  // |presented_request_| is reset early. Thus, there is no need to do any
+  // dismissal bookkeeping since the request has been removed.
+  if (detached_queue_replaced_delegate_) {
+    presenting_ = false;
+    detached_queue_replaced_delegate_ = false;
+    if (GetActiveRequest()) {
+      PresentOverlayForActiveRequest();
+    }
+    return;
+  }
+
+  DCHECK_EQ(presented_request_, request);
+
   // Pop the request for overlays dismissed by the user.  The check against
   // |removed_request_awaiting_dismissal_| prevents the queue's front request
   // from being popped if this dismissal was caused by |request|'s removal from
@@ -271,6 +286,14 @@ void OverlayPresenterImpl::OverlayWasDismissed(
 
   presenting_ = false;
   presented_request_ = nullptr;
+  // The OverlayPresenter remains as the delegate for
+  // |detached_presenting_request_queue_| to ensure that |presented_request_| is
+  // not deleted before the dismissal of its UI is finished.  Since the UI is
+  // now being dismissed, the delegate can be reset.
+  if (detached_presenting_request_queue_) {
+    detached_presenting_request_queue_->SetDelegate(nullptr);
+    detached_presenting_request_queue_ = nullptr;
+  }
 
   // Notify the observers that the overlay UI was hidden.
   for (auto& observer : observers_) {
@@ -307,16 +330,33 @@ void OverlayPresenterImpl::CancelAllOverlayUI() {
 
 #pragma mark WebState helpers
 
-void OverlayPresenterImpl::StartObservingWebState(web::WebState* web_state) {
+void OverlayPresenterImpl::WebStateAddedToBrowser(web::WebState* web_state) {
   OverlayRequestQueueImpl* queue = GetQueueForWebState(web_state);
   queue->AddObserver(this);
-  queue->set_delegate(this);
+  queue->SetDelegate(this);
 }
 
-void OverlayPresenterImpl::StopObservingWebState(web::WebState* web_state) {
+void OverlayPresenterImpl::WebStateRemovedFromBrowser(
+    web::WebState* web_state) {
   OverlayRequestQueueImpl* queue = GetQueueForWebState(web_state);
   queue->RemoveObserver(this);
-  queue->set_delegate(nullptr);
+  // Only reset the delegate if there isn't a currently presented overlay or
+  // |presented_request_|'s WebState is not the WebState being removed. This
+  // will allow the presenter to extend the lifetime of |presented_request_| if
+  // it is removed from the queue before its dismissal finishes.
+  if (!presented_request_ ||
+      presented_request_->GetQueueWebState() != web_state) {
+    queue->SetDelegate(nullptr);
+  }
+
+  if (presented_request_ &&
+      presented_request_->GetQueueWebState() == web_state) {
+    detached_presenting_request_queue_ = GetQueueForWebState(web_state);
+  } else {
+    // For inactive WebState removals, the overlay UI can be cancelled
+    // immediately.
+    CancelOverlayUIForRequest(GetFrontRequestForWebState(web_state));
+  }
 }
 
 #pragma mark -
@@ -327,8 +367,11 @@ void OverlayPresenterImpl::BrowserDestroyed(Browser* browser) {
   SetActiveWebState(nullptr, ActiveWebStateChangeReason::Closed);
 
   for (int i = 0; i < web_state_list_->count(); ++i) {
-    StopObservingWebState(web_state_list_->GetWebStateAt(i));
+    WebStateRemovedFromBrowser(web_state_list_->GetWebStateAt(i));
   }
+  // All Webstates are detached before the Browser is destroyed so all request
+  // must be cancelled at this point.
+  DCHECK(!detached_presenting_request_queue_);
   web_state_list_->RemoveObserver(this);
   web_state_list_ = nullptr;
   removed_request_awaiting_dismissal_ = nullptr;
@@ -342,12 +385,36 @@ void OverlayPresenterImpl::OverlayRequestRemoved(
     std::unique_ptr<OverlayRequest> request,
     bool cancelled) {
   OverlayRequest* removed_request = request.get();
-  if (presented_request_ == removed_request)
+  if (presented_request_ == removed_request) {
     removed_request_awaiting_dismissal_ = std::move(request);
+    if (detached_presenting_request_queue_) {
+      detached_presenting_request_queue_ = nullptr;
+      queue->SetDelegate(nullptr);
+    }
+  }
   if (cancelled)
     CancelOverlayUIForRequest(removed_request);
 }
 
+void OverlayPresenterImpl::OverlayRequestQueueWillReplaceDelegate(
+    OverlayRequestQueueImpl* queue) {
+  if (!presented_request_ || presented_request_ != queue->front_request())
+    return;
+  // If |presented_request_| is in the queue that is replacing this presenter
+  // as the delegate, it is no longer possible to extend the lifetime of
+  // |presented_request_|. Thus, call DidHideOverlay while it is still valid
+  // and reset its reference.
+  for (auto& observer : observers_) {
+    if (observer.GetRequestSupport(this)->IsRequestSupported(
+            presented_request_)) {
+      observer.DidHideOverlay(this, presented_request_);
+    }
+  }
+  presented_request_ = nullptr;
+  detached_presenting_request_queue_ = nullptr;
+  detached_queue_replaced_delegate_ = true;
+}
+
 #pragma mark OverlayRequestQueueImpl::Observer
 
 void OverlayPresenterImpl::RequestAddedToQueue(OverlayRequestQueueImpl* queue,
@@ -428,34 +495,24 @@ void OverlayPresenterImpl::WebStateInsertedAt(WebStateList* web_state_list,
                                               web::WebState* web_state,
                                               int index,
                                               bool activating) {
-  StartObservingWebState(web_state);
+  WebStateAddedToBrowser(web_state);
 }
 
 void OverlayPresenterImpl::WebStateReplacedAt(WebStateList* web_state_list,
                                               web::WebState* old_web_state,
                                               web::WebState* new_web_state,
                                               int index) {
-  StopObservingWebState(old_web_state);
-  StartObservingWebState(new_web_state);
-  if (old_web_state != active_web_state_) {
-    // If the active WebState is being replaced, its overlay UI will be
-    // cancelled later when |new_web_state| is activated.  For inactive WebState
-    // replacements, the overlay UI can be cancelled immediately.
-    CancelOverlayUIForRequest(GetFrontRequestForWebState(old_web_state));
-  }
+  WebStateRemovedFromBrowser(old_web_state);
+  WebStateAddedToBrowser(new_web_state);
 }
 
 void OverlayPresenterImpl::WillDetachWebStateAt(WebStateList* web_state_list,
                                                 web::WebState* web_state,
                                                 int index) {
-  StopObservingWebState(web_state);
-  detaching_active_web_state_ = web_state == active_web_state_;
-  if (!detaching_active_web_state_) {
-    // If the active WebState is being detached, its overlay UI will be
-    // cancelled later when the active WebState is reset.  For inactive WebState
-    // replacements, the overlay UI can be cancelled immediately.
-    CancelOverlayUIForRequest(GetFrontRequestForWebState(web_state));
-  }
+  detaching_presenting_web_state_ =
+      presented_request_ ? presented_request_->GetQueueWebState() == web_state
+                         : false;
+  WebStateRemovedFromBrowser(web_state);
 }
 
 void OverlayPresenterImpl::WebStateActivatedAt(

ios/chrome/browser/overlays/overlay_presenter_impl_unittest.mm

@@ -391,6 +391,79 @@ TEST_F(OverlayPresenterImplTest, RemoveActiveWebState) {
             presentation_context().GetPresentationState(request));
 }
 
+// Tests detaching the active WebState while it is presenting an overlay and
+// removing the presenter as the queue's delegate.
+TEST_F(OverlayPresenterImplTest, DetachWebStateRemoveDelegate) {
+  // Add a WebState to the list and add a request to that WebState's queue.
+  presenter().SetPresentationContext(&presentation_context());
+  web_state_list().InsertWebState(
+      /*index=*/0, std::make_unique<web::TestWebState>(),
+      WebStateList::InsertionFlags::INSERT_ACTIVATE, WebStateOpener());
+  web::WebState* web_state = active_web_state();
+  OverlayRequest* request = AddRequest(web_state);
+  EXPECT_EQ(FakeOverlayPresentationContext::PresentationState::kPresented,
+            presentation_context().GetPresentationState(request));
+  EXPECT_TRUE(presenter().IsShowingOverlayUI());
+
+  presentation_context().SetDismissalCallbacksEnabled(false);
+  // Remove the WebState and verify that its overlay was cancelled.
+  std::unique_ptr<web::WebState> detached_web_state =
+      web_state_list().DetachWebStateAt(/*index=*/0);
+  EXPECT_CALL(observer(), DidHideOverlay(&presenter(), request));
+  GetQueueForWebState(detached_web_state.get())->SetDelegate(nullptr);
+  presentation_context().SetDismissalCallbacksEnabled(true);
+  EXPECT_EQ(FakeOverlayPresentationContext::PresentationState::kCancelled,
+            presentation_context().GetPresentationState(request));
+}
+
+// Tests detaching the active WebState while it is presenting an overlay and
+// cancelling all requests before the dismissal callback for the presenting
+// overlay executes.
+TEST_F(OverlayPresenterImplTest,
+       DetachWebStateCancelRequestBeforeDismissalCallback) {
+  // Add a WebState to the list and add a request to that WebState's queue.
+  presenter().SetPresentationContext(&presentation_context());
+  web_state_list().InsertWebState(
+      /*index=*/0, std::make_unique<web::TestWebState>(),
+      WebStateList::InsertionFlags::INSERT_ACTIVATE, WebStateOpener());
+  web::WebState* web_state = active_web_state();
+  OverlayRequest* request = AddRequest(web_state);
+  EXPECT_EQ(FakeOverlayPresentationContext::PresentationState::kPresented,
+            presentation_context().GetPresentationState(request));
+  EXPECT_TRUE(presenter().IsShowingOverlayUI());
+
+  presentation_context().SetDismissalCallbacksEnabled(false);
+  // Remove the WebState and verify that its overlay was cancelled.
+  std::unique_ptr<web::WebState> detached_web_state =
+      web_state_list().DetachWebStateAt(/*index=*/0);
+  GetQueueForWebState(detached_web_state.get())->CancelAllRequests();
+  EXPECT_CALL(observer(), DidHideOverlay(&presenter(), request));
+  presentation_context().SetDismissalCallbacksEnabled(true);
+}
+
+// Tests detaching the active WebState while it is presenting an overlay and
+// executing the dismissal callback after detachment.
+TEST_F(OverlayPresenterImplTest,
+       DetachWebStateDismissalCallbackCallsDidHideOverlay) {
+  // Add a WebState to the list and add a request to that WebState's queue.
+  presenter().SetPresentationContext(&presentation_context());
+  web_state_list().InsertWebState(
+      /*index=*/0, std::make_unique<web::TestWebState>(),
+      WebStateList::InsertionFlags::INSERT_ACTIVATE, WebStateOpener());
+  web::WebState* web_state = active_web_state();
+  OverlayRequest* request = AddRequest(web_state);
+  EXPECT_EQ(FakeOverlayPresentationContext::PresentationState::kPresented,
+            presentation_context().GetPresentationState(request));
+  EXPECT_TRUE(presenter().IsShowingOverlayUI());
+
+  presentation_context().SetDismissalCallbacksEnabled(false);
+  // Remove the WebState and verify that its overlay was cancelled.
+  std::unique_ptr<web::WebState> detached_web_state =
+      web_state_list().DetachWebStateAt(/*index=*/0);
+  EXPECT_CALL(observer(), DidHideOverlay(&presenter(), request));
+  presentation_context().SetDismissalCallbacksEnabled(true);
+}
+
 // Tests dismissing an overlay for user interaction.
 TEST_F(OverlayPresenterImplTest, DismissForUserInteraction) {
   // Add a WebState to the list and add two request to that WebState's queue.

ios/chrome/browser/overlays/overlay_request_queue_impl.h

@@ -50,6 +50,9 @@ class OverlayRequestQueueImpl : public OverlayRequestQueue {
     virtual void OverlayRequestRemoved(OverlayRequestQueueImpl* queue,
                                        std::unique_ptr<OverlayRequest> request,
                                        bool cancelled) = 0;
+    // Called when the queue is about to replace the existing delegate.
+    virtual void OverlayRequestQueueWillReplaceDelegate(
+        OverlayRequestQueueImpl* queue) = 0;
   };
 
   // Observer class for the queue.
@@ -69,7 +72,7 @@ class OverlayRequestQueueImpl : public OverlayRequestQueue {
                                                OverlayModality modality);
 
   // Sets the delegate.
-  void set_delegate(Delegate* delegate) { delegate_ = delegate; }
+  void SetDelegate(Delegate* delegate);
 
   // Adds and removes observers.
   void AddObserver(Observer* observer);

ios/chrome/browser/overlays/overlay_request_queue_impl.mm

@@ -63,6 +63,14 @@ OverlayRequestQueueImpl::~OverlayRequestQueueImpl() {
 
 #pragma mark Public
 
+void OverlayRequestQueueImpl::SetDelegate(Delegate* delegate) {
+  if (delegate_ == delegate)
+    return;
+  if (delegate_)
+    delegate_->OverlayRequestQueueWillReplaceDelegate(this);
+  delegate_ = delegate;
+}
+
 void OverlayRequestQueueImpl::AddObserver(Observer* observer) {
   observers_.AddObserver(observer);
 }

ios/chrome/browser/overlays/overlay_request_queue_impl_unittest.mm

@@ -33,6 +33,9 @@ class FakeOverlayRequestQueueImplDelegate
     removed_requests_.emplace_back(std::move(request), cancelled);
   }
 
+  void OverlayRequestQueueWillReplaceDelegate(
+      OverlayRequestQueueImpl* queue) override {}
+
   // Whether |request| was removed from the queue.
   bool WasRequestRemoved(OverlayRequest* request) {
     return GetRemovedRequestStorage(request) != nullptr;
@@ -102,12 +105,12 @@ class OverlayRequestQueueImplTest : public PlatformTest {
   OverlayRequestQueueImplTest()
       : PlatformTest(), web_state_(std::make_unique<web::TestWebState>()) {
     OverlayRequestQueueImpl::Container::CreateForWebState(web_state_.get());
-    queue()->set_delegate(&delegate_);
+    queue()->SetDelegate(&delegate_);
     queue()->AddObserver(&observer_);
   }
   ~OverlayRequestQueueImplTest() override {
     if (web_state_) {
-      queue()->set_delegate(nullptr);
+      queue()->SetDelegate(nullptr);
       queue()->RemoveObserver(&observer_);
     }
   }