[Nav Experiment] Skip ErrorRetryState update for unhandled errors.

ErrorRetryStateMachine is meant to manage the displaying of error pages
when a navigation fails and retrying the original URL when user
navigates back to an error page. SSL interstitial and other errors that
do not load error page (e.g. canceled navigation) should not modify an
item's ErrorRetryStateMachine to avoid trigger the error retry logic
when user navigates back to such an item.

Bug: 837210
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I8dfd11821dfb8cb9ee6f10820b88499c915d1994
Reviewed-on: https://chromium-review.googlesource.com/1151949
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579089}

ios/web/navigation/error_retry_state_machine.mm

@@ -29,7 +29,19 @@ ErrorRetryState ErrorRetryStateMachine::state() const {
 }
 
 void ErrorRetryStateMachine::SetDisplayingNativeError() {
-  DCHECK_EQ(ErrorRetryState::kReadyToDisplayErrorForFailedNavigation, state_);
+  // Native error is displayed in two scenarios:
+  // (1) Placeholder entry for network load error finished loading in web view.
+  //     This is the common case.
+  // (2) Retry of a previously failed load failed in SSL error. This can happen
+  //     when the first navigation failed in offline mode. SSL interstitial does
+  //     not normally trigger ErrorRetryStateMachine because the error page is
+  //     not to become part of the navigation history. This leaves the item
+  //     stuck in the transient kRetryFailedNavigationItem state. So for this
+  //     specific case, treat the SSL interstitial as a native error so that
+  //     error retry works as expected on subsequent back/forward navigations.
+  DCHECK(state_ == ErrorRetryState::kReadyToDisplayErrorForFailedNavigation ||
+         state_ == ErrorRetryState::kRetryFailedNavigationItem)
+      << "Unexpected error retry state: " << static_cast<int>(state_);
   state_ = ErrorRetryState::kDisplayingNativeErrorForFailedNavigation;
 }
 

ios/web/navigation/wk_based_navigation_manager_impl.mm

@@ -46,6 +46,12 @@ web::NavigationItemImpl* GetNavigationItemFromWKItem(
       navigationItem];
 }
 
+// Returns true if |url1| is the same as |url2| or is a placeholder of |url2|.
+bool IsSameOrPlaceholderOf(const GURL& url1, const GURL& url2) {
+  return url1 == url2 ||
+         url1 == web::wk_navigation_util::CreatePlaceholderUrlForUrl(url2);
+}
+
 }  // namespace
 
 namespace web {
@@ -153,16 +159,17 @@ void WKBasedNavigationManagerImpl::AddPendingItem(
   // item has already been updated to point to the new location in back-forward
   // history, so pending item index should be set to the current item index.
   // Similarly, current item should be reused when reloading a placeholder URL.
+  //
+  // WebErrorPages and URL rewriting in ErrorRetryStateMachine make it possible
+  // for the web view URL to be the target URL even though current item is the
+  // placeholder. This is taken into consideration when checking equivalence
+  // between the URLs.
   id<CRWWebViewNavigationProxy> proxy = delegate_->GetWebViewNavigationProxy();
   WKBackForwardListItem* current_wk_item = proxy.backForwardList.currentItem;
-  GURL current_item_url = net::GURLWithNSURL(current_wk_item.URL);
-  bool current_item_is_pending_item =
-      current_item_url == pending_item_->GetURL() ||
-      current_item_url == wk_navigation_util::CreatePlaceholderUrlForUrl(
-                              pending_item_->GetURL());
+  const GURL current_item_url = net::GURLWithNSURL(current_wk_item.URL);
   if (proxy.backForwardList.currentItem &&
-      current_item_url == net::GURLWithNSURL(proxy.URL) &&
-      current_item_is_pending_item) {
+      IsSameOrPlaceholderOf(current_item_url, pending_item_->GetURL()) &&
+      IsSameOrPlaceholderOf(current_item_url, net::GURLWithNSURL(proxy.URL))) {
     pending_item_index_ = web_view_cache_.GetCurrentItemIndex();
 
     // If |currentItem| is not already associated with a NavigationItemImpl,
@@ -294,6 +301,7 @@ NavigationItem* WKBasedNavigationManagerImpl::GetVisibleItem() const {
 void WKBasedNavigationManagerImpl::DiscardNonCommittedItems() {
   pending_item_.reset();
   transient_item_.reset();
+  pending_item_index_ = -1;
 }
 
 int WKBasedNavigationManagerImpl::GetItemCount() const {

ios/web/web_state/ui/crw_web_controller.mm

@@ -772,8 +772,7 @@ typedef void (^ViewportStateCompletion)(const web::PageViewportState*);
 - (void)updateSSLStatusForCurrentNavigationItem;
 // Called when a load ends in an SSL error and certificate chain.
 - (void)handleSSLCertError:(NSError*)error
-             forNavigation:navigation
-         errorRetryCommand:(web::ErrorRetryCommand)command;
+             forNavigation:(WKNavigation*)navigation;
 
 // Used in webView:didReceiveAuthenticationChallenge:completionHandler: to
 // reply with NSURLSessionAuthChallengeDisposition and credentials.
@@ -890,9 +889,7 @@ typedef void (^ViewportStateCompletion)(const web::PageViewportState*);
 // If this returns NO, the load should be cancelled.
 - (BOOL)shouldAllowLoadWithNavigationAction:(WKNavigationAction*)action;
 // Called when a load ends in an error.
-- (void)handleLoadError:(NSError*)error
-          forNavigation:(WKNavigation*)navigation
-      errorRetryCommand:(web::ErrorRetryCommand)command;
+- (void)handleLoadError:(NSError*)error forNavigation:(WKNavigation*)navigation;
 
 // Handles cancelled load in WKWebView (error with NSURLErrorCancelled code).
 - (void)handleCancelledError:(NSError*)error
@@ -2954,8 +2951,7 @@ registerLoadRequestForURL:(const GURL&)requestURL
 }
 
 - (void)handleLoadError:(NSError*)error
-          forNavigation:(WKNavigation*)navigation
-      errorRetryCommand:(web::ErrorRetryCommand)errorRetryCommand {
+          forNavigation:(WKNavigation*)navigation {
   if (error.code == NSURLErrorUnsupportedURL)
     return;
 
@@ -3045,9 +3041,26 @@ registerLoadRequestForURL:(const GURL&)requestURL
     [self loadErrorPageForNavigationItem:self.currentNavItem
                        navigationContext:navigationContext];
   } else {
-    [self handleErrorRetryCommand:errorRetryCommand
-                   navigationItem:self.currentNavItem
-                navigationContext:navigationContext];
+    web::NavigationItemImpl* item = web::GetItemWithUniqueID(
+        self.navigationManagerImpl,
+        navigationContext->GetNavigationItemUniqueID());
+    if (item) {
+      GURL errorURL =
+          net::GURLWithNSURL(error.userInfo[NSURLErrorFailingURLErrorKey]);
+      WKWebViewErrorSource source = WKWebViewErrorSourceFromError(error);
+      web::ErrorRetryCommand command = web::ErrorRetryCommand::kDoNothing;
+      if (source == PROVISIONAL_LOAD) {
+        command =
+            item->error_retry_state_machine().DidFailProvisionalNavigation(
+                net::GURLWithNSURL(_webView.URL), errorURL);
+      } else if (source == NAVIGATION) {
+        command = item->error_retry_state_machine().DidFailNavigation(
+            net::GURLWithNSURL(_webView.URL), errorURL);
+      }
+      [self handleErrorRetryCommand:command
+                     navigationItem:item
+                  navigationContext:navigationContext];
+    }
   }
 
   if (base::FeatureList::IsEnabled(web::features::kWebErrorPages) &&
@@ -3674,8 +3687,7 @@ registerLoadRequestForURL:(const GURL&)requestURL
 }
 
 - (void)handleSSLCertError:(NSError*)error
-             forNavigation:(WKNavigation*)navigation
-         errorRetryCommand:(web::ErrorRetryCommand)command {
+             forNavigation:(WKNavigation*)navigation {
   CHECK(web::IsWKWebViewSSLCertError(error));
 
   net::SSLInfo info;
@@ -3685,9 +3697,7 @@ registerLoadRequestForURL:(const GURL&)requestURL
     // |info.cert| can be null if certChain in NSError is empty or can not be
     // parsed, in this case do not ask delegate if error should be allowed, it
     // should not be.
-    [self handleLoadError:error
-            forNavigation:navigation
-        errorRetryCommand:command];
+    [self handleLoadError:error forNavigation:navigation];
     return;
   }
 
@@ -3718,6 +3728,21 @@ registerLoadRequestForURL:(const GURL&)requestURL
     }
   }
 
+  // If the current navigation item is in error state, update the error retry
+  // state machine to indicate that SSL interstitial error will be displayed to
+  // make sure subsequent back/forward navigation to this item starts with the
+  // correct error retry state.
+  web::NavigationContextImpl* context =
+      [_navigationStates contextForNavigation:navigation];
+  if (context) {
+    web::NavigationItemImpl* item = web::GetItemWithUniqueID(
+        self.navigationManagerImpl, context->GetNavigationItemUniqueID());
+    if (item && item->error_retry_state_machine().state() ==
+                    web::ErrorRetryState::kRetryFailedNavigationItem) {
+      item->error_retry_state_machine().SetDisplayingNativeError();
+    }
+  }
+
   // Ask web client if this cert error should be allowed.
   web::GetWebClient()->AllowCertificateError(
       _webStateImpl, net::MapCertStatusToNetError(info.cert_status), info,
@@ -3971,9 +3996,7 @@ registerLoadRequestForURL:(const GURL&)requestURL
           messageRouter:messageRouter
       completionHandler:^(NSError* loadError) {
         if (loadError)
-          [self handleLoadError:loadError
-                  forNavigation:nil
-              errorRetryCommand:web::ErrorRetryCommand::kDoNothing];
+          [self handleLoadError:loadError forNavigation:nil];
         else
           self.webStateImpl->SetContentsMimeType("text/html");
       }];
@@ -4581,29 +4604,10 @@ registerLoadRequestForURL:(const GURL&)requestURL
     [self handleCancelledError:error forNavigation:navigation];
   } else {
     error = WKWebViewErrorWithSource(error, PROVISIONAL_LOAD);
-
-    web::NavigationContextImpl* context =
-        [_navigationStates contextForNavigation:navigation];
-    web::NavigationItemImpl* item =
-        context ? web::GetItemWithUniqueID(self.navigationManagerImpl,
-                                           context->GetNavigationItemUniqueID())
-                : nullptr;
-    web::ErrorRetryCommand command = web::ErrorRetryCommand::kDoNothing;
-    if (item) {
-      GURL errorURL =
-          net::GURLWithNSURL(error.userInfo[NSURLErrorFailingURLErrorKey]);
-      command = item->error_retry_state_machine().DidFailProvisionalNavigation(
-          net::GURLWithNSURL(webView.URL), errorURL);
-    }
-
     if (web::IsWKWebViewSSLCertError(error)) {
-      [self handleSSLCertError:error
-                 forNavigation:navigation
-             errorRetryCommand:command];
+      [self handleSSLCertError:error forNavigation:navigation];
     } else {
-      [self handleLoadError:error
-              forNavigation:navigation
-          errorRetryCommand:command];
+      [self handleLoadError:error forNavigation:navigation];
     }
   }
 
@@ -4913,42 +4917,11 @@ registerLoadRequestForURL:(const GURL&)requestURL
             withError:(NSError*)error {
   [self didReceiveWebViewNavigationDelegateCallback];
 
-  GURL webViewURL = net::GURLWithNSURL(webView.URL);
-
-  auto errorRetryCommand = web::ErrorRetryCommand::kDoNothing;
-  if (web::GetWebClient()->IsSlimNavigationManagerEnabled() ||
-      base::FeatureList::IsEnabled(web::features::kWebErrorPages)) {
-    // Navigation to placeholder URL should never fail.
-    GURL errorURL =
-        net::GURLWithNSURL(error.userInfo[NSURLErrorFailingURLErrorKey]);
-    DCHECK(!IsPlaceholderUrl(errorURL));
-    // Sometimes |didFailNavigation| callback arrives after |stopLoading| has
-    // been called. Abort in this case.
-    if ([_navigationStates stateForNavigation:navigation] ==
-        web::WKNavigationState::NONE) {
-      return;
-    }
-
-    web::NavigationContextImpl* context =
-        [_navigationStates contextForNavigation:navigation];
-    web::NavigationItemImpl* item =
-        context ? web::GetItemWithUniqueID(self.navigationManagerImpl,
-                                           context->GetNavigationItemUniqueID())
-                : nullptr;
-    if (item) {
-      GURL errorURL =
-          net::GURLWithNSURL(error.userInfo[NSURLErrorFailingURLErrorKey]);
-      errorRetryCommand = item->error_retry_state_machine().DidFailNavigation(
-          webViewURL, errorURL);
-    }
-  }
-
   [_navigationStates setState:web::WKNavigationState::FAILED
                 forNavigation:navigation];
 
   [self handleLoadError:WKWebViewErrorWithSource(error, NAVIGATION)
-          forNavigation:navigation
-      errorRetryCommand:errorRetryCommand];
+          forNavigation:navigation];
   _certVerificationErrors->Clear();
   [self forgetNullWKNavigation:navigation];
 }