Fix android_webview.test.ClientOnReceivedHttpErrorTest.* with NS enabled |
||||||
Issue descriptioncurrently failing with NS: -org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testAfterRedirect -org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testForMainResource -org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testForSubresource -org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testForUserGesture -org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testNotCalledIfNoHttpError
,
Oct 9
,
Oct 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/77e3a8b37b2c6c164f8569ff366c8ccf48bd5a5f commit 77e3a8b37b2c6c164f8569ff366c8ccf48bd5a5f Author: Tim Volodine <timvolodine@chromium.org> Date: Fri Oct 12 16:52:25 2018 [Android WebView] Add InterceptedRequest class and OnReceivedHttpError callback. Add InterceptedRequest class, which represents an in-progress network request and allows to receive callbacks at certain points in the process and control the flow. This patch also adds code for the OnReceivedHttpError callback, which fixes the following tests: - org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testAfterRedirect - org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testForMainResource - org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testForSubresource - org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testNotCalledIfNoHttpError This patch in more detail: - add IntercepedRequest internal class - add code necessary for AwContentsClientBridge lookup - detect http errors and code for the onReceivedHttpError callback - update tests filter BUG= 891722 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: I2123a30b0c9ea8c3f07c34adc0fedb6bc69ca8f3 Reviewed-on: https://chromium-review.googlesource.com/c/1259019 Commit-Queue: Tim Volodine <timvolodine@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Clark DuVall <cduvall@chromium.org> Reviewed-by: Richard Coles <torne@chromium.org> Cr-Commit-Position: refs/heads/master@{#599250} [modify] https://crrev.com/77e3a8b37b2c6c164f8569ff366c8ccf48bd5a5f/android_webview/browser/DEPS [modify] https://crrev.com/77e3a8b37b2c6c164f8569ff366c8ccf48bd5a5f/android_webview/browser/aw_content_browser_client.cc [modify] https://crrev.com/77e3a8b37b2c6c164f8569ff366c8ccf48bd5a5f/android_webview/browser/aw_proxying_url_loader_factory.cc [modify] https://crrev.com/77e3a8b37b2c6c164f8569ff366c8ccf48bd5a5f/android_webview/browser/aw_proxying_url_loader_factory.h [modify] https://crrev.com/77e3a8b37b2c6c164f8569ff366c8ccf48bd5a5f/testing/buildbot/filters/mojo.fyi.network_webview_instrumentation_test_apk.filter
,
Oct 12
,
Oct 12
so here: org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testForUserGesture is still failing.. C 101.869s Main [FAIL] org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest#testForUserGesture: C 101.869s Main java.lang.AssertionError C 101.869s Main at org.junit.Assert.fail(Assert.java:86) C 101.869s Main at org.junit.Assert.assertTrue(Assert.java:41) C 101.869s Main at org.junit.Assert.assertTrue(Assert.java:52) C 101.870s Main at org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testForUserGesture(ClientOnReceivedHttpErrorTest.java:161) C 101.870s Main at java.lang.reflect.Method.invoke(Native Method) ------- basically fails the assert: Assert.assertTrue(request.hasUserGesture); [1] [1] https://cs.chromium.org/chromium/src/android_webview/javatests/src/org/chromium/android_webview/test/ClientOnReceivedHttpErrorTest.java?l=161 which is a bit strange because I am simply passing network::ResourceRequest.has_user_gesture..
,
Oct 12
+cc:jam@
,
Oct 16
sorry for the delay. The old path generates hasUserGesture in AwWebResourceRequest's constructor, which gets it from ResourceRequestInfo::HasUserGesture. That struct gets constructed 2 ways, depending on if it's a subresource or navigation. For subresources: https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc?l=995 this gets it directly from network::ResourceRequest, same as network service path, so we should be fine. For navigations: https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc?rcl=b683690689a9176cf7e0096082f05e85659434e0&l=1561 this gets it from NavigationRequestInfo's common_params.has_user_gesture. It looks like in the navigation case, navigation_url_loader_impl.cc when it creates the ResourceRequest object it doesn't set the has_user_gesture field. Probably because the network service doesn't use it. The fix would be to change that file's CreateResourceRequest method to add new_request->method = request_info->common_params.has_user_gesture;
,
Oct 17
indeed this appears to solve the issue, thanks John! uploaded patch for review: https://chromium-review.googlesource.com/c/chromium/src/+/1286821
,
Oct 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5a8004ef413b75defefa1202584392a6b06db038 commit 5a8004ef413b75defefa1202584392a6b06db038 Author: Tim Volodine <timvolodine@chromium.org> Date: Wed Oct 17 23:01:10 2018 [Android WebView] Make sure to set has_user_gesture when creating ResourceRequest As was observed in https://crbug.com/891722 by jam@ the user_gesture is not being set in the network service code path, resulting in failing user_gesture related instrumentation tests. This patch fixes the issue. Fixes tests: -org.chromium.android_webview.test.ClientOnReceivedHttpErrorTest.testForUserGesture BUG= 891722 ,841556 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: I25be23261c178cd43b1e46440dd96dce89f70bf7 Reviewed-on: https://chromium-review.googlesource.com/c/1286821 Commit-Queue: Tim Volodine <timvolodine@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#600587} [modify] https://crrev.com/5a8004ef413b75defefa1202584392a6b06db038/content/browser/loader/navigation_url_loader_impl.cc [modify] https://crrev.com/5a8004ef413b75defefa1202584392a6b06db038/testing/buildbot/filters/mojo.fyi.network_webview_instrumentation_test_apk.filter
,
Oct 18
,
Oct 18
C 71.673s Main ******************************************************************************** C 71.673s Main Summary C 71.673s Main ******************************************************************************** C 71.674s Main [==========] 10 tests ran. C 71.674s Main [ PASSED ] 10 tests. C 71.674s Main ******************************************************************************** |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by timvolod...@chromium.org
, Oct 3