New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 878060 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Aug 30
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug
Proj-Servicification

Blocked on:
issue 721398



Sign in to add a comment

Fix WebViewTest.ClearDataCache for Network Service

Project Member Reported by chongz@chromium.org, Aug 27

Issue description

(See  issue 721398 #c29 for more backgrounds)

We want to understand and fix this WebView test, but it won't block Canary.

 
Cc: roc...@chromium.org lazyboy@chromium.org
The problem seems to be that |webRequest.onResponseStarted| was not fired when the WebView is making XHR requests.

Stack trace tells me |webRequest.onResponseStarted| relies on |ChromeExtensionsNetworkDelegateImpl::OnResponseStarted|, which shouldn't be available when Network Service was enabled.

I'm not sure what's the statues of migrating WebView, should we:
1. Inject |ChromeExtensionsNetworkDelegate| into |NetworkServiceNetworkDelegate|, or
2. Do something about |WebRequestProxyingURLLoaderFactory|? (not sure what this factory does but it seems to be Network Service related)

+lazyboy@, rockot@ WDYT? Thanks!


Components: Platform>Extensions>API
Cc: cduvall@chromium.org
This will probably need a fix similar to https://chromium-review.googlesource.com/c/chromium/src/+/1165504/7/extensions/test/data/web_view/apitest/main.js

Due to http://crrev.com/c/1139048 there is a difference in behavior in the network service where web request listeners may not catch requests made immediately after they are registered. To solve this, you can load a page in the web view after the listener is registered, and then run the rest of the test.
I see, will give it a try. Thanks for the explanation!

cduvall@: Load the page again after listeners are registered fixed the problem in the description, thanks for the hint!

However I'm running into another problem where |response_from_cache| was always set to |false|.

I noticed that you had a TODO in |WebRequestInfo::AddResponseInfoFromResourceResponse()|[1], and I'm wondering do you have a plan to fix it in the near future, or should I do something less elegant to fix the test? (e.g. Communicate with |embedded_test_server| to check whether the url was loaded from there)

[1] https://cs.chromium.org/chromium/src/extensions/browser/api/web_request/web_request_info.cc?l=357&rcl=cc8fa1660d52dfe963ed8526026b898195399687

Looks like that TODO was added by rockot@, so I defer to him on this, but it seems like you could add request->was_cached() to ResourceResponseHead here: https://cs.chromium.org/chromium/src/services/network/url_loader.cc?l=56&rcl=9ed0f4e593446616bd7c32a5d9e89e24a3a180eb

And then you would be able to set the response_from_cache bit from that in AddResponseInfoFromResourceResponse.
I see, thanks for the pointer! Not sure why we don't have this in the first place though, will give it a try to see if there are any rabbit holes.
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 30

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

commit b94164ad9f4091897f33e3ddbdca70097c82181c
Author: Chong Zhang <chongz@chromium.org>
Date: Thu Aug 30 20:46:43 2018

NetworkService: Fix WebViewTest.ClearDataCache

This patch does 3 things to fix the test:
1. Register listeners before loading the actual guest page. See bug for
   more background.
2. Add |was_fetched_via_cache| to |ResourceResponseInfo|.
3. Call |NetworkContext::ClearHttpCache()| in
   |WebViewGuest::ClearData()| when Network Service was enabled. We
   cannot use |BrowsingDataRemover| because it doesn't support
   non-default |StoragePartition|.

Bug:  878060 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I5f0ea8e62ee8f44944428cfb041f9d4e427bd67c
Reviewed-on: https://chromium-review.googlesource.com/1195596
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Chong Zhang <chongz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587753}
[modify] https://crrev.com/b94164ad9f4091897f33e3ddbdca70097c82181c/chrome/test/data/extensions/platform_apps/web_view/clear_data_cache/embedder.js
[add] https://crrev.com/b94164ad9f4091897f33e3ddbdca70097c82181c/chrome/test/data/extensions/platform_apps/web_view/clear_data_cache/empty_guest.html
[modify] https://crrev.com/b94164ad9f4091897f33e3ddbdca70097c82181c/extensions/browser/api/web_request/web_request_info.cc
[modify] https://crrev.com/b94164ad9f4091897f33e3ddbdca70097c82181c/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/b94164ad9f4091897f33e3ddbdca70097c82181c/services/network/public/cpp/network_ipc_param_traits.h
[modify] https://crrev.com/b94164ad9f4091897f33e3ddbdca70097c82181c/services/network/public/cpp/resource_response.cc
[modify] https://crrev.com/b94164ad9f4091897f33e3ddbdca70097c82181c/services/network/public/cpp/resource_response_info.h
[modify] https://crrev.com/b94164ad9f4091897f33e3ddbdca70097c82181c/services/network/url_loader.cc
[modify] https://crrev.com/b94164ad9f4091897f33e3ddbdca70097c82181c/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Labels: M-71
Status: Fixed (was: Started)
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 31

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

commit d0a1ab991c0bb708e6a2e7da9b7bdffa6231a4cd
Author: calamity <calamity@chromium.org>
Date: Fri Aug 31 01:10:15 2018

Revert "NetworkService: Fix WebViewTest.ClearDataCache"

This reverts commit b94164ad9f4091897f33e3ddbdca70097c82181c.

Reason for revert: Suspected of causing
ExtensionWebRequestApiTest.WebRequestApiDoesNotCrashOnErrorAfterProfileDestroyed
failure in
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29/71221

Original change's description:
> NetworkService: Fix WebViewTest.ClearDataCache
> 
> This patch does 3 things to fix the test:
> 1. Register listeners before loading the actual guest page. See bug for
>    more background.
> 2. Add |was_fetched_via_cache| to |ResourceResponseInfo|.
> 3. Call |NetworkContext::ClearHttpCache()| in
>    |WebViewGuest::ClearData()| when Network Service was enabled. We
>    cannot use |BrowsingDataRemover| because it doesn't support
>    non-default |StoragePartition|.
> 
> Bug:  878060 
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: I5f0ea8e62ee8f44944428cfb041f9d4e427bd67c
> Reviewed-on: https://chromium-review.googlesource.com/1195596
> Reviewed-by: Tom Sepez <tsepez@chromium.org>
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Reviewed-by: Clark DuVall <cduvall@chromium.org>
> Commit-Queue: Chong Zhang <chongz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#587753}

TBR=rockot@chromium.org,tsepez@chromium.org,chongz@chromium.org,cduvall@chromium.org

Change-Id: I0a4f300e25d7fd9160d9a8e9ac02c8c733654818
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  878060 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1198642
Reviewed-by: calamity <calamity@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587892}
[modify] https://crrev.com/d0a1ab991c0bb708e6a2e7da9b7bdffa6231a4cd/chrome/test/data/extensions/platform_apps/web_view/clear_data_cache/embedder.js
[delete] https://crrev.com/1f0cd154e77461b58f0a7571175e847c9f95a558/chrome/test/data/extensions/platform_apps/web_view/clear_data_cache/empty_guest.html
[modify] https://crrev.com/d0a1ab991c0bb708e6a2e7da9b7bdffa6231a4cd/extensions/browser/api/web_request/web_request_info.cc
[modify] https://crrev.com/d0a1ab991c0bb708e6a2e7da9b7bdffa6231a4cd/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/d0a1ab991c0bb708e6a2e7da9b7bdffa6231a4cd/services/network/public/cpp/network_ipc_param_traits.h
[modify] https://crrev.com/d0a1ab991c0bb708e6a2e7da9b7bdffa6231a4cd/services/network/public/cpp/resource_response.cc
[modify] https://crrev.com/d0a1ab991c0bb708e6a2e7da9b7bdffa6231a4cd/services/network/public/cpp/resource_response_info.h
[modify] https://crrev.com/d0a1ab991c0bb708e6a2e7da9b7bdffa6231a4cd/services/network/url_loader.cc
[modify] https://crrev.com/d0a1ab991c0bb708e6a2e7da9b7bdffa6231a4cd/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 31

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

commit b5d3b92437cd039580d5921f304bf898d86c7305
Author: Chong Zhang <chongz@chromium.org>
Date: Fri Aug 31 19:10:58 2018

Reland "NetworkService: Fix WebViewTest.ClearDataCache"

This is a reland of b94164ad9f4091897f33e3ddbdca70097c82181c

The original patch was reverted accidentally in
https://crrev.com/c/1198642.

TBR=cduvall@chromium.org

Original change's description:
> NetworkService: Fix WebViewTest.ClearDataCache
>
> This patch does 3 things to fix the test:
> 1. Register listeners before loading the actual guest page. See bug for
>    more background.
> 2. Add |was_fetched_via_cache| to |ResourceResponseInfo|.
> 3. Call |NetworkContext::ClearHttpCache()| in
>    |WebViewGuest::ClearData()| when Network Service was enabled. We
>    cannot use |BrowsingDataRemover| because it doesn't support
>    non-default |StoragePartition|.
>
> Bug:  878060 
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: I5f0ea8e62ee8f44944428cfb041f9d4e427bd67c
> Reviewed-on: https://chromium-review.googlesource.com/1195596
> Reviewed-by: Tom Sepez <tsepez@chromium.org>
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Reviewed-by: Clark DuVall <cduvall@chromium.org>
> Commit-Queue: Chong Zhang <chongz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#587753}

Bug:  878060 
Change-Id: Ief60260db74bdb98ca1826e345fa6036176a4426
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1199611
Reviewed-by: Chong Zhang <chongz@chromium.org>
Commit-Queue: Chong Zhang <chongz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588110}
[modify] https://crrev.com/b5d3b92437cd039580d5921f304bf898d86c7305/chrome/test/data/extensions/platform_apps/web_view/clear_data_cache/embedder.js
[add] https://crrev.com/b5d3b92437cd039580d5921f304bf898d86c7305/chrome/test/data/extensions/platform_apps/web_view/clear_data_cache/empty_guest.html
[modify] https://crrev.com/b5d3b92437cd039580d5921f304bf898d86c7305/extensions/browser/api/web_request/web_request_info.cc
[modify] https://crrev.com/b5d3b92437cd039580d5921f304bf898d86c7305/extensions/browser/guest_view/web_view/web_view_guest.cc
[modify] https://crrev.com/b5d3b92437cd039580d5921f304bf898d86c7305/services/network/public/cpp/network_ipc_param_traits.h
[modify] https://crrev.com/b5d3b92437cd039580d5921f304bf898d86c7305/services/network/public/cpp/resource_response.cc
[modify] https://crrev.com/b5d3b92437cd039580d5921f304bf898d86c7305/services/network/public/cpp/resource_response_info.h
[modify] https://crrev.com/b5d3b92437cd039580d5921f304bf898d86c7305/services/network/url_loader.cc
[modify] https://crrev.com/b5d3b92437cd039580d5921f304bf898d86c7305/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Sign in to add a comment