New issue
Advanced search Search tips

Issue 864769 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug

Blocked on:
issue 899383


Participants' hotlists:
Slim-Nav-Cleanup


Sign in to add a comment

NULL NavigationItem in NavigationContext in webView:didFinishNavigation

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

Issue description

This was causing low rate crashes at https://chromium.googlesource.com/chromium/src/+/68.0.3440.42/ios/web/web_state/ui/crw_web_controller.mm#4876.
See https://bugs.chromium.org/p/chromium/issues/detail?id=851969#c16

For some reason, web::GetItemWithUniqueID(self.navigationManagerImpl, context->GetNavigationItemUniqueID()) returns nullptr. Code inspection shows that context->GetNavigationItemUniqueID() is always set whenever a navigation context is created. So the only other possibility is somehow by the time |webView:didFinishNavigation|, the pending item originally associated with the navigation context disappeared.

This only happens when #slim-navigation-manager is enabled.
 
Cc: eugene...@chromium.org
Maybe |webView:didFinishNavigation| is called twice or just out of order with other callbacks?
Blockedon: 899383
Re Comment#1: possible. I should add a UMA counter to see how often repeated calls of |webView:didFinishNavigation| really happens.

We already have a counter for |webView:didCommitNavigation| happening before |webView:didFinishNavigation|: https://uma.googleplex.com/p/chrome/timeline_v2?sid=c58940b4fb816dcfabf6820708fe9c0b
It seems to be happening at really low fractions (~0.035%). Any thoughts on what else we can do to figure out what's causing these behaviors?
In theory with legacy navigation manager then pending navigation item can be discarded before webView:didFinishNavigation:. This will leave us with empty navigation manager.
Labels: -Pri-3 Postmortem-Followup Pri-2
Use this bug to track the corresponding action item from postmortem: https://docs.google.com/document/d/1fPylc7WQEClKhI_1HkrKUwXShJvk0UwG5Gj0TxFk9oA/edit
I looked into the hypothesis that "calling AddPendingItem() before WKWebView triggers |webView:didCommitNavigation| for the previous in-flight navigation can trigger the invariant violation". This is a dead end.

AddPendingItem() is only ever called in -[CRWWebController registerLoadRequestForURL:referrer:transition:sameDocumentNavigation:hasUserGesture:]. If a pending item already exists when the latter is called, no new pending item is created. The URL of the existing pending item may be updated: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.mm?rcl=7dc5533c186a0f0d134b25b6f9882101a34c1843&l=1416 This code has been around for a while to handle client-side redirect. Even though reusing existing pending item may be a separate bug, at least there is no way to clobber the existing pending item by starting a browser-initiated navigation.

A new renderer-initiated navigation can clobber the pending item if and only if the pending item is a placeholder URL. This is because placeholder item is not committed until either WebUI or native content is presented in |webView:didFinishNavigation|. From WKWebView's perspective, it is safe to start a new navigation after |webView:didCommitNavigation|. However, this case still would not trigger the crash seen in the postmortem from Comment #5 because the crashing line is inside a !IsWKInternalURL() check.

Sign in to add a comment