[PlzNavigate] WebView causes onReceivedError on redirects with invalid url schemes |
|||||||||
Issue descriptionSee b/69352309 for context. WebView should not complain when handling URLs with incorrect url schemes as long as those URLs are handled in shouldOverrideUrlLoading. With PlzNavigate we are checking the schemes of URLs (that are the result of a redirect) before passing them to ShouldOverrideUrlLoading - this is incorrect and should be fixed. Should this target / be merged to 63?
,
Nov 27 2017
,
Nov 27 2017
,
Nov 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4793571a1e9a585b6143bc63f2525dbea5c62906 commit 4793571a1e9a585b6143bc63f2525dbea5c62906 Author: Gustav Sennton <gsennton@google.com> Date: Tue Nov 28 17:34:51 2017 Call shouldOverrideUrlLoading before verifying redirect url. WebView should not post onReceivedError when shouldOverrideUrlLoading returns true for invalid url schemes - with PlzNavigate we do post onReceivedError when the url checked is a server redirect. This CL fixes that issue by calling shouldOverrideUrlLoading before verifying that the scheme of the new target url in the redirect is valid. Note: if the url is overridden we still update the state of the navigation handle to point to the new url, this is to ensure the WebView (Aw)WebContentsObserver sees the new url. BUG= 788775 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I4ad36e3ae3feaa39a00024282752c02bbe88bd6f Reviewed-on: https://chromium-review.googlesource.com/790993 Commit-Queue: Gustav Sennton <gsennton@chromium.org> Reviewed-by: Richard Coles <torne@chromium.org> Reviewed-by: Camille Lamy <clamy@chromium.org> Cr-Commit-Position: refs/heads/master@{#519724} [modify] https://crrev.com/4793571a1e9a585b6143bc63f2525dbea5c62906/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java [modify] https://crrev.com/4793571a1e9a585b6143bc63f2525dbea5c62906/content/browser/frame_host/navigation_handle_impl.cc [modify] https://crrev.com/4793571a1e9a585b6143bc63f2525dbea5c62906/content/browser/frame_host/navigation_handle_impl.h [modify] https://crrev.com/4793571a1e9a585b6143bc63f2525dbea5c62906/content/browser/frame_host/navigation_request.cc
,
Nov 29 2017
,
Nov 29 2017
This bug requires manual review: We are only 5 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 29 2017
,
Nov 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fd1da640d9f8b273ba1d786531cc119445733341 commit fd1da640d9f8b273ba1d786531cc119445733341 Author: Gustav Sennton <gsennton@google.com> Date: Wed Nov 29 16:29:16 2017 Call shouldOverrideUrlLoading before verifying redirect url. WebView should not post onReceivedError when shouldOverrideUrlLoading returns true for invalid url schemes - with PlzNavigate we do post onReceivedError when the url checked is a server redirect. This CL fixes that issue by calling shouldOverrideUrlLoading before verifying that the scheme of the new target url in the redirect is valid. Note: if the url is overridden we still update the state of the navigation handle to point to the new url, this is to ensure the WebView (Aw)WebContentsObserver sees the new url. BUG= 788775 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation Change-Id: I4ad36e3ae3feaa39a00024282752c02bbe88bd6f Reviewed-on: https://chromium-review.googlesource.com/790993 Commit-Queue: Gustav Sennton <gsennton@chromium.org> Reviewed-by: Richard Coles <torne@chromium.org> Reviewed-by: Camille Lamy <clamy@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#519724}(cherry picked from commit 4793571a1e9a585b6143bc63f2525dbea5c62906) Reviewed-on: https://chromium-review.googlesource.com/797090 Cr-Commit-Position: refs/branch-heads/3239@{#608} Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578} [modify] https://crrev.com/fd1da640d9f8b273ba1d786531cc119445733341/android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientShouldOverrideUrlLoadingTest.java [modify] https://crrev.com/fd1da640d9f8b273ba1d786531cc119445733341/content/browser/frame_host/navigation_handle_impl.cc [modify] https://crrev.com/fd1da640d9f8b273ba1d786531cc119445733341/content/browser/frame_host/navigation_handle_impl.h [modify] https://crrev.com/fd1da640d9f8b273ba1d786531cc119445733341/content/browser/frame_host/navigation_request.cc
,
Nov 29 2017
Marking this as fixed for now. Will follow-up with the test-team on the buganizer bug - b/69352309
,
Dec 4 2017
Verified on Pixel Xl/ OPM1.171019.013 having Monochrome 64.0.3282.7 , Login issue no longer reproducible with the apk provided on b/69352309 as per comment #11. Thanks for the fix.
,
Dec 5 2017
Verified on 63.0.3239.83 on Pixel 2XL/OPM1.171019.013 with apk provided and no longer issue is reproducible mentioned in b/69352309 |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by clamy@chromium.org
, Nov 27 2017