Gracefully handle didCommitNavigation: call after didFinishNavigation:

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 49e05b2de8f502e2749c905d6061bec48b62bf71..f0cd6cea6d037fdbbe61c7ceba402c4dabaa84fa 100644
--- a/ios/web/web_state/ui/crw_web_controller.mm
+++ b/ios/web/web_state/ui/crw_web_controller.mm
@@ -4538,6 +4538,9 @@ registerLoadRequestForURL:(const GURL&)requestURL
     didCommitNavigation:(WKNavigation*)navigation {
   [self displayWebView];
 
+  bool navigationFinished = [_navigationStates stateForNavigation:navigation] ==
+                            web::WKNavigationState::FINISHED;
+
   // Record the navigation state.
   [_navigationStates setState:web::WKNavigationState::COMMITTED
                 forNavigation:navigation];
@@ -4629,10 +4632,21 @@ registerLoadRequestForURL:(const GURL&)requestURL
     UMA_HISTOGRAM_BOOLEAN("WebController.WKWebViewHasCertForSecureConnection",
                           static_cast<bool>(cert));
   }
+
+  if (navigationFinished) {
+    // webView:didFinishNavigation: was called before
+    // webView:didCommitNavigation:, so remove the navigation now and signal
+    // that navigation was finished.
+    [_navigationStates removeNavigation:navigation];
+    [self didFinishNavigation:navigation];
+  }
 }
 
 - (void)webView:(WKWebView*)webView
     didFinishNavigation:(WKNavigation*)navigation {
+  bool navigationCommitted =

[kkhorimoto, 2017/06/22 00:49:01]

Should we just early return if the navigation isn't committed yet?  Are we sure
that if we receive this, we'll always get a subsequent call to
didCommitNavigation where we can call into this method again?  This solution
changes the order in which the DidCommitNavigation() and DidFinishNavigation()
WSO callbacks are received, which may break chrome or components code.

[Eugene But (OOO till 7-30), 2017/06/22 01:58:06]

There is no change in NavigationItemCommitted/DidFinishNavigation order.
DidFinishNavigation is called when navigation is committed (it is Chromium
concept which is different from WKWebView).

I think we still should attempt to fetch favicons by calling 
__gCrWeb.didFinishNavigation.


This CL does 2 things:
  1.) Removes navigation on webView:didCommitNavigation: if
webView:didFinishNavigaiton: was called
  2.) Signals that page finished loading in webView:didCommitNavigation: if
webView:didFinishNavigaiton: was called

I don't know if it is possible to have webView:didFinishNavigaiton: call w/o
webView:didCommitNavigation:, but if it happens, then progress indicator and
network spinner will not be removed. That's something that we will likely will
notice.

[kkhorimoto, 2017/06/22 02:09:48]

whoops, I thought that the WSO didFinishNavigation was sent from
CRWWebController's |-didfinishNavigtion|.

+      [_navigationStates stateForNavigation:navigation] ==
+      web::WKNavigationState::COMMITTED;
   [_navigationStates setState:web::WKNavigationState::FINISHED
                 forNavigation:navigation];
 
@@ -4643,7 +4657,12 @@ registerLoadRequestForURL:(const GURL&)requestURL
   // appropriate time rather than invoking here.
   web::ExecuteJavaScript(webView, @"__gCrWeb.didFinishNavigation()", nil);

[kkhorimoto, 2017/06/22 00:49:02]

What does it mean to execute JS on a page when the navigation is finished, but
not committed?  Previously we assumed that if the page is committed, then JS
will be executed on that page's DOM, but now I'm not so sure.

[Eugene But (OOO till 7-30), 2017/06/22 01:58:06]

This fetches favicons, which still should work and should still be useful.

   [self didFinishNavigation:navigation];
-  [_navigationStates removeNavigation:navigation];
+
+  // Remove navigation only if it has been committed. Otherwise it will be
+  // removed in webView:didCommitNavigation: callback.
+  if (navigationCommitted) {
+    [_navigationStates removeNavigation:navigation];
+  }
 }
 
 - (void)webView:(WKWebView*)webView