Visiting Restricted URL generate extra backward entries |
||||||||||||||
Issue descriptionChrome Version (from "Settings > About Google Chrome"): 67.0.3393.0 Device Type: (iPad 2, iPhone 4, etc): iPad Air 2, iOS 11.2 Steps to reproduce: 1) Go to iOS settings > Restrictions and set a site domain as restricted (say nba.com) 2) In Bling Canary omnibox, enter restricted domain (say nba.com) 3) Hit enter 4) Long tap on Back arrow Expected result: "New Tab" is the only entry in backward history. Actual result: Three entries exist: "Access Restricted", "www.nba.com", "New Tab"
,
Apr 11 2018
This is an interaction with the Content Filtering (aka parental control) feature in WKWebView. When the user navigates to a restricted URL, WKWebView fails the initial navigation and triggers|webView:didFailProvisionalNavigation:|, then it loads an error page in WKWebView. This is the "Access Restricted" entry in history. Meanwhile, with #slim-navigation-manager, Chrome has also kicked off a new navigation to load the placeholder URL into WKWebView in preparation for showing the native error view. This is the extra "www.nba.com" entry. Without #slim-navigation-manager, Chrome removes the WKWebView in |webView:didFailProvisionalNavigation:|, so the "Access Restricted" entry is never loaded. Attached is the screenshot of the "Access Restricted" page inserted by WKWebView. It's identical to what is shown in Safari in this situation. It makes it clear that Restriction is the cause of the error and provides a one-click option to whitelist the current URL. Here are the list of navigation events: 2018-04-11 15:17:04.867184-0400 Chromium[2592:487920] didStart [<WKNavigation: 0x10e0aa130>] http://www.nba.com/ 2018-04-11 15:17:04.980791-0400 Chromium[2592:487920] didFailProvisional [<WKNavigation: 0x10e0aa130>] about:blank?for=chrome%3A%2F%2Fnewtab%2F Error: Error Domain=WebKitErrorDomain Code=105 "The URL was blocked by a content filter" UserInfo={_WKRecoveryAttempterErrorKey=<WKReloadFrameErrorRecoveryAttempter: 0x1c4430fe0>, NSErrorFailingURLStringKey=http://www.nba.com/, NSErrorFailingURLKey=http://www.nba.com/, NSLocalizedDescription=The URL was blocked by a content filter} 2018-04-11 15:17:04.997172-0400 Chromium[2592:487920] didStart [(null)] http://www.nba.com/ 2018-04-11 15:17:05.019293-0400 Chromium[2592:487920] didCommit [(null)] http://www.nba.com/ 2018-04-11 15:17:05.100407-0400 Chromium[2592:487920] didFinish [(null)] http://www.nba.com/ 2018-04-11 15:17:05.182935-0400 Chromium[2592:487920] didStart [<WKNavigation: 0x10e2480b0>] about:blank?for=http%3A%2F%2Fwww.nba.com%2F 2018-04-11 15:17:05.198025-0400 Chromium[2592:487920] didCommit [<WKNavigation: 0x10e2480b0>] about:blank?for=http%3A%2F%2Fwww.nba.com%2F [0411/151705.198821:FATAL:crw_web_controller.mm(2720)] Check failed: _loadPhase == web::LOAD_REQUESTED (2 vs. 0) Option 1: we can preserve the current behavior in Chrome by detecting the "Access Restriction" navigation in |webView:didStartProvisionalNavigation:| and cancelling it. WKNavigation* being nil may be usable as a signature, though I'm not sure what other edge cases may cause WKNavigation* to be nil in this callback. Option 2: the WKWebView built-in error page seems more user friendly. We can check for error.code = 105 (assuming this is the undocumented error code for blocking by content filter) and abort |handleLoadError:| flow. eugenebut@: I think Option 1 makes sense now so we avoid introducing new user-visible behaviors in #slim-navigation-manager. Once the nav experiment is launched, we can come back and implement Option 2. They are about the same difficulty to implement, so I'm OK with going directly to Option 2 as well. WDYT?
,
Apr 11 2018
I think we should just go with Option #2 and fix crbug.com/793317 . Mardini, are you ok with changing the behavior for WK-based navigation?
,
Apr 12 2018
+mardini@ pkl@ also helped me verify on iOS 10 that WKWebView throws error code 105 just like iOS 11. So Option 2 should work for iOS 11 as well. Mardini - are you OK with changing to use system error UI for page blocked by Restriction when using WK-based navigation?
,
Apr 12 2018
Yes. I am fine with using the System Error UI for a page blocked by restrictions since this is really done at the OS level and not at the Chrome level and should be changed at the OS level if the user would like to change it. I am guessing a small user population would ever see these screens. I just want to make sure that we won't be showing the Safari UI that is in comment #2. i.e. we will show the WKWebView error inside our Chrome UI. We won't launch some SFViewController. + ainslie, pinkerton so that there are no surprises should you encounter this.
,
Apr 12 2018
Great. Thank you!
,
Apr 14 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2233628f5f5b32c7b458428f8d5cfbd0a18be82e commit 2233628f5f5b32c7b458428f8d5cfbd0a18be82e Author: Danyao Wang <danyao@chromium.org> Date: Sat Apr 14 17:14:06 2018 [Nav Experiment] Skip native error if URL is blocked by content filter. When a URL is blocked due to Restrictions settings, WKWebView triggers |webView:didFailProvisionalNavigation| and automatically triggers a second navigation that loads an error page. Without this change, Chrome injects an extra navigation entry for displaying native error, which shows up after the system "Access Restricted" error. Bug: 831381 , 793317 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: Iaafcc8f1c5abde120ad10d0794832ab09abeec6d Reviewed-on: https://chromium-review.googlesource.com/1012470 Reviewed-by: Eugene But <eugenebut@chromium.org> Commit-Queue: Danyao Wang <danyao@chromium.org> Cr-Commit-Position: refs/heads/master@{#550898} [modify] https://crrev.com/2233628f5f5b32c7b458428f8d5cfbd0a18be82e/ios/web/public/web_kit_constants.h [modify] https://crrev.com/2233628f5f5b32c7b458428f8d5cfbd0a18be82e/ios/web/web_state/ui/crw_web_controller.mm
,
Apr 16 2018
,
Apr 16 2018
,
Apr 16 2018
Removing merge request for now until tested in tomorrow's Canary build.
,
Apr 17 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2233628f5f5b32c7b458428f8d5cfbd0a18be82e commit 2233628f5f5b32c7b458428f8d5cfbd0a18be82e Author: Danyao Wang <danyao@chromium.org> Date: Sat Apr 14 17:14:06 2018 [Nav Experiment] Skip native error if URL is blocked by content filter. When a URL is blocked due to Restrictions settings, WKWebView triggers |webView:didFailProvisionalNavigation| and automatically triggers a second navigation that loads an error page. Without this change, Chrome injects an extra navigation entry for displaying native error, which shows up after the system "Access Restricted" error. Bug: 831381 , 793317 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: Iaafcc8f1c5abde120ad10d0794832ab09abeec6d Reviewed-on: https://chromium-review.googlesource.com/1012470 Reviewed-by: Eugene But <eugenebut@chromium.org> Commit-Queue: Danyao Wang <danyao@chromium.org> Cr-Commit-Position: refs/heads/master@{#550898} [modify] https://crrev.com/2233628f5f5b32c7b458428f8d5cfbd0a18be82e/ios/web/public/web_kit_constants.h [modify] https://crrev.com/2233628f5f5b32c7b458428f8d5cfbd0a18be82e/ios/web/web_state/ui/crw_web_controller.mm
,
Apr 24 2018
Verified on 68.0.3405.0 Canary in iOS 11.2.6(iPhone 8plus, ipad Mini) and iOS 10.3.3(iPhone 7plus, iPad mini2) issue is resolved no extra entries is shown on long pressing backward button
,
May 23 2018
Verified in: App Version: 67.0.3396.57 beta Devices: iPhone8, iPhone X, iPad Mini iOS Version: 11.2.6, 11.3.1, 11.4 beta 6 Issue is still reproducible. It seems like changes are not yet merged to M67. Please look in to it. Video: https://drive.google.com/open?id=1nEXxCO54zjdbu_RN52hCmaXVvtCdXYU0
,
May 23 2018
Kariah, Danyao - it seems this was merged to "testbranch"? not 67 branch. PTAL
,
May 23 2018
Sorry I must have made a typo when merging... Should I merge again?
,
May 23 2018
I know there was a bug earlier in this milestone that bugdroid was adding merge-merged-testbranch to lots of bugs. Can we verify first that this was added to testbranch and not M67. If so, yes please merge asap.
,
May 23 2018
I checked the M67 branch and the fix is indeed not yet merged. Merging now.
,
May 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5997eb8bc1adef874d95ac6a78ec8d39d0b60491 commit 5997eb8bc1adef874d95ac6a78ec8d39d0b60491 Author: Danyao Wang <danyao@chromium.org> Date: Wed May 23 16:03:41 2018 [Nav Experiment] Skip native error if URL is blocked by content filter. Cherrypick to refs/branch-heads/3396. When a URL is blocked due to Restrictions settings, WKWebView triggers |webView:didFailProvisionalNavigation| and automatically triggers a second navigation that loads an error page. Without this change, Chrome injects an extra navigation entry for displaying native error, which shows up after the system "Access Restricted" error. Bug: 831381 , 793317 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: Iaafcc8f1c5abde120ad10d0794832ab09abeec6d Reviewed-on: https://chromium-review.googlesource.com/1012470 Reviewed-by: Eugene But <eugenebut@chromium.org> Commit-Queue: Danyao Wang <danyao@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#550898}(cherry picked from commit 2233628f5f5b32c7b458428f8d5cfbd0a18be82e) Reviewed-on: https://chromium-review.googlesource.com/1070408 Reviewed-by: Danyao Wang <danyao@chromium.org> Cr-Commit-Position: refs/branch-heads/3396@{#685} Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428} [modify] https://crrev.com/5997eb8bc1adef874d95ac6a78ec8d39d0b60491/ios/web/public/web_kit_constants.h [modify] https://crrev.com/5997eb8bc1adef874d95ac6a78ec8d39d0b60491/ios/web/web_state/ui/crw_web_controller.mm
,
May 23 2018
,
May 24 2018
Verified in: App Version: 67.0.3396.59 beta Devices: iPhone 7, iPhone 8, iPad Mini iOS Versions: 10.3.3, 11.2.6, 11.4 beta 6 Issue is fixed. No extra entries shown in backward history for restricted websites. Video: https://drive.google.com/open?id=15NvL3R0aNKI5Hnm8R1cunZ95xU7pjc8y |
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by danyao@chromium.org
, Apr 10 2018