New issue
Advanced search Search tips

Issue 891722 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 18
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug

Blocking:
issue 841556



Sign in to add a comment

Fix android_webview.test.ClientOnReceivedHttpErrorTest.* with NS enabled

Project Member Reported by timvolod...@chromium.org, Oct 3

Issue description

currently 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

 
Status: Started (was: Assigned)
Blocking: 841556
Project Member

Comment 3 by bugdroid1@chromium.org, 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

Components: Mobile>WebView
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..

Cc: jam@chromium.org
+cc:jam@
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;
indeed this appears to solve the issue, thanks John!

uploaded patch for review:
https://chromium-review.googlesource.com/c/chromium/src/+/1286821

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Status: Verified (was: Fixed)
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