[iOS] Cancel OverlayRequests for navigation events.

- Adds OverlayPresenter::Observer::RequestCancelledByQueue().
  OverlayPresenterImpl uses this callback to cancel overlay UI for
  requests cancelled by the queue.
- Adds OverlayRequestQueueImpl::RequestCanceller, which uses
  WebStateObserver callbacks to cancel requests in a queue for
  navigation events.

Bug: 941745
Change-Id: Ia5f78e22fe4a0397aa1530bf41cb0fb1b434e398
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1598287
Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Mike Dougherty <michaeldo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659524}

ios/chrome/browser/overlays/overlay_presenter_impl.h

@@ -81,10 +81,10 @@ class OverlayPresenterImpl : public BrowserObserver,
   // delegate is executed.
   void OverlayWasDismissed(UIDelegate* ui_delegate,
                            OverlayRequest* request,
-                           OverlayRequestQueueImpl* queue,
+                           base::WeakPtr<OverlayRequestQueueImpl> queue,
                            OverlayDismissalReason reason);
 
-  // Cancels all overlays for |queue|.
+  // Cancels all overlays for |request|.
   void CancelOverlayUIForRequest(OverlayRequest* request);
 
   // Cancels all overlays for the Browser.
@@ -96,6 +96,8 @@ class OverlayPresenterImpl : public BrowserObserver,
   // OverlayRequestQueueImpl::Observer:
   void RequestAddedToQueue(OverlayRequestQueueImpl* queue,
                            OverlayRequest* request) override;
+  void QueuedRequestCancelled(OverlayRequestQueueImpl* queue,
+                              OverlayRequest* request) override;
 
   // WebStateListObserver:
   void WebStateInsertedAt(WebStateList* web_state_list,
@@ -115,6 +117,13 @@ class OverlayPresenterImpl : public BrowserObserver,
                            int active_index,
                            int reason) override;
 
+  // Whether the UI delegate is presenting overlay UI for this presenter.  Stays
+  // true from the beginning of the presentation until the end of the
+  // dismissal.
+  bool presenting_ = false;
+  // Whether the active WebState is being detached.
+  bool detaching_active_web_state_ = false;
+
   OverlayModality modality_;
   WebStateList* web_state_list_ = nullptr;
   web::WebState* active_web_state_ = nullptr;
@@ -122,8 +131,6 @@ class OverlayPresenterImpl : public BrowserObserver,
   base::ObserverList<OverlayPresenter::Observer,
                      /* check_empty= */ true>
       observers_;
-  bool presenting_ = false;
-  bool detaching_active_web_state_ = false;
   base::WeakPtrFactory<OverlayPresenterImpl> weak_factory_;
 };
 

ios/chrome/browser/overlays/overlay_presenter_impl.mm

@@ -184,14 +184,15 @@ void OverlayPresenterImpl::PresentOverlayForActiveRequest() {
 
   OverlayDismissalCallback dismissal_callback = base::BindOnce(
       &OverlayPresenterImpl::OverlayWasDismissed, weak_factory_.GetWeakPtr(),
-      ui_delegate_, request, GetActiveQueue());
+      ui_delegate_, request, GetActiveQueue()->GetWeakPtr());
   ui_delegate_->ShowOverlayUI(this, request, std::move(dismissal_callback));
 }
 
-void OverlayPresenterImpl::OverlayWasDismissed(UIDelegate* ui_delegate,
-                                               OverlayRequest* request,
-                                               OverlayRequestQueueImpl* queue,
-                                               OverlayDismissalReason reason) {
+void OverlayPresenterImpl::OverlayWasDismissed(
+    UIDelegate* ui_delegate,
+    OverlayRequest* request,
+    base::WeakPtr<OverlayRequestQueueImpl> queue,
+    OverlayDismissalReason reason) {
   // If the UI delegate is reset while presenting an overlay, that overlay will
   // be cancelled and dismissed.  The presenter is now using the new UI
   // delegate's presentation context, so this dismissal should not trigger
@@ -199,13 +200,14 @@ void OverlayPresenterImpl::OverlayWasDismissed(UIDelegate* ui_delegate,
   if (ui_delegate_ != ui_delegate)
     return;
 
-  // If the overlay was dismissed for user interaction, pop it from the queue
-  // since it will never be shown again.
-  // TODO(crbug.com/941745): Prevent the queue from being accessed if deleted.
-  // This is possible if a WebState is closed during an overlay dismissal
-  // animation triggered by user interaction.
-  if (reason == OverlayDismissalReason::kUserInteraction)
+  // Pop the request for overlays dismissed by the user.  The check against the
+  // queue's front request prevents popping the request twice in the event that
+  // the front request was cancelled by the queue during a user-triggered
+  // dismissal.
+  if (reason == OverlayDismissalReason::kUserInteraction && queue &&
+      queue->front_request() == request) {
     queue->PopFrontRequest();
+  }
 
   presenting_ = false;
 
@@ -257,7 +259,13 @@ void OverlayPresenterImpl::RequestAddedToQueue(OverlayRequestQueueImpl* queue,
     PresentOverlayForActiveRequest();
 }
 
-#pragma mark WebStateListObserver
+void OverlayPresenterImpl::QueuedRequestCancelled(
+    OverlayRequestQueueImpl* queue,
+    OverlayRequest* request) {
+  CancelOverlayUIForRequest(request);
+}
+
+#pragma mark - WebStateListObserver
 
 void OverlayPresenterImpl::WebStateInsertedAt(WebStateList* web_state_list,
                                               web::WebState* web_state,

ios/chrome/browser/overlays/overlay_presenter_impl_unittest.mm

@@ -272,3 +272,25 @@ TEST_F(OverlayPresenterImplTest, DismissForUserInteraction) {
   EXPECT_EQ(second_request, queue->front_request());
   EXPECT_EQ(1U, queue->size());
 }
+
+// Tests cancelling the requests.
+TEST_F(OverlayPresenterImplTest, CancelRequests) {
+  // Add a WebState to the list and a request to that WebState's queue.
+  presenter().SetUIDelegate(&ui_delegate());
+  web_state_list().InsertWebState(0, std::make_unique<web::TestWebState>(),
+                                  WebStateList::InsertionFlags::INSERT_ACTIVATE,
+                                  WebStateOpener());
+  OverlayRequestQueueImpl* queue = GetQueueForWebState(active_web_state());
+  OverlayRequest* active_request = AddRequest(active_web_state());
+  OverlayRequest* queued_request = AddRequest(active_web_state());
+
+  EXPECT_EQ(FakeOverlayPresenterUIDelegate::PresentationState::kPresented,
+            ui_delegate().GetPresentationState(active_request));
+
+  // Cancel the queue's requests and verify that the UI is also cancelled.
+  queue->CancelAllRequests();
+  EXPECT_EQ(FakeOverlayPresenterUIDelegate::PresentationState::kCancelled,
+            ui_delegate().GetPresentationState(active_request));
+  EXPECT_EQ(FakeOverlayPresenterUIDelegate::PresentationState::kCancelled,
+            ui_delegate().GetPresentationState(queued_request));
+}

ios/chrome/browser/overlays/overlay_request_queue_impl.h

@@ -9,10 +9,12 @@
 #include <memory>
 
 #include "base/containers/circular_deque.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/observer_list_types.h"
 #include "ios/chrome/browser/overlays/public/overlay_modality.h"
 #import "ios/chrome/browser/overlays/public/overlay_request_queue.h"
+#import "ios/web/public/web_state/web_state_observer.h"
 #import "ios/web/public/web_state/web_state_user_data.h"
 
 // Mutable implementation of OverlayRequestQueue.
@@ -35,6 +37,7 @@ class OverlayRequestQueueImpl : public OverlayRequestQueue {
     WEB_STATE_USER_DATA_KEY_DECL();
     Container(web::WebState* web_state);
 
+    web::WebState* web_state_ = nullptr;
     std::map<OverlayModality, std::unique_ptr<OverlayRequestQueueImpl>> queues_;
   };
 
@@ -44,12 +47,22 @@ class OverlayRequestQueueImpl : public OverlayRequestQueue {
     // Called after |request| has been added to |queue|.
     virtual void RequestAddedToQueue(OverlayRequestQueueImpl* queue,
                                      OverlayRequest* request) {}
+
+    // Called when |request| is cancelled before it is removed from |queue|.
+    // All requests in a queue are cancelled before the queue is destroyed.
+    virtual void QueuedRequestCancelled(OverlayRequestQueueImpl* queue,
+                                        OverlayRequest* request) {}
   };
 
   // Adds and removes observers.
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
+  // Returns a weak pointer to the queue.
+  base::WeakPtr<OverlayRequestQueueImpl> GetWeakPtr();
+
+  // Whether the queue is empty.
+  bool empty() const { return requests_.empty(); }
   // The number of requests in the queue.
   size_t size() const { return requests_.size(); }
 
@@ -58,18 +71,40 @@ class OverlayRequestQueueImpl : public OverlayRequestQueue {
   void PopFrontRequest();
   void PopBackRequest();
 
+  // Cancels the UI for all requests in the queue then empties the queue.
+  void CancelAllRequests();
+
   // OverlayRequestQueue:
   void AddRequest(std::unique_ptr<OverlayRequest> request) override;
   OverlayRequest* front_request() const override;
 
  private:
   // Private constructor called by container.
-  OverlayRequestQueueImpl();
+  explicit OverlayRequestQueueImpl(web::WebState* web_state);
+
+  // Helper object that cancels requests for navigation events.
+  class RequestCancellationHelper : public web::WebStateObserver {
+   public:
+    RequestCancellationHelper(OverlayRequestQueueImpl* queue,
+                              web::WebState* web_state);
+
+   private:
+    // web::WebStateObserver:
+    void DidFinishNavigation(
+        web::WebState* web_state,
+        web::NavigationContext* navigation_context) override;
+    void RenderProcessGone(web::WebState* web_state) override;
+    void WebStateDestroyed(web::WebState* web_state) override;
+
+    OverlayRequestQueueImpl* queue_ = nullptr;
+  };
 
+  RequestCancellationHelper cancellation_helper_;
   base::ObserverList<Observer, /* check_empty= */ true> observers_;
   // The queue used to hold the received requests.  Stored as a circular dequeue
   // to allow performant pop events from the front of the queue.
   base::circular_deque<std::unique_ptr<OverlayRequest>> requests_;
+  base::WeakPtrFactory<OverlayRequestQueueImpl> weak_factory_;
 };
 
 #endif  // IOS_CHROME_BROWSER_OVERLAYS_OVERLAY_REQUEST_QUEUE_IMPL_H_

ios/chrome/browser/overlays/overlay_request_queue_impl.mm

@@ -9,6 +9,7 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "ios/chrome/browser/overlays/public/overlay_request.h"
+#import "ios/web/public/web_state/navigation_context.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
@@ -28,20 +29,22 @@ OverlayRequestQueue* OverlayRequestQueue::FromWebState(
 
 WEB_STATE_USER_DATA_KEY_IMPL(OverlayRequestQueueImpl::Container)
 
-OverlayRequestQueueImpl::Container::Container(web::WebState* web_state) {}
+OverlayRequestQueueImpl::Container::Container(web::WebState* web_state)
+    : web_state_(web_state) {}
 OverlayRequestQueueImpl::Container::~Container() = default;
 
 OverlayRequestQueueImpl* OverlayRequestQueueImpl::Container::QueueForModality(
     OverlayModality modality) {
   auto& queue = queues_[modality];
   if (!queue)
-    queue = base::WrapUnique(new OverlayRequestQueueImpl());
+    queue = base::WrapUnique(new OverlayRequestQueueImpl(web_state_));
   return queue.get();
 }
 
 #pragma mark - OverlayRequestQueueImpl
 
-OverlayRequestQueueImpl::OverlayRequestQueueImpl() = default;
+OverlayRequestQueueImpl::OverlayRequestQueueImpl(web::WebState* web_state)
+    : cancellation_helper_(this, web_state), weak_factory_(this) {}
 OverlayRequestQueueImpl::~OverlayRequestQueueImpl() = default;
 
 #pragma mark Public
@@ -54,6 +57,10 @@ void OverlayRequestQueueImpl::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
+base::WeakPtr<OverlayRequestQueueImpl> OverlayRequestQueueImpl::GetWeakPtr() {
+  return weak_factory_.GetWeakPtr();
+}
+
 void OverlayRequestQueueImpl::PopFrontRequest() {
   DCHECK(!requests_.empty());
   requests_.pop_front();
@@ -64,6 +71,17 @@ void OverlayRequestQueueImpl::PopBackRequest() {
   requests_.pop_back();
 }
 
+void OverlayRequestQueueImpl::CancelAllRequests() {
+  while (!empty()) {
+    // Requests are cancelled in reverse order to prevent attempting to present
+    // subsequent requests after the dismissal of the front request's UI.
+    for (auto& observer : observers_) {
+      observer.QueuedRequestCancelled(this, requests_.back().get());
+    }
+    PopBackRequest();
+  }
+}
+
 #pragma mark OverlayRequestQueue
 
 void OverlayRequestQueueImpl::AddRequest(
@@ -77,3 +95,31 @@ void OverlayRequestQueueImpl::AddRequest(
 OverlayRequest* OverlayRequestQueueImpl::front_request() const {
   return requests_.empty() ? nullptr : requests_.front().get();
 }
+
+#pragma mark RequestCancellationHelper
+
+OverlayRequestQueueImpl::RequestCancellationHelper::RequestCancellationHelper(
+    OverlayRequestQueueImpl* queue,
+    web::WebState* web_state)
+    : queue_(queue) {
+  web_state->AddObserver(this);
+}
+
+void OverlayRequestQueueImpl::RequestCancellationHelper::DidFinishNavigation(
+    web::WebState* web_state,
+    web::NavigationContext* navigation_context) {
+  if (navigation_context->HasCommitted() &&
+      !navigation_context->IsSameDocument()) {
+    queue_->CancelAllRequests();
+  }
+}
+
+void OverlayRequestQueueImpl::RequestCancellationHelper::RenderProcessGone(
+    web::WebState* web_state) {
+  queue_->CancelAllRequests();
+}
+
+void OverlayRequestQueueImpl::RequestCancellationHelper::WebStateDestroyed(
+    web::WebState* web_state) {
+  web_state->RemoveObserver(this);
+}

ios/chrome/browser/overlays/overlay_request_queue_impl_unittest.mm

@@ -24,6 +24,8 @@ class MockOverlayRequestQueueImplObserver
 
   MOCK_METHOD2(RequestAddedToQueue,
                void(OverlayRequestQueueImpl*, OverlayRequest*));
+  MOCK_METHOD2(QueuedRequestCancelled,
+               void(OverlayRequestQueueImpl*, OverlayRequest*));
 };
 }  // namespace
 
@@ -45,6 +47,15 @@ class OverlayRequestQueueImplTest : public PlatformTest {
   }
   MockOverlayRequestQueueImplObserver& observer() { return observer_; }
 
+  OverlayRequest* AddRequest() {
+    std::unique_ptr<OverlayRequest> passed_request =
+        OverlayRequest::CreateWithConfig<FakeOverlayUserData>(nullptr);
+    OverlayRequest* request = passed_request.get();
+    EXPECT_CALL(observer(), RequestAddedToQueue(queue(), request));
+    queue()->AddRequest(std::move(passed_request));
+    return request;
+  }
+
  private:
   web::TestWebState web_state_;
   MockOverlayRequestQueueImplObserver observer_;
@@ -53,22 +64,10 @@ class OverlayRequestQueueImplTest : public PlatformTest {
 // Tests that state is updated correctly and observer callbacks are received
 // when adding requests to the back of the queue.
 TEST_F(OverlayRequestQueueImplTest, AddRequest) {
-  std::unique_ptr<OverlayRequest> first_request =
-      OverlayRequest::CreateWithConfig<FakeOverlayUserData>(nullptr);
-  OverlayRequest* first_request_ptr = first_request.get();
-  std::unique_ptr<OverlayRequest> second_request =
-      OverlayRequest::CreateWithConfig<FakeOverlayUserData>(nullptr);
-  OverlayRequest* second_request_ptr = second_request.get();
-
-  // Add two requests and pop the first, verifying that the size and front
-  // requests are updated.
-  EXPECT_CALL(observer(), RequestAddedToQueue(queue(), first_request_ptr));
-  queue()->AddRequest(std::move(first_request));
-
-  EXPECT_CALL(observer(), RequestAddedToQueue(queue(), second_request_ptr));
-  queue()->AddRequest(std::move(second_request));
-
-  EXPECT_EQ(first_request_ptr, queue()->front_request());
+  OverlayRequest* first_request = AddRequest();
+  AddRequest();
+
+  EXPECT_EQ(first_request, queue()->front_request());
   EXPECT_EQ(2U, queue()->size());
 }
 
@@ -76,23 +75,9 @@ TEST_F(OverlayRequestQueueImplTest, AddRequest) {
 // when popping the requests.
 TEST_F(OverlayRequestQueueImplTest, PopRequests) {
   // Add three requests to the queue.
-  std::unique_ptr<OverlayRequest> passed_request =
-      OverlayRequest::CreateWithConfig<FakeOverlayUserData>(nullptr);
-  OverlayRequest* first_request = passed_request.get();
-  EXPECT_CALL(observer(), RequestAddedToQueue(queue(), first_request));
-  queue()->AddRequest(std::move(passed_request));
-
-  passed_request =
-      OverlayRequest::CreateWithConfig<FakeOverlayUserData>(nullptr);
-  OverlayRequest* second_request = passed_request.get();
-  EXPECT_CALL(observer(), RequestAddedToQueue(queue(), second_request));
-  queue()->AddRequest(std::move(passed_request));
-
-  passed_request =
-      OverlayRequest::CreateWithConfig<FakeOverlayUserData>(nullptr);
-  OverlayRequest* third_request = passed_request.get();
-  EXPECT_CALL(observer(), RequestAddedToQueue(queue(), third_request));
-  queue()->AddRequest(std::move(passed_request));
+  OverlayRequest* first_request = AddRequest();
+  OverlayRequest* second_request = AddRequest();
+  AddRequest();
 
   ASSERT_EQ(first_request, queue()->front_request());
   ASSERT_EQ(3U, queue()->size());
@@ -109,3 +94,19 @@ TEST_F(OverlayRequestQueueImplTest, PopRequests) {
   EXPECT_EQ(second_request, queue()->front_request());
   EXPECT_EQ(1U, queue()->size());
 }
+
+// Tests that state is updated correctly and observer callbacks are received
+// when popping the requests.
+TEST_F(OverlayRequestQueueImplTest, CancelAllRequests) {
+  // Add two requests to the queue then cancel all requests, verifying that
+  // the observer callback is received for each.
+  OverlayRequest* first_request = AddRequest();
+  OverlayRequest* second_request = AddRequest();
+
+  EXPECT_CALL(observer(), QueuedRequestCancelled(queue(), first_request));
+  EXPECT_CALL(observer(), QueuedRequestCancelled(queue(), second_request));
+  queue()->CancelAllRequests();
+
+  EXPECT_EQ(0U, queue()->size());
+  EXPECT_TRUE(queue()->empty());
+}

ios/chrome/browser/overlays/test/fake_overlay_presenter_ui_delegate.cc

@@ -54,6 +54,6 @@ void FakeOverlayPresenterUIDelegate::CancelOverlayUI(
   if (state == PresentationState::kPresented) {
     SimulateDismissalForRequest(request, OverlayDismissalReason::kCancellation);
   } else {
-    state = PresentationState::kNotPresented;
+    state = PresentationState::kCancelled;
   }
 }