New issue
Advanced search Search tips

Issue 792515 link

Starred by 1 user
Status: Available
Owner: ----
Cc:
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 3
Type: Task

Blocking:
issue 661316
issue 788465



Sign in to add a comment

Deprecate _pendingNavigationInfo in CRWWebController

Project Member Reported by danyao@chromium.org, Dec 6 2017 
_pendingNavigationInfo, _loadingPhase, _lastRegisteredRequestURL in CRWWebController assume that there is only one provisional navigation. This assumption was true in UIWebView, but no longer true in WKWebView. They are the cause of race conditions and weird edge cases. Examples include  crbug.com/788464 , crbug.com/789993, crbug.com/737595

Patches landed over time has created a lot of tech debt in CRWWebController and made it increasingly hard to reasonable about behavior. See [CRWWebController -webViewLoadingStateDidChange] for an example.

A better solution is to store these navigation states in NavigationContextImpl.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 6 2017 

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

commit 7aae11ad65a6816a03771a80ec466504093f161c
Author: Danyao Wang <danyao@chromium.org>
Date: Wed Dec 06 23:06:18 2017

[ios] Save WKNavigationType in NavigationContextImpl.

This is the first step to removing _lastRegisteredRequestURL from
CRWWebController. See crbug.com/792515 for rationale.

There are two use cases for _lastRegisteredRequestURL:
1. As the URL of the pending navigation
2. Detect fast back-forward navigations in
   |-webViewLoadingStateDidChange| to explicitly trigger
   |-didFinishNavigation:|

NavigationContext::GetUrl() can already support (1).
NavigationContext::GetPageTransition() cannot current distinguish fast
and slow back-forward navigation. Storing WKNavigationType makes this
possible.

Bug: 789993,792515
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I4a919a48bd1d7755d006dbebff305558e885c8f2
Reviewed-on: https://chromium-review.googlesource.com/806766
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522235}
[modify] https://crrev.com/7aae11ad65a6816a03771a80ec466504093f161c/ios/web/web_state/navigation_context_impl.h
[modify] https://crrev.com/7aae11ad65a6816a03771a80ec466504093f161c/ios/web/web_state/navigation_context_impl.mm
[modify] https://crrev.com/7aae11ad65a6816a03771a80ec466504093f161c/ios/web/web_state/navigation_context_impl_unittest.mm
[modify] https://crrev.com/7aae11ad65a6816a03771a80ec466504093f161c/ios/web/web_state/ui/crw_web_controller.mm
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 7 2017 

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

commit d4f3e0f5257005ac109c6283f99d2ee6d7037883
Author: Danyao Wang <danyao@chromium.org>
Date: Thu Dec 07 00:16:28 2017

[ios] Remove _lastRegisteredRequestURL from -webView:didCommitNavigation

The removed code was originally added to fix a security bug in iOS 9
(crbug.com/610297). The issue seems to be fixed in WKWebView now so
this patch is no longer necessary.

This is step 2 to removing _lastRegisteredRequestURL from
CRWWebController.

Bug: 789993,792515,610297
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I2eaf5ec47d156ce71d724752fb6d35001c044b3f
Reviewed-on: https://chromium-review.googlesource.com/812174
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522265}
[modify] https://crrev.com/d4f3e0f5257005ac109c6283f99d2ee6d7037883/ios/web/web_state/ui/crw_web_controller.mm
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 7 2017 

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

commit 34c982d18a47e5f946072579a0e3ff20e6dcd60b
Author: Danyao Wang <danyao@chromium.org>
Date: Thu Dec 07 16:21:52 2017

[ios] Remove _lastRegisteredRequestURL from webViewLoadingStateDidChange

This is step 3 to removing _lastRegisteredRequestURL from
CRWWebController.

Bug: 789993,792515
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ib3ea5bb686a91e0d28cbb8ec7c1c860518c85090
Reviewed-on: https://chromium-review.googlesource.com/812407
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522443}
[modify] https://crrev.com/34c982d18a47e5f946072579a0e3ff20e6dcd60b/ios/web/web_state/ui/crw_web_controller.mm
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 7 2017 

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

commit 2c07baee5680bb2e0a453766dfcb40a20484ad96
Author: Danyao Wang <danyao@chromium.org>
Date: Thu Dec 07 17:28:11 2017

[ios] Remove DCHECKs related to _lastRegisteredRequestURL.

This is step 3 to removing _lastRegisteredRequestURL from
CRWWebController. This CL has no functional change.

Bug: 789993,792515
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ia415197025b4b3461fc807602e17063a753c39c1
Reviewed-on: https://chromium-review.googlesource.com/812446
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522460}
[modify] https://crrev.com/2c07baee5680bb2e0a453766dfcb40a20484ad96/ios/web/web_state/ui/crw_web_controller.mm
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 7 2017 

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

commit c2678879b8b6c3e9a075cf6aa4a8a422da69d598
Author: Danyao Wang <danyao@chromium.org>
Date: Thu Dec 07 18:38:27 2017

[ios] Finally remove _lastRegisteredRequestURL.

This is part 5 and final part for removing _lastRegisteredRequestURL
from CRWWebController.

Bug: 789993,792515
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I5707ff5ddbe61b3a2cfec76d667297753ef0c86f
Reviewed-on: https://chromium-review.googlesource.com/812489
Commit-Queue: Danyao Wang <danyao@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522487}
[modify] https://crrev.com/c2678879b8b6c3e9a075cf6aa4a8a422da69d598/ios/web/web_state/ui/crw_web_controller.mm
Project Member

Comment 6 by sheriffbot@chromium.org, Dec 10

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

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

Comment 7 by danyao@google.com, Dec 10

Status: Available (was: Untriaged)
Still want to do, but still low priority.
►

Sign in to add a comment