Handle correctly start and commit of non-latest navigations.

ios/web/web_state/ui/crw_web_controller.mm

Index: ios/web/web_state/ui/crw_web_controller.mm
diff --git a/ios/web/web_state/ui/crw_web_controller.mm b/ios/web/web_state/ui/crw_web_controller.mm
index a11db2cd1f93d34b9d54ca75e09a11516d9a6a6b..e38869c2e5bf3633140e7b6b36bf2cd2517a74fb 100644
--- a/ios/web/web_state/ui/crw_web_controller.mm
+++ b/ios/web/web_state/ui/crw_web_controller.mm
@@ -616,10 +616,11 @@ NSError* WKWebViewErrorWithSource(NSError* error, WKWebViewErrorSource source) {
 // events. Navigation is considered complete when the document has finished
 // loading, or when other page load mechanics are completed on a
 // non-document-changing URL change.
-- (void)didFinishNavigation;
+- (void)didFinishNavigation:(WKNavigation*)navigation;
 // Update the appropriate parts of the model and broadcast to the embedder. This
 // may be called multiple times and thus must be idempotent.
-- (void)loadCompleteWithSuccess:(BOOL)loadSuccess;
+- (void)loadCompleteWithSuccess:(BOOL)loadSuccess
+                  forNavigation:(WKNavigation*)navigation;
 // Called after URL is finished loading and _loadPhase is set to PAGE_LOADED.
 - (void)didFinishWithURL:(const GURL&)currentURL loadSuccess:(BOOL)loadSuccess;
 // Navigates forwards or backwards by |delta| pages. No-op if delta is out of
@@ -756,7 +757,7 @@ typedef void (^ViewportStateCompletion)(const web::PageViewportState*);
 // provided by web view.
 - (void)updateSSLStatusForCurrentNavigationItem;
 // Called when a load ends in an SSL error and certificate chain.
-- (void)handleSSLCertError:(NSError*)error;
+- (void)handleSSLCertError:(NSError*)error forNavigation:navigation;
 
 // Used in webView:didReceiveAuthenticationChallenge:completionHandler: to
 // reply with NSURLSessionAuthChallengeDisposition and credentials.
@@ -889,7 +890,9 @@ typedef void (^ViewportStateCompletion)(const web::PageViewportState*);
 // TODO(stuartmorgan): Figure out if there's actually enough shared logic that
 // this makes sense. At the very least remove inMainFrame since that only makes
 // sense for UIWebView.
-- (void)handleLoadError:(NSError*)error inMainFrame:(BOOL)inMainFrame;
+- (void)handleLoadError:(NSError*)error
+            inMainFrame:(BOOL)inMainFrame
+          forNavigation:(WKNavigation*)navigation;
 
 // Handles cancelled load in WKWebView (error with NSURLErrorCancelled code).
 - (void)handleCancelledError:(NSError*)error;
@@ -2070,7 +2073,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
       [sessionController isSameDocumentNavigationBetweenItem:fromItem
                                                      andItem:toItem];
   if (sameDocumentNavigation) {
-    [sessionController goToItemAtIndex:index];
+    [sessionController goToItemAtIndex:index discardNonCommittedItems:YES];
     [self updateHTML5HistoryState];
   } else {
     [sessionController discardNonCommittedItems];
@@ -2088,15 +2091,16 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   return _loadPhase == web::PAGE_LOADED;
 }
 
-- (void)didFinishNavigation {
+- (void)didFinishNavigation:(WKNavigation*)navigation {
   // This can be called at multiple times after the document has loaded. Do
   // nothing if the document has already loaded.
   if (_loadPhase == web::PAGE_LOADED)
     return;
-  [self loadCompleteWithSuccess:YES];
+  [self loadCompleteWithSuccess:YES forNavigation:navigation];
 }
 
-- (void)loadCompleteWithSuccess:(BOOL)loadSuccess {
+- (void)loadCompleteWithSuccess:(BOOL)loadSuccess
+                  forNavigation:(WKNavigation*)navigation {
   [self removePlaceholderOverlay];
   // The webView may have been torn down (or replaced by a native view). Be
   // safe and do nothing if that's happened.
@@ -2111,12 +2115,11 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
 
   [self optOutScrollsToTopForSubviews];
 
-  // Ensure the URL is as expected (and already reported to the delegate).
-  // If |_lastRegisteredRequestURL| is invalid then |currentURL| will be
-  // "about:blank".
-  DCHECK((currentURL == _lastRegisteredRequestURL) ||
+  DCHECK((currentURL == _lastRegisteredRequestURL) ||  // latest navigation
+         // previous navigation
+         ![[_navigationStates lastAddedNavigation] isEqual:navigation] ||
          (!_lastRegisteredRequestURL.is_valid() &&
-          _documentURL.spec() == url::kAboutBlankURL))
+          _documentURL.spec() == url::kAboutBlankURL) /* invalid URL load */)

[kkhorimoto, 2017/04/18 23:03:15]

Some of these are line comments, but some are block comments.  Do you want to
choose one and stick with it?

[Eugene But (OOO till 7-30), 2017/04/19 00:26:46]

Done.

       << std::endl
       << "currentURL = [" << currentURL << "]" << std::endl
       << "_lastRegisteredRequestURL = [" << _lastRegisteredRequestURL << "]";
@@ -2823,7 +2826,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
       return;
     base::scoped_nsobject<CRWWebController> strongSelf([weakSelf retain]);
     [strongSelf optOutScrollsToTopForSubviews];
-    [strongSelf didFinishNavigation];
+    [strongSelf didFinishNavigation:nil];
   }];
   return YES;
 }
@@ -2878,7 +2881,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
     if (!weakSelf || weakSelf.get()->_isBeingDestroyed)
       return;
     base::scoped_nsobject<CRWWebController> strongSelf([weakSelf retain]);
-    [strongSelf didFinishNavigation];
+    [strongSelf didFinishNavigation:nil];
   }];
   return YES;
 }
@@ -3113,7 +3116,9 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   return YES;
 }
 
-- (void)handleLoadError:(NSError*)error inMainFrame:(BOOL)inMainFrame {
+- (void)handleLoadError:(NSError*)error
+            inMainFrame:(BOOL)inMainFrame
+          forNavigation:(WKNavigation*)navigation {
   NSString* MIMEType = [_pendingNavigationInfo MIMEType];
   if ([_passKitDownloader isMIMETypePassKitType:MIMEType])
     return;
@@ -3176,7 +3181,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
       id<CRWNativeContent> controller =
           [_delegate controllerForUnhandledContentAtURL:errorGURL];
       if (controller) {
-        [self loadCompleteWithSuccess:NO];
+        [self loadCompleteWithSuccess:NO forNavigation:navigation];
         [self removeWebViewAllowingCachedReconstruction:NO];
         [self setNativeController:controller];
         [self loadNativeViewWithSuccess:YES];
@@ -3204,7 +3209,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
                  NSURLErrorFailingURLStringErrorKey : [errorURL absoluteString],
                  NSUnderlyingErrorKey : error
                }];
-    [self loadCompleteWithSuccess:NO];
+    [self loadCompleteWithSuccess:NO forNavigation:navigation];
     [self loadErrorInNativeView:wrapperError];
     return;
   }
@@ -3216,7 +3221,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
     return;
   }
 
-  [self loadCompleteWithSuccess:NO];
+  [self loadCompleteWithSuccess:NO forNavigation:navigation];
   [self loadErrorInNativeView:error];
 }
 
@@ -3949,7 +3954,8 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   _webStateImpl->OnVisibleSecurityStateChange();
 }
 
-- (void)handleSSLCertError:(NSError*)error {
+- (void)handleSSLCertError:(NSError*)error
+             forNavigation:(WKNavigation*)navigation {
   CHECK(web::IsWKWebViewSSLCertError(error));
 
   net::SSLInfo info;
@@ -3959,7 +3965,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
     // |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 inMainFrame:YES];
+    [self handleLoadError:error inMainFrame:YES forNavigation:navigation];
     return;
   }
 
@@ -4159,7 +4165,9 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
                         messageRouter:messageRouter
                     completionHandler:^(NSError* loadError) {
                       if (loadError)
-                        [self handleLoadError:loadError inMainFrame:YES];
+                        [self handleLoadError:loadError
+                                  inMainFrame:YES
+                                forNavigation:nil];
                       else
                         self.webStateImpl->SetContentsMimeType("text/html");
                     }];
@@ -4423,6 +4431,13 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   [_navigationStates setState:web::WKNavigationState::STARTED
                 forNavigation:navigation];
 
+  if (navigation &&
+      ![[_navigationStates lastAddedNavigation] isEqual:navigation]) {
+    // |navigation| is not the latest navigation and will be cancelled.
+    // Ignore |navigation| as a new navigation will start soon.
+    return;
+  }
+
   GURL webViewURL = net::GURLWithNSURL(webView.URL);
   if (webViewURL.is_empty()) {
     // May happen on iOS9, however in didCommitNavigation: callback the URL
@@ -4510,9 +4525,9 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
     error = WKWebViewErrorWithSource(error, PROVISIONAL_LOAD);
 
     if (web::IsWKWebViewSSLCertError(error))
-      [self handleSSLCertError:error];
+      [self handleSSLCertError:error forNavigation:navigation];
     else
-      [self handleLoadError:error inMainFrame:YES];
+      [self handleLoadError:error inMainFrame:YES forNavigation:navigation];
   }
 
   // This must be reset at the end, since code above may need information about
@@ -4539,9 +4554,16 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
     // will be "about:blank".
     [[self sessionController] updatePendingItem:_documentURL];
   }
-  DCHECK(_documentURL == _lastRegisteredRequestURL ||
+
+  // If |navigation| is nil (which happens for windows open by DOM), then it
+  // should be the first and the only pending navigaiton.
+  BOOL isLastNavigation =
+      !navigation ||
+      [[_navigationStates lastAddedNavigation] isEqual:navigation];
+  DCHECK(_documentURL == _lastRegisteredRequestURL ||  // latest navigation
+         !isLastNavigation ||                          // previous navigation
          (!_lastRegisteredRequestURL.is_valid() &&
-          _documentURL.spec() == url::kAboutBlankURL));
+          _documentURL.spec() == url::kAboutBlankURL) /* invalid URL load */);

[kkhorimoto, 2017/04/18 23:03:15]

ditto

[Eugene But (OOO till 7-30), 2017/04/19 00:26:46]

Done.

 
   self.webStateImpl->UpdateHttpResponseHeaders(_documentURL);
   [self commitPendingNavigationInfo];
@@ -4567,7 +4589,21 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
     [self injectWindowID];
   }
 
-  [self webPageChanged];
+  if (isLastNavigation) {
+    [self webPageChanged];
+  } else {
+    // WKWebView has more than one in progress navigaiton, and committed
+    // navigation was not the latests. It is critical to keep last committed
+    // URL the same as actual document URL, so try guessing which navigation
+    // item should be set to current.
+    // TODO(crbug.com/712269):
+    for (int i = 0; i < self.navigationManagerImpl->GetItemCount(); i++) {
+      web::NavigationItem* item = self.navigationManagerImpl->GetItemAtIndex(i);
+      if (item->GetURL() == _documentURL) {
+        [self.sessionController goToItemAtIndex:i discardNonCommittedItems:NO];

[kkhorimoto, 2017/04/18 23:03:15]

Could you explain why we aren't discarding the non-committed items here?

[Eugene But (OOO till 7-30), 2017/04/19 00:26:46]

Done.

+      }
+    }
+  }
   self.webStateImpl->OnNavigationCommitted(_documentURL);
 
   [self updateSSLStatusForCurrentNavigationItem];
@@ -4599,7 +4635,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   // WKUserScriptInjectionTimeAtDocumentEnd to inject this material at the
   // appropriate time rather than invoking here.
   web::ExecuteJavaScript(webView, @"__gCrWeb.didFinishNavigation()", nil);
-  [self didFinishNavigation];
+  [self didFinishNavigation:navigation];
 }
 
 - (void)webView:(WKWebView*)webView
@@ -4609,7 +4645,8 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
                 forNavigation:navigation];
 
   [self handleLoadError:WKWebViewErrorWithSource(error, NAVIGATION)
-            inMainFrame:YES];
+            inMainFrame:YES
+          forNavigation:navigation];
   _certVerificationErrors->Clear();
 }
 
@@ -4769,7 +4806,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   // Fast back forward navigation may not call |didFinishNavigation:|, so
   // signal did finish navigation explicitly.
   if (_lastRegisteredRequestURL == _documentURL) {
-    [self didFinishNavigation];
+    [self didFinishNavigation:nil];
   }
 }
 
@@ -4916,7 +4953,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
     [self didStartLoadingURL:_documentURL];
     _webStateImpl->OnSameDocumentNavigation(newURL);
     [self updateSSLStatusForCurrentNavigationItem];
-    [self didFinishNavigation];
+    [self didFinishNavigation:nil];
   }
 }