New issue
Advanced search Search tips

Issue 864525 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug


Participants' hotlists:
Slim-Nav-Burndown


Sign in to add a comment

DCHECK hit in CRWWebController -webView:didReceiveAuthenticationChallenge:completionHandler:

Project Member Reported by gambard@chromium.org, Jul 17

Issue description

With slim navigation manager enabled.

What steps will reproduce the problem?
(1) Open a NTP
(2) Load a page
(3) Interrupt the navigation by going back (before the end of the page load)

What is the expected result?
Everything should be fine.

What happens instead?
I hit the first DCHECK in .
https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?type=cs&sq=package:chromium&g=0&l=4917

I am able to reproduce it 1/5, but I hit it pretty often.

Assigning to danyao@ as I hit it with the flag enabled.
 
Status: Started (was: Assigned)
Thanks for filing this bug. I think the fix is to just remove the DCHECK. It turns out that in back/forward navigation, -webView:didReceiveAuthenticationChallenge callback sometimes arrives (most likely for subresource load on the previous page) after WKWebView.URL is already updated to the new item. So my assumption that this is only called before the navigation finishes is incorrect.

Looking at the code, this method doesn't use any states associated with self.currentNavItem. So it's safe to just run it through.
Labels: Proj-WKBackForwardList
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 25

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

commit 0f8a2964d432cf3a0cf213e35aaa26961eef20a8
Author: Danyao Wang <danyao@chromium.org>
Date: Wed Jul 25 16:26:14 2018

[Nav Experiment] Remove DCHECK for incorrect assumption.

Previously I assumed -webView:didReceivedAuthenticationChallenge callback
is always called for the navigation associated with the current
WKWebView.URL value. In testing, it turns out in back/forward navigation,
this callback may be triggered for a subresource on the previous item
after WKWebView.URL has already been updated to the new item. As a result,
this DCHECK is incorrectly triggered when navigating back to NTP from a
web page before the web page finishes loading completely.

From code inspection, the implementation of this delegate call doesn't
depend on states associated with self.currentNavItem. This scenario
already exists between web pages. So the DCHECK is not protecting any
meaningful state corruption.

Bug:  864525 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I02a1678721e2db59ed1f288c9e4ec7139efc8632
Reviewed-on: https://chromium-review.googlesource.com/1147328
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577924}
[modify] https://crrev.com/0f8a2964d432cf3a0cf213e35aaa26961eef20a8/ios/web/web_state/ui/crw_web_controller.mm

Status: Fixed (was: Started)
I found another DCHECK hit in |webView:didFailNavigation| that is wrong for a similar reason:
https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?l=4948
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 26

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

commit c2c325c70e9688de84e5c5c32471ba711a2a18e7
Author: Danyao Wang <danyao@chromium.org>
Date: Thu Jul 26 17:55:26 2018

[Nav Experiment] Update incorrect DCHECK assumption in didFailNavigation

This DCHECK is meant to assert that placeholder navigation never fails.
However, webView.URL may have reverted to the previous entry when a
failure happens so is not always the URL that triggers the error. The
correct approach is to check the error URL.

Bug:  864525 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I4efb01041a39f87c4c1ac36115c93364bd41aff9
Reviewed-on: https://chromium-review.googlesource.com/1151391
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578359}
[modify] https://crrev.com/c2c325c70e9688de84e5c5c32471ba711a2a18e7/ios/web/web_state/ui/crw_web_controller.mm

Sign in to add a comment