[Nav Experiment] Fix error retry state corruption for WebErrorPages.

When an error page is displayed in web view, the error retry state of
the navigation item is never transitioned to
kDisplayingWebErrorForFailedNavigation. This does not prevent the
displaying of error page on the first failure, but subsequent back-
forward navigation to this item starts with the incorrect state and
can result in DCHECKs of unexpected error state, error view not
being loaded and poor interaction with other types of errors, such as
SSL interstitial.

This CL refactors the state transitions for WebErrorPages so it is
handled identically to native error. This also required web error
load to not skip DidFinishNavigation callback to correctly update
the ErrorRetryStateMachine.

Also parameterized error_page_inttest.mm so both navigation manager
implementations are tested on trybots.

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

ios/web/navigation/error_retry_state_machine.mm

@@ -104,7 +104,7 @@ ErrorRetryCommand ErrorRetryStateMachine::DidFinishNavigation(
         return ErrorRetryCommand::kLoadErrorView;
       }
       if (wk_navigation_util::IsRestoreSessionUrl(web_view_url)) {
-        // (10) initial load of restore_session.html. Don't change state or
+        // (8) Initial load of restore_session.html. Don't change state or
         // issue command. Wait for the client-side redirect.
       } else {
         // (2) Initial load succeeded.
@@ -112,24 +112,16 @@ ErrorRetryCommand ErrorRetryStateMachine::DidFinishNavigation(
                web_view_url == url_);
         state_ = ErrorRetryState::kNoNavigationError;
       }
-
       break;
 
     case ErrorRetryState::kReadyToDisplayErrorForFailedNavigation:
-      if (web_view_url ==
-          wk_navigation_util::CreatePlaceholderUrlForUrl(url_)) {
-        // (4) Back/forward to or reload of placeholder URL. Rewrite WebView URL
-        // to prepare for retry.
-        state_ = ErrorRetryState::kNavigatingToFailedNavigationItem;
-        return ErrorRetryCommand::kRewriteWebViewURL;
-      }
-
       // (3) Finished loading error in web view.
       DCHECK_EQ(web_view_url, url_);
       state_ = ErrorRetryState::kDisplayingWebErrorForFailedNavigation;
       break;
 
     case ErrorRetryState::kDisplayingNativeErrorForFailedNavigation:
+    case ErrorRetryState::kDisplayingWebErrorForFailedNavigation:
       if (web_view_url ==
           wk_navigation_util::CreatePlaceholderUrlForUrl(url_)) {
         // (4) Back/forward to or reload of placeholder URL. Rewrite WebView URL
@@ -138,8 +130,13 @@ ErrorRetryCommand ErrorRetryStateMachine::DidFinishNavigation(
         return ErrorRetryCommand::kRewriteWebViewURL;
       }
 
-      // (5) Reload of original URL succeeded in WebView (either because it was
-      // already in Page Cache or the network load succeded.
+      // (5) This is either a reload of the original URL that succeeded in
+      // WebView (either because it was already in Page Cache or the network
+      // load succeded), or a back/forward of a previous WebUI error that is
+      // served from Page Cache. It's impossible to distinguish between the two
+      // because in both cases, |web_view_url| is the original URL. This can
+      // lead to network error being displayed even when network condition
+      // is regained. User has to reload explicitly to retry loading online.
       DCHECK_EQ(web_view_url, url_);
       state_ = ErrorRetryState::kNoNavigationError;
       break;
@@ -153,8 +150,6 @@ ErrorRetryCommand ErrorRetryStateMachine::DidFinishNavigation(
 
     // (7) Retry loading succeeded.
     case ErrorRetryState::kRetryFailedNavigationItem:
-    // (8) Back/forward to or reload of a previous WebUI error succeeds.
-    case ErrorRetryState::kDisplayingWebErrorForFailedNavigation:
       DCHECK_EQ(web_view_url, url_);
       state_ = ErrorRetryState::kNoNavigationError;
       break;

ios/web/navigation/error_retry_state_machine_unittest.mm

@@ -118,6 +118,15 @@ TEST_F(ErrorRetryStateMachineTest, WebErrorPageThenReload) {
     ASSERT_EQ(ErrorRetryState::kNoNavigationError, clone.state());
   }
 
+  // Back-forward navigation to failed navigation rewrites Web View URL.
+  {
+    ErrorRetryStateMachine clone(machine);
+    ASSERT_EQ(ErrorRetryCommand::kRewriteWebViewURL,
+              clone.DidFinishNavigation(placeholder_url));
+    ASSERT_EQ(ErrorRetryState::kNavigatingToFailedNavigationItem,
+              clone.state());
+  }
+
   // Reload fails again in provisional navigation.
   {
     ErrorRetryStateMachine clone(machine);

ios/web/web_state/error_page_inttest.mm

@@ -43,10 +43,19 @@ class TestWebClient : public WebClient {
 };
 }  // namespace
 
+// ErrorPageTest is parameterized on this enum to test both
+// LegacyNavigationManagerImpl and WKBasedNavigationManagerImpl.
+enum class NavigationManagerChoice {
+  LEGACY,
+  WK_BASED,
+};
+
 // Test fixture for error page testing. Error page simply renders the arguments
 // passed to WebClient::PrepareErrorPage, so the test also acts as integration
 // test for PrepareErrorPage WebClient method.
-class ErrorPageTest : public WebTestWithWebState {
+class ErrorPageTest
+    : public WebTestWithWebState,
+      public ::testing::WithParamInterface<NavigationManagerChoice> {
  protected:
   ErrorPageTest() : WebTestWithWebState(std::make_unique<TestWebClient>()) {
     RegisterDefaultHandlers(&server_);
@@ -60,7 +69,15 @@ class ErrorPageTest : public WebTestWithWebState {
     server_.RegisterRequestHandler(
         base::BindRepeating(&net::test_server::HandlePrefixedRequest, "/form",
                             base::BindRepeating(&testing::HandleForm)));
-    scoped_feature_list_.InitAndEnableFeature(features::kWebErrorPages);
+
+    std::vector<base::Feature> enabled_features = {features::kWebErrorPages};
+    std::vector<base::Feature> disabled_features;
+    if (GetParam() == NavigationManagerChoice::LEGACY) {
+      disabled_features.push_back(features::kSlimNavigationManager);
+    } else {
+      enabled_features.push_back(features::kSlimNavigationManager);
+    }
+    scoped_feature_list_.InitWithFeatures(enabled_features, disabled_features);
   }
 
   void SetUp() override {
@@ -77,7 +94,7 @@ class ErrorPageTest : public WebTestWithWebState {
 };
 
 // Loads the URL which fails to load, then sucessfully reloads the page.
-TEST_F(ErrorPageTest, ReloadErrorPage) {
+TEST_P(ErrorPageTest, ReloadErrorPage) {
   // No response leads to -1005 error code.
   server_responds_with_content_ = false;
   test::LoadUrl(web_state(), server_.GetURL("/echo-query?foo"));
@@ -92,7 +109,7 @@ TEST_F(ErrorPageTest, ReloadErrorPage) {
 }
 
 // Sucessfully loads the page, stops the server and reloads the page.
-TEST_F(ErrorPageTest, ReloadPageAfterServerIsDown) {
+TEST_P(ErrorPageTest, ReloadPageAfterServerIsDown) {
   // Sucessfully load the page.
   server_responds_with_content_ = true;
   test::LoadUrl(web_state(), server_.GetURL("/echo-query?foo"));
@@ -108,7 +125,7 @@ TEST_F(ErrorPageTest, ReloadPageAfterServerIsDown) {
 
 // Sucessfully loads the page, goes back, stops the server, goes forward and
 // reloads.
-TEST_F(ErrorPageTest, GoForwardAfterServerIsDownAndReload) {
+TEST_P(ErrorPageTest, GoForwardAfterServerIsDownAndReload) {
   // First page loads sucessfully.
   test::LoadUrl(web_state(), server_.GetURL("/echo"));
   ASSERT_TRUE(test::WaitForWebViewContainingText(web_state(), "Echo"));
@@ -141,7 +158,7 @@ TEST_F(ErrorPageTest, GoForwardAfterServerIsDownAndReload) {
 // Sucessfully loads the page, then loads the URL which fails to load, then
 // sucessfully goes back to the first page and goes forward to error page.
 // Back-forward navigations are browser-initiated.
-TEST_F(ErrorPageTest, GoBackFromErrorPageAndForwardToErrorPage) {
+TEST_P(ErrorPageTest, GoBackFromErrorPageAndForwardToErrorPage) {
   // First page loads sucessfully.
   test::LoadUrl(web_state(), server_.GetURL("/echo"));
   ASSERT_TRUE(test::WaitForWebViewContainingText(web_state(), "Echo"));
@@ -164,8 +181,13 @@ TEST_F(ErrorPageTest, GoBackFromErrorPageAndForwardToErrorPage) {
 // Sucessfully loads the page, then loads the URL which fails to load, then
 // sucessfully goes back to the first page and goes forward to error page.
 // Back-forward navigations are renderer-initiated.
-TEST_F(ErrorPageTest,
+TEST_P(ErrorPageTest,
        RendererInitiatedGoBackFromErrorPageAndForwardToErrorPage) {
+  if (GetParam() == NavigationManagerChoice::WK_BASED) {
+    // TODO(crbug.com/867927): Re-enable this test.
+    return;
+  }
+
   // First page loads sucessfully.
   test::LoadUrl(web_state(), server_.GetURL("/echo"));
   ASSERT_TRUE(test::WaitForWebViewContainingText(web_state(), "Echo"));
@@ -186,7 +208,7 @@ TEST_F(ErrorPageTest,
 }
 
 // Loads the URL which redirects to unresponsive server.
-TEST_F(ErrorPageTest, RedirectToFailingURL) {
+TEST_P(ErrorPageTest, RedirectToFailingURL) {
   // No response leads to -1005 error code.
   server_responds_with_content_ = false;
   test::LoadUrl(web_state(), server_.GetURL("/server-redirect?echo-query"));
@@ -196,7 +218,7 @@ TEST_F(ErrorPageTest, RedirectToFailingURL) {
 
 // Loads the page with iframe, and that iframe fails to load. There should be no
 // error page if the main frame has sucessfully loaded.
-TEST_F(ErrorPageTest, ErrorPageInIFrame) {
+TEST_P(ErrorPageTest, ErrorPageInIFrame) {
   test::LoadUrl(web_state(), server_.GetURL("/iframe?echo-query"));
   EXPECT_TRUE(WaitUntilConditionOrTimeout(kWaitForPageLoadTimeout, ^{
     return test::IsWebViewContainingElement(
@@ -206,7 +228,7 @@ TEST_F(ErrorPageTest, ErrorPageInIFrame) {
 }
 
 // Loads the URL with off the record browser state.
-TEST_F(ErrorPageTest, OtrError) {
+TEST_P(ErrorPageTest, OtrError) {
   TestBrowserState browser_state;
   browser_state.SetOffTheRecord(true);
   WebState::CreateParams params(&browser_state);
@@ -223,7 +245,7 @@ TEST_F(ErrorPageTest, OtrError) {
 }
 
 // Loads the URL with form which fails to submit.
-TEST_F(ErrorPageTest, FormSubmissionError) {
+TEST_P(ErrorPageTest, FormSubmissionError) {
   test::LoadUrl(web_state(), server_.GetURL("/form?close-socket"));
   ASSERT_TRUE(
       test::WaitForWebViewContainingText(web_state(), testing::kTestFormPage));
@@ -235,4 +257,8 @@ TEST_F(ErrorPageTest, FormSubmissionError) {
       web_state(), "domain: NSURLErrorDomain code: -1005 post: 1 otr: 0"));
 }
 
+INSTANTIATE_TEST_CASE_P(ProgrammaticErrorPageTest,
+                        ErrorPageTest,
+                        ::testing::Values(NavigationManagerChoice::LEGACY,
+                                          NavigationManagerChoice::WK_BASED));
 }  // namespace web

ios/web/web_state/ui/crw_web_controller.mm

@@ -1770,6 +1770,7 @@ registerLoadRequestForURL:(const GURL&)requestURL
         /*has_user_gesture=*/false, ui::PAGE_TRANSITION_FIRST,
         /*is_renderer_initiated=*/false);
     loadHTMLContext->SetLoadingErrorPage(true);
+    loadHTMLContext->SetNavigationItemUniqueID(item->GetUniqueID());
 
     [_navigationStates setContext:std::move(loadHTMLContext)
                     forNavigation:navigation];
@@ -4821,9 +4822,6 @@ registerLoadRequestForURL:(const GURL&)requestURL
 
   web::NavigationContextImpl* context =
       [_navigationStates contextForNavigation:navigation];
-  if (context && context->IsLoadingErrorPage()) {
-    return;
-  }
 
   if (context && context->GetUrl() == currentWKItemURL) {
     // If webView.backForwardList.currentItem.URL matches |context|, then this