Fix URL change type detection in WKWebView

Since the heurstic for registering a load request depends on state that
is reset on URL change, the load request for a non-document URL change
must be procesed before the URL is updated, not after. (This also
matches the normal flow, where a load request by definition happens
before the load commits.)

Also fixes issues surfaced during investigation of navigation test
failures:
- Don't register a new pending item if there already is one (happens
  for certain back/forward navigations)
- Use interaction registration, not touch, to drive the heuristic,
  since there are cases that are user interaction but not touch events
  and this is already plumbed into the superclass.

BUG=540786, 550474, 550473

Review URL: https://codereview.chromium.org/1411843010

Cr-Commit-Position: refs/heads/master@{#357843}

3 files changed, 85 insertions, 32 deletions

diff --git a/ios/web/web_state/ui/crw_web_controller+protected.h b/ios/web/web_state/ui/crw_web_controller+protected.h
index 187d12f..bfabca4 100644
--- a/ios/web/web_state/ui/crw_web_controller+protected.h
+++ b/ios/web/web_state/ui/crw_web_controller+protected.h
@@ -240,9 +240,9 @@ struct NewWindowInfo {
 // Returns whether the user is interacting with the page.
 @property(nonatomic, readonly) BOOL userIsInteracting;
 
-// YES if a user interaction has been registered at any time once the page has
+// YES if a user interaction has been registered at any time since the page has
 // loaded.
-@property(nonatomic, readonly) BOOL userInteractionRegistered;
+@property(nonatomic, readwrite) BOOL userInteractionRegistered;
 
 // Returns the current window id.
 @property(nonatomic, readonly) NSString* windowId;
@@ -299,10 +299,6 @@ struct NewWindowInfo {
 // state not specific to web pages, and informs the delegate.
 - (void)didStartLoadingURL:(const GURL&)URL updateHistory:(BOOL)updateHistory;
 
-// Should be called with YES if a user interaction has been registered at any
-// time once the page has loaded.
-- (void)setUserInteractionRegistered:(BOOL)flag;
-
 // Returns YES if the user interacted with the page recently.
 - (BOOL)userClickedRecently;
 
diff --git a/ios/web/web_state/ui/crw_web_controller.mm b/ios/web/web_state/ui/crw_web_controller.mm
index ce3510f..630a84b 100644
--- a/ios/web/web_state/ui/crw_web_controller.mm
+++ b/ios/web/web_state/ui/crw_web_controller.mm
@@ -2611,7 +2611,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   _URLOnStartLoading = url;
   _displayStateOnStartLoading = self.pageDisplayState;
 
-  _userInteractionRegistered = NO;
+  self.userInteractionRegistered = NO;
   _pageHasZoomed = NO;
 
   [[self sessionController] commitPendingEntry];
@@ -3094,7 +3094,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
 - (void)touched:(BOOL)touched {
   _clickInProgress = touched;
   if (touched) {
-    _userInteractionRegistered = YES;
+    self.userInteractionRegistered = YES;
     _userInteractedWithWebController = YES;
     if (_isBeingDestroyed)
       return;
@@ -3302,7 +3302,7 @@ const NSTimeInterval kSnapshotOverlayTransition = 0.5;
   // - the user has interacted with the page.
   CRWSessionEntry* current = [self currentSessionEntry];
   if (current && [current navigationItem]->GetURL() == [self currentURL] &&
-      _userInteractionRegistered) {
+      self.userInteractionRegistered) {
     [current navigationItem]->SetPageDisplayState(self.pageDisplayState);
   }
 }
diff --git a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
index 3165401..3163c5c 100644
--- a/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
+++ b/ios/web/web_state/ui/crw_wk_web_view_web_controller.mm
@@ -26,6 +26,7 @@
 #include "ios/web/public/cert_store.h"
 #include "ios/web/public/navigation_item.h"
 #include "ios/web/public/ssl_status.h"
+#include "ios/web/public/url_util.h"
 #include "ios/web/public/web_client.h"
 #import "ios/web/public/web_state/js/crw_js_injection_manager.h"
 #import "ios/web/public/web_state/ui/crw_native_content_provider.h"
@@ -175,8 +176,9 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
   // |didFailProvisionalNavigation:| delegate method.
   scoped_ptr<CertVerificationErrorsCacheType> _certVerificationErrors;
 
-  // YES if the user has touched the content area since the last URL change.
-  BOOL _touchedSinceLastURLChange;
+  // YES if the user has interacted with the content area since the last URL
+  // change.
+  BOOL _interactionRegisteredSinceLastURLChange;
 }
 
 // Response's MIME type of the last known navigation.
@@ -319,10 +321,20 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 // transition based on user interaction state.
 - (void)registerLoadRequest:(const GURL&)url;
 
+// Returns YES if a KVO change to |newURL| could be a 'navigation' within the
+// document (hash change, pushState/replaceState, etc.). This should only be
+// used in the context of a URL KVO callback firing, and only if |isLoading| is
+// YES for the web view (since if it's not, no guesswork is needed).
+- (BOOL)isKVOChangePotentialSameDocumentNavigationToURL:(const GURL&)newURL;
+
 // Called when a non-document-changing URL change occurs. Updates the
 // _documentURL, and informs the superclass of the change.
 - (void)URLDidChangeWithoutDocumentChange:(const GURL&)URL;
 
+// Returns YES if there is currently a requested but uncommitted load for
+// |targetURL|.
+- (BOOL)isLoadRequestPendingForURL:(const GURL&)targetURL;
+
 // Called when web controller receives a new message from the web page.
 - (void)didReceiveScriptMessage:(WKScriptMessage*)message;
 
@@ -414,12 +426,6 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
   [super close];
 }
 
-- (void)touched:(BOOL)touched {
-  [super touched:touched];
-  if (touched)
-    _touchedSinceLastURLChange = YES;
-}
-
 #pragma mark -
 #pragma mark Testing-Only Methods
 
@@ -450,6 +456,13 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
   return _currentReferrerString.get();
 }
 
+// Overridden to track interactions since URL change.
+- (void)setUserInteractionRegistered:(BOOL)flag {
+  [super setUserInteractionRegistered:flag];
+  if (flag)
+    _interactionRegisteredSinceLastURLChange = YES;
+}
+
 #pragma mark Protected method implementations
 
 - (void)ensureWebViewCreated {
@@ -782,7 +795,7 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
 - (void)setDocumentURL:(const GURL&)newURL {
   if (newURL != _documentURL) {
     _documentURL = newURL;
-    _touchedSinceLastURLChange = NO;
+    _interactionRegisteredSinceLastURLChange = NO;
   }
 }
 
@@ -1176,9 +1189,9 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
     case WKNavigationTypeOther:
       // The "Other" type covers a variety of very different cases, which may
       // or may not be the result of user actions. For now, guess based on
-      // whether there's been a touch since the last URL change.
+      // whether there's been an interaction since the last URL change.
       // TODO(crbug.com/549301): See if this heuristic can be improved.
-      transition = _touchedSinceLastURLChange
+      transition = _interactionRegisteredSinceLastURLChange
                        ? ui::PAGE_TRANSITION_LINK
                        : ui::PAGE_TRANSITION_CLIENT_REDIRECT;
       break;
@@ -1188,10 +1201,29 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
   [self registerLoadRequest:url referrer:emptyReferrer transition:transition];
 }
 
+- (BOOL)isKVOChangePotentialSameDocumentNavigationToURL:(const GURL&)newURL {
+  DCHECK([_wkWebView isLoading]);
+  // If the origin changes, it can't be same-document.
+  if (_documentURL.GetOrigin().is_empty() ||
+      _documentURL.GetOrigin() != newURL.GetOrigin()) {
+    return NO;
+  }
+  if (self.loadPhase == web::LOAD_REQUESTED) {
+    // Normally LOAD_REQUESTED indicates that this is a regular, pending
+    // navigation, but it can also happen during a fast-back navigation across
+    // a hash change, so that case is potentially a same-document navigation.
+    return web::GURLByRemovingRefFromGURL(newURL) ==
+           web::GURLByRemovingRefFromGURL(_documentURL);
+  }
+  // If it passes all the checks above, it might be (but there's no guarantee
+  // that it is).
+  return YES;
+}
+
 - (void)URLDidChangeWithoutDocumentChange:(const GURL&)newURL {
   DCHECK(newURL == net::GURLWithNSURL([_wkWebView URL]));
   DCHECK_EQ(_documentURL.host(), newURL.host());
-  [self setDocumentURL:newURL];
+
   // If called during window.history.pushState or window.history.replaceState
   // JavaScript evaluation, only update the document URL. This callback does not
   // have any information about the state object and cannot create (or edit) the
@@ -1199,13 +1231,37 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
   // history changes when a window.history.didPushState or
   // window.history.didReplaceState message is received, which should happen in
   // the next runloop.
+  //
+  // Otherwise, simulate the whole delegate flow for a load (since the
+  // superclass currently doesn't have a clean separation between URL changes
+  // and document changes). Note that the order of these calls is important:
+  // registering a load request logically comes before updating the document
+  // URL, but also must come first since it uses state that is reset on URL
+  // changes.
+  if (!_changingHistoryState) {
+    // If this wasn't a previously-expected load (e.g., certain back/forward
+    // navigations), register the load request.
+    if (![self isLoadRequestPendingForURL:newURL])
+      [self registerLoadRequest:newURL];
+  }
+
+  [self setDocumentURL:newURL];
+
   if (!_changingHistoryState) {
-    [self registerLoadRequest:_documentURL];
     [self didStartLoadingURL:_documentURL updateHistory:YES];
     [self didFinishNavigation];
   }
 }
 
+- (BOOL)isLoadRequestPendingForURL:(const GURL&)targetURL {
+  if (self.loadPhase != web::LOAD_REQUESTED)
+    return NO;
+
+  web::NavigationItem* pendingItem =
+      self.webState->GetNavigationManager()->GetPendingItem();
+  return pendingItem && pendingItem->GetURL() == targetURL;
+}
+
 - (void)didReceiveScriptMessage:(WKScriptMessage*)message {
   // Broken out into separate method to catch errors.
   if (![self respondToWKScriptMessage:message]) {
@@ -1402,22 +1458,22 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
   // navigation failure and do nothing. If the URL does not match, assume it is
   // a non-document-changing URL change, and handle accordingly.
   //
-  // If |isLoading| is YES, then it could either be case 1 (if the load phase is
-  // web::LOAD_REQUESTED), or it could be case 2 on a page that hasn't finished
-  // loading yet. If the domain of the new URL matches the last committed URL,
-  // then check window.location.href, and if it matches, trust it. The domain
+  // If |isLoading| is YES, then it could either be case 1, or it could be case
+  // 2 on a page that hasn't finished loading yet. If it's possible that it
+  // could be a same-page navigation (in which case there may not be any other
+  // callback about the URL having changed), then check the actual page URL via
+  // JavaScript. If the origin of the new URL matches the last committed URL,
+  // then check window.location.href, and if it matches, trust it. The origin
   // check ensures that if a site somehow corrupts window.location.href it can't
   // do a redirect to a slow-loading target page while it is still loading to
-  // spoof the domain. On a document-changing URL change, the
+  // spoof the origin. On a document-changing URL change, the
   // window.location.href will match the previous URL at this stage, not the web
   // view's current URL.
   if (![_wkWebView isLoading]) {
     if (_documentURL == url)
       return;
     [self URLDidChangeWithoutDocumentChange:url];
-  } else if (!_documentURL.host().empty() &&
-             _documentURL.host() == url.host() &&
-             self.loadPhase != web::LOAD_REQUESTED) {
+  } else if ([self isKVOChangePotentialSameDocumentNavigationToURL:url]) {
     [_wkWebView evaluateJavaScript:@"window.location.href"
                  completionHandler:^(id result, NSError* error) {
                      // If the web view has gone away, or the location
@@ -1428,8 +1484,9 @@ WKWebViewErrorSource WKWebViewErrorSourceFromError(NSError* error) {
                      }
                      GURL jsURL([result UTF8String]);
                      // Make sure that the URL is as expected, and re-check
-                     // the host to prevent race conditions.
-                     if (jsURL == url && _documentURL.host() == url.host()) {
+                     // the origin to prevent race conditions.
+                     if (jsURL == url &&
+                         _documentURL.GetOrigin() == url.GetOrigin()) {
                        [self URLDidChangeWithoutDocumentChange:url];
                      }
                  }];