eugenebut could you help triage please?

Forgot to attach screenshot

Deleted: IMG_0076.jpg 122 KB

	IMG_0076.jpg 122 KB View Download

This happens because of WKWebView bug:

After tapping on SRP link and then back the following happens:

didStartProvisionalNavigation: is called for www.techrepublic.com navigation
didCommitNavigation: is called for www.techrepublic.com navigation
didFinishNavigation: is called for www.techrepublic.com navigation
didStartProvisionalNavigation: is called for back navigation
didCommitNavigation: is called for www.techrepublic.com navigation again

The last didCommitNavigation: callback should be called for www.techrepublic.com navigation, but corresponding WKNavigation object was deallocated (which is a bug). One way to fix the problem is probably to retain WKNavigation objects until the navigation is committed and finished.

Also WKWebView calls didFailProvisionalNavigation: for back navigation providing cancelled error code. 

Danyao, I can not reproduce WKNavigationDelegate callbacks bug with stock WKWebView. But what do you think about this fix: https://crrev.com/c/862542/ ?

The fact that we cannot reproduce WKNavigationDelegate callbacks make me suspicious that the bug is in our code, not necessarily a WKWebView bug.

What is the timing of the didFailProvisionalNavigation? Was it the provisional navigation to SRP have failed? The screenshot shows that SRP was loaded. Am I understanding correctly then in this case WKWebView called didFailProvisionalNavigation: yet continued to load the SRP page?

The once case I've seen didCommitNavigation: arrive after didFailProvisionalNavigation: is when |abortLoad| is called inside didStartProvisionalNavigation:. We do call |abortLoad| in several places. Could there be an unexpected interaction?

The bug can not be in Chrome code. WKWebView should not call didCommitNavigation: twice. The bug probably requires very specific use of WKWebView (like injecting scripts, specific creation timing, etc..), but it's still WKWebView bug. Usually it takes a lot of efforts to create a test case with stock WKWebView and in this case there is not much value in getting the fix from Apple (workaround is not needed for WK-based navigation, so even if the bug is fixed in iOS 12, it does not help us much).

didFailProvisionalNavigation: is called after the second didStartProvisionalNavigation: for SRP WKNavigation object. The page is loaded correctly.

I was a good idea to check for |abortLoad| calls, but Chrome does not make this call.

Ah I see what might be going on. After |didFailProvisionalNavigation:|, WKWebView.URL reverts to the previous item, because from WKWebView's point-of-view, the load failed. If my theory is correct, then the two didCommitNavigation: callbacks should have been given different WKNavigation* objects. Would it be hard for you to verify this?

It would be a WKWebView bug if the WKNavigation* object passed to the 2nd didCommitNavigation: callback is nil. If it is not, then WKWebView has not called didCommitNavigation: twice on the same navigation. It may still be a bug that didCommitNavigation: came after didFailProvisionalNavigation:, but I think it would make more sense to fix this bug by discarding any WKNavigationDelegate callback for a given WKNavigation* object after didFailProvisionalNavigation or didFailNavigation.

I've been thinking that our current practice of using WKWebView.URL as the URL of the in-progress navigation in WKNavigationDelegate callbacks is probably wrong. The contract only guarantees that WKNavigation* should be used as a stable identifier. Another example where we had tripped on this before is here: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?l=4436 (  crbug.com/784480  ). It would be more robust to use WKNavigation* to find NavigationContext and then use context->GetUrl(). Better yet, there maybe should be a pointer from NavigationContext to the NavigationItem. It's sort of there today via GetUniqueID(), but that is not always set.

Just want to note that the edge case for getting URL using WKNavigation* -> NavigationContext lookup is in renderer-initiated didStartProvisionalNavigation:, where we don't have a NavigationContext yet, and webView.URL is the only URL we have. 

Ideally, I'd like to have a way to tie WKNavigation* to the NavigationAction in decidePolicyForNavigationAction: so WKNavigation* becomes a true stable identifier of the navigation throughout its lifecycle. But that's totally out of the scope of this fix...

Both didCommitNavigation: callbacks receive the same WKNavigation* object, sorry did not explicitly write that.

Unfortunately WKWebView.URL is more reliable. In this very bug for example the second didCommitNavigation: call receives incorrect WKNavigation* object.

Looking up for WKNavigation* inside decidePolicyForNavigationAction: should be possible. We just do the lookup by URL as it's already done in decidePolicyForNavigationResponse:.

Thanks for verifying. It's very surprising that didCommitNavigation: callbacks received the same WKNavigation*, but I believe you. I have a couple of specific comment regarding the fix and will leave it on the CL.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3a4e87614a1a3877e0940020c55df7798e9ad8f3

commit 3a4e87614a1a3877e0940020c55df7798e9ad8f3
Author: Eugene But <eugenebut@google.com>
Date: Wed Jan 17 00:02:50 2018

Handle subsequent webView:didFinishNavigation: calls for the same WKNavigation.

Bug:  801000 , 727289
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Id7fa69b49fabe2c3b9d5e5d1e42c9979d6b37128
Reviewed-on: https://chromium-review.googlesource.com/862542
Reviewed-by: Danyao Wang <danyao@chromium.org>
Commit-Queue: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#529522}
[modify] https://crrev.com/3a4e87614a1a3877e0940020c55df7798e9ad8f3/ios/web/web_state/ui/crw_web_controller.mm
[modify] https://crrev.com/3a4e87614a1a3877e0940020c55df7798e9ad8f3/ios/web/web_state/ui/crw_wk_navigation_states.h
[modify] https://crrev.com/3a4e87614a1a3877e0940020c55df7798e9ad8f3/ios/web/web_state/ui/crw_wk_navigation_states.mm
[modify] https://crrev.com/3a4e87614a1a3877e0940020c55df7798e9ad8f3/ios/web/web_state/ui/crw_wk_navigation_states_unittest.mm

D'oh

Verified in M66.0.3329.0 canary
Device: iPhoneX, iPad Pro
iOS: 11.2.5, 11.2.2

Google Search Results page is displayed correctly after tapping on back button.

This bug requires manual review: Less than 21 days to go before AppStore submit on M65
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This landed before the branch point. Srikanthg, please retest with M-65

Looks good on M65.0.3325.61 beta
Verified on iPhone6s iOS11.3, iPhone8 iOS11.2.5

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot