[iOS] Don't remove navigation for committed SSL interstitial in didFailProvisionalNavigation

The WKNavigation for a committed interstitial is used in
the callback passed to ChromeWebClient::PrepareErrorPage, in
order to commit the associated navigation item. For SSL
interstitials, this callback is called later, asynchronously.
This means that deleting the navigation in
didFailProvisionalNavigation later leads to a crash when
attempting to access the navigation's item, in the case
where the error page load is triggered in
didFailProvisionalNavigation. This happens in
ErrorRetryStateMachine::DidFailProvisionalNavigation when
the current webview URL is a restore_session.html URL.

This CL fixes this crash by not deleting the navigation in
didFailProvisionalNavigation when it is for a committed SSL
interstital. There is already similar logic for deciding
whether to delete a navigation in didFinishNavigation.

Change-Id: Ib8aabeeeca33f63b313daa0772990b8b841d462a
Bug: 1050808
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2248137
Commit-Queue: Ali Juma <ajuma@chromium.org>
Reviewed-by: Livvie Lin <livvielin@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779833}

ios/chrome/browser/web/ssl_egtest.mm

@@ -27,6 +27,13 @@
 
 @implementation SSLTestCase
 
+- (AppLaunchConfiguration)appConfigurationForTestCase {
+  AppLaunchConfiguration config;
+  config.features_enabled.push_back(web::features::kSSLCommittedInterstitials);
+  config.relaunch_policy = NoForceRelaunchAndResetState;
+  return config;
+}
+
 - (void)setUp {
   [super setUp];
   _HTTPSServer = std::make_unique<net::test_server::EmbeddedTestServer>(
@@ -37,12 +44,6 @@
 // Test loading a page with a bad SSL certificate from the NTP, to avoid
 // https://crbug.com/1067250 from regressing.
 - (void)testBadSSLOnNTP {
-  if (!base::FeatureList::IsEnabled(
-          web::features::kSSLCommittedInterstitials)) {
-    // The content of the interstitial isn't in the webstate in that case.
-    EARL_GREY_TEST_SKIPPED(@"The test needs committed interstitials enabled.");
-  }
-
   GREYAssertTrue(_HTTPSServer->Start(), @"Test server failed to start.");
 
   const GURL pageURL = _HTTPSServer->GetURL("/echo");
@@ -52,4 +53,20 @@
                                                     IDS_SSL_V2_HEADING)];
 }
 
+// Test loading a page with a bad SSL certificate during session restore, to
+// avoid regressing https://crbug.com/1050808.
+- (void)testBadSSLInSessionRestore {
+  GREYAssertTrue(_HTTPSServer->Start(), @"Test server failed to start.");
+
+  GURL pageURL = _HTTPSServer->GetURL("/echo");
+  [ChromeEarlGrey loadURL:pageURL];
+
+  [ChromeEarlGrey waitForWebStateContainingText:l10n_util::GetStringUTF8(
+                                                    IDS_SSL_V2_HEADING)];
+
+  [ChromeEarlGrey triggerRestoreViaTabGridRemoveAllUndo];
+  [ChromeEarlGrey waitForWebStateContainingText:l10n_util::GetStringUTF8(
+                                                    IDS_SSL_V2_HEADING)];
+}
+
 @end

ios/web/navigation/crw_wk_navigation_handler.mm

@@ -783,9 +783,19 @@ void ReportOutOfSyncURLInDidStartProvisionalNavigation(
   if (!web::IsWKWebViewSSLCertError(error)) {
     _certVerificationErrors->Clear();
   }
-  // Remove the navigation to immediately get rid of pending item.
+
+  web::NavigationContextImpl* context =
+      [self.navigationStates contextForNavigation:navigation];
+
+  // Remove the navigation to immediately get rid of pending item. Navigation
+  // should not be cleared, however, in the case of a committed interstitial
+  // for an SSL error.
   if (web::WKNavigationState::NONE !=
-      [self.navigationStates stateForNavigation:navigation]) {
+          [self.navigationStates stateForNavigation:navigation] &&
+      !(context && web::IsWKWebViewSSLCertError(context->GetError()) &&
+        base::FeatureList::IsEnabled(
+            web::features::kSSLCommittedInterstitials) &&
+        !base::FeatureList::IsEnabled(web::features::kUseJSForErrorPage))) {
     [self.navigationStates removeNavigation:navigation];
   }
 }