Do not access web state while the CRWWebController is being destroyed.

Bug: 935555
Change-Id: I3ace36ecc6ec84205b90edb57de6c2c993201bec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1489713
Commit-Queue: Mike Dougherty <[email protected]>
Reviewed-by: Eugene But <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#639194}(cherry picked from commit 43bb9fdd8e623691e6529987777ebe8a5a2ab365)
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1518894
Reviewed-by: Mike Dougherty <[email protected]>
Cr-Commit-Position: refs/branch-heads/3729@{#53}
Cr-Branched-From: d4a8972-refs/heads/master@{#638880}

Author: michaeldo1

ios/web/public/test/fakes/BUILD.gn

@@ -42,6 +42,8 @@ source_set("fakes") {
     "fake_navigation_context.mm",
     "fake_web_frame.cc",
     "fake_web_frame.h",
+    "fake_web_state_policy_decider.h",
+    "fake_web_state_policy_decider.mm",
     "test_browser_state.cc",
     "test_browser_state.h",
     "test_java_script_dialog_presenter.h",

ios/web/public/test/fakes/fake_web_state_policy_decider.h

@@ -0,0 +1,39 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_WEB_PUBLIC_TEST_FAKES_FAKE_WEB_STATE_POLICY_DECIDER_H_
+#define IOS_WEB_PUBLIC_TEST_FAKES_FAKE_WEB_STATE_POLICY_DECIDER_H_
+
+#import "ios/web/public/web_state/web_state.h"
+#import "ios/web/public/web_state/web_state_policy_decider.h"
+
+@class NSURLRequest;
+@class NSURLResponse;
+
+namespace web {
+
+class FakeWebStatePolicyDecider : public WebStatePolicyDecider {
+ public:
+  explicit FakeWebStatePolicyDecider(WebState* web_state);
+  ~FakeWebStatePolicyDecider() override = default;
+
+  // Sets the value returned from |ShouldAllowRequest|.
+  void SetShouldAllowRequest(bool should_allow_request);
+
+  // WebStatePolicyDecider overrides
+  // Returns the value set with |SetShouldAllowRequest|. Defaults to true.
+  bool ShouldAllowRequest(NSURLRequest* request,
+                          const RequestInfo& request_info) override;
+  // Always returns true to allow |response|.
+  bool ShouldAllowResponse(NSURLResponse* response,
+                           bool for_main_frame) override;
+  void WebStateDestroyed() override {}
+
+ private:
+  bool should_allow_request_ = true;
+};
+
+}  // namespace web
+
+#endif  // IOS_WEB_PUBLIC_TEST_FAKES_FAKE_WEB_STATE_POLICY_DECIDER_H_

ios/web/public/test/fakes/fake_web_state_policy_decider.mm

@@ -0,0 +1,32 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/web/public/test/fakes/fake_web_state_policy_decider.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+namespace web {
+
+FakeWebStatePolicyDecider::FakeWebStatePolicyDecider(WebState* web_state)
+    : WebStatePolicyDecider(web_state) {}
+
+void FakeWebStatePolicyDecider::SetShouldAllowRequest(
+    bool should_allow_request) {
+  should_allow_request_ = should_allow_request;
+}
+
+bool FakeWebStatePolicyDecider::ShouldAllowRequest(
+    NSURLRequest* request,
+    const RequestInfo& request_info) {
+  return should_allow_request_;
+}
+
+bool FakeWebStatePolicyDecider::ShouldAllowResponse(NSURLResponse* response,
+                                                    bool for_main_frame) {
+  return true;
+}
+
+}  // namespace web

ios/web/public/test/web_test_with_web_state.h

@@ -22,6 +22,10 @@ class WebState;
 // Base test fixture that provides WebState for testing.
 class WebTestWithWebState : public WebTest,
                             public base::MessageLoop::TaskObserver {
+ public:
+  // Destroys underlying WebState. web_state() will return null after this call.
+  void DestroyWebState();
+
  protected:
   explicit WebTestWithWebState(
       TestWebThreadBundle::Options = TestWebThreadBundle::Options::DEFAULT);
@@ -60,8 +64,6 @@ class WebTestWithWebState : public WebTest,
   void WaitForCondition(ConditionBlock condition);
   // Synchronously executes JavaScript and returns result as id.
   id ExecuteJavaScript(NSString* script);
-  // Destroys underlying WebState. web_state() will return null after this call.
-  void DestroyWebState();
 
   // Returns the base URL of the loaded page.
   std::string BaseUrl() const;

ios/web/test/fakes/BUILD.gn

@@ -23,6 +23,8 @@ source_set("fakes") {
     "crw_fake_nsurl_session_task.mm",
     "crw_fake_session_controller_delegate.h",
     "crw_fake_session_controller_delegate.mm",
+    "crw_fake_wk_frame_info.h",
+    "crw_fake_wk_frame_info.mm",
     "crw_fake_wk_navigation_action.h",
     "crw_fake_wk_navigation_action.mm",
     "crw_fake_wk_navigation_response.h",

ios/web/test/fakes/crw_fake_back_forward_list.h

@@ -22,6 +22,10 @@ NS_ASSUME_NONNULL_BEGIN
 @property(nullable, nonatomic, copy)
     NSArray<WKBackForwardListItem*>* forwardList;
 @property(nullable, nonatomic, strong) WKBackForwardListItem* currentItem;
+@property(nullable, nonatomic, readonly, strong)
+    WKBackForwardListItem* backItem;
+@property(nullable, nonatomic, readonly, strong)
+    WKBackForwardListItem* forwardItem;
 - (nullable WKBackForwardListItem*)itemAtIndex:(NSInteger)index;
 
 // Resets this instance to simulate a session with no back/forward history, and

ios/web/test/fakes/crw_fake_back_forward_list.mm

@@ -87,4 +87,12 @@ - (NSArray*)mockSublistWithURLArray:(NSArray<NSString*>*)URLs {
   return [NSArray arrayWithArray:array];
 }
 
+- (WKBackForwardListItem*)backItem {
+  return self.backList.lastObject;
+}
+
+- (WKBackForwardListItem*)forwardItem {
+  return self.forwardList.firstObject;
+}
+
 @end

ios/web/test/fakes/crw_fake_wk_frame_info.h

@@ -0,0 +1,19 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_WEB_TEST_FAKES_CRW_FAKE_WK_FRAME_INFO_H_
+#define IOS_WEB_TEST_FAKES_CRW_FAKE_WK_FRAME_INFO_H_
+
+#import <WebKit/WebKit.h>
+
+// Fake WKFrameInfo class which can be used for testing.
+@interface CRWFakeWKFrameInfo : WKFrameInfo
+// Redefined WKNavigationAction properties as readwrite.
+@property(nonatomic, readwrite, getter=isMainFrame) BOOL mainFrame;
+@property(nonatomic, readwrite, copy) NSURLRequest* request;
+@property(nonatomic, readwrite) WKSecurityOrigin* securityOrigin;
+@property(nonatomic, readwrite, weak) WKWebView* webView;
+@end
+
+#endif  // IOS_WEB_TEST_FAKES_CRW_FAKE_WK_FRAME_INFO_H_

ios/web/test/fakes/crw_fake_wk_frame_info.mm

@@ -0,0 +1,16 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/web/test/fakes/crw_fake_wk_frame_info.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+@implementation CRWFakeWKFrameInfo
+@synthesize mainFrame = _mainFrame;
+@synthesize request = _request;
+@synthesize securityOrigin = _securityOrigin;
+@synthesize webView = _webView;
+@end

ios/web/test/web_test_with_web_controller.h

@@ -18,7 +18,7 @@ class WebTestWithWebController : public WebTestWithWebState {
   WebTestWithWebController();
   explicit WebTestWithWebController(std::unique_ptr<web::WebClient> web_client);
   ~WebTestWithWebController() override;
-  // Returns web controller for testing.
+  // Returns web controller for testing or null if |web_state()| is null.
   CRWWebController* web_controller();
 };
 

ios/web/test/web_test_with_web_controller.mm

@@ -22,6 +22,9 @@
 WebTestWithWebController::~WebTestWithWebController() {}
 
 CRWWebController* WebTestWithWebController::web_controller() {
+  if (!web_state()) {
+    return nullptr;
+  }
   return static_cast<web::WebStateImpl*>(web_state())->GetWebController();
 }
 

ios/web/web_state/ui/crw_web_controller.mm

@@ -4480,6 +4480,11 @@ - (void)webView:(WKWebView*)webView
 
     allowLoad =
         self.webStateImpl->ShouldAllowRequest(action.request, requestInfo);
+    // The WebState may have been closed in the ShouldAllowRequest callback.
+    if (_isBeingDestroyed) {
+      decisionHandler(WKNavigationActionPolicyCancel);
+      return;
+    }
   }
 
   if (allowLoad) {
@@ -4499,8 +4504,17 @@ - (void)webView:(WKWebView*)webView
       // inserting the webview.
       [self discardNonCommittedItemsIfLastCommittedWasNotNativeView];
 
-      if (!_isBeingDestroyed && [self shouldClosePageOnNativeApplicationLoad])
+      if (!_isBeingDestroyed && [self shouldClosePageOnNativeApplicationLoad]) {
+        // Loading was started for user initiated navigations and should be
+        // stopped because no other WKWebView callbacks are called.
+        // TODO(crbug.com/767092): Loading should not start until
+        // webView.loading is changed to YES.
+        self.webStateImpl->SetIsLoading(false);
+
         self.webStateImpl->CloseWebState();
+        decisionHandler(WKNavigationActionPolicyCancel);
+        return;
+      }
     }
 
     if (!_isBeingDestroyed) {
@@ -4562,9 +4576,12 @@ - (void)webView:(WKWebView*)webView
         }));
   }
 
-  WKNavigationActionPolicy allowPolicy = web::GetAllowNavigationActionPolicy(
-      self.webState->GetBrowserState()->IsOffTheRecord());
-  decisionHandler(allowLoad ? allowPolicy : WKNavigationActionPolicyCancel);
+  if (!allowLoad) {
+    decisionHandler(WKNavigationActionPolicyCancel);
+    return;
+  }
+  BOOL isOffTheRecord = self.webState->GetBrowserState()->IsOffTheRecord();
+  decisionHandler(web::GetAllowNavigationActionPolicy(isOffTheRecord));
 }
 
 - (void)webView:(WKWebView*)webView

ios/web/web_state/ui/crw_web_controller_unittest.mm

@@ -26,6 +26,7 @@
 #include "ios/web/public/features.h"
 #include "ios/web/public/referrer.h"
 #include "ios/web/public/test/fakes/fake_download_controller_delegate.h"
+#import "ios/web/public/test/fakes/fake_web_state_policy_decider.h"
 #include "ios/web/public/test/fakes/test_browser_state.h"
 #import "ios/web/public/test/fakes/test_native_content.h"
 #import "ios/web/public/test/fakes/test_native_content_provider.h"
@@ -41,12 +42,14 @@
 #include "ios/web/public/web_state/url_verification_constants.h"
 #include "ios/web/public/web_state/web_state_observer.h"
 #import "ios/web/test/fakes/crw_fake_back_forward_list.h"
+#import "ios/web/test/fakes/crw_fake_wk_frame_info.h"
 #import "ios/web/test/fakes/crw_fake_wk_navigation_action.h"
 #import "ios/web/test/fakes/crw_fake_wk_navigation_response.h"
 #include "ios/web/test/test_url_constants.h"
 #import "ios/web/test/web_test_with_web_controller.h"
 #import "ios/web/test/wk_web_view_crash_utils.h"
 #include "ios/web/web_state/ui/block_universal_links_buildflags.h"
+#import "ios/web/web_state/ui/crw_web_controller.h"
 #import "ios/web/web_state/ui/crw_web_controller_container_view.h"
 #import "ios/web/web_state/ui/web_view_js_utils.h"
 #import "ios/web/web_state/ui/wk_navigation_action_policy_util.h"
@@ -224,6 +227,8 @@ void SetWebViewURL(NSString* url_string) {
     OCMStub([result setAllowsBackForwardNavigationGestures:NO]);
     OCMStub([result setAllowsBackForwardNavigationGestures:YES]);
     OCMStub([result isLoading]);
+    OCMStub([result stopLoading]);
+    OCMStub([result removeFromSuperview]);
 
     return result;
   }
@@ -772,6 +777,10 @@ bool VerifyDecidePolicyForNavigationAction(
         [[CRWFakeWKNavigationAction alloc] init];
     navigation_action.request = request;
 
+    CRWFakeWKFrameInfo* frame_info = [[CRWFakeWKFrameInfo alloc] init];
+    frame_info.mainFrame = YES;
+    navigation_action.targetFrame = frame_info;
+
     // Call decidePolicyForNavigationResponse and wait for decisionHandler's
     // callback.
     __block bool policy_match = false;
@@ -867,6 +876,65 @@ bool VerifyDecidePolicyForNavigationAction(
       blob_url_request, WKNavigationActionPolicyAllow));
 }
 
+// Tests that navigations which close the WebState cancels the navigation.
+// This occurs, for example, when a new page is opened with a link that is
+// handled by a native application.
+TEST_P(CRWWebControllerPolicyDeciderTest, ClosedWebState) {
+  static CRWWebControllerPolicyDeciderTest* test_fixture = nullptr;
+  test_fixture = this;
+  class FakeWebStateDelegate : public TestWebStateDelegate {
+   public:
+    void CloseWebState(WebState* source) override {
+      test_fixture->DestroyWebState();
+    }
+  };
+  FakeWebStateDelegate delegate;
+  web_state()->SetDelegate(&delegate);
+
+  FakeWebStatePolicyDecider policy_decider(web_state());
+  policy_decider.SetShouldAllowRequest(false);
+
+  NSURL* url =
+      [NSURL URLWithString:@"https://itunes.apple.com/us/album/american-radio/"
+                           @"1449089454?fbclid=IwAR2NKLvDGH_YY4uZbU7Cj_"
+                           @"e3h7q7DvORCI4Edvi1K9LjcUHfYObmOWl-YgE"];
+  NSMutableURLRequest* url_request = [NSMutableURLRequest requestWithURL:url];
+  EXPECT_TRUE(VerifyDecidePolicyForNavigationAction(
+      url_request, WKNavigationActionPolicyCancel));
+}
+
+// Tests that navigations are cancelled if the web state is closed in
+// |ShouldAllowRequest|.
+TEST_P(CRWWebControllerPolicyDeciderTest, ClosedWebStateInShouldAllowRequest) {
+  static CRWWebControllerPolicyDeciderTest* test_fixture = nullptr;
+  test_fixture = this;
+
+  class TestWebStatePolicyDecider : public WebStatePolicyDecider {
+   public:
+    explicit TestWebStatePolicyDecider(WebState* web_state)
+        : WebStatePolicyDecider(web_state) {}
+    ~TestWebStatePolicyDecider() override = default;
+
+    // WebStatePolicyDecider overrides
+    bool ShouldAllowRequest(NSURLRequest* request,
+                            const RequestInfo& request_info) override {
+      test_fixture->DestroyWebState();
+      return true;
+    }
+    bool ShouldAllowResponse(NSURLResponse* response,
+                             bool for_main_frame) override {
+      return true;
+    }
+    void WebStateDestroyed() override {}
+  };
+  TestWebStatePolicyDecider policy_decider(web_state());
+
+  NSURL* url = [NSURL URLWithString:@(kTestURLString)];
+  NSMutableURLRequest* url_request = [NSMutableURLRequest requestWithURL:url];
+  EXPECT_TRUE(VerifyDecidePolicyForNavigationAction(
+      url_request, WKNavigationActionPolicyCancel));
+}
+
 INSTANTIATE_TEST_SUITES(CRWWebControllerPolicyDeciderTest);
 
 // Test fixture for testing CRWWebController presenting native content.