Issue metadata
Sign in to add a comment
|
Fix WebViewTest.ClearDataCache for Network Service |
||||||||||||||||||||||||
Issue description(See issue 721398 #c29 for more backgrounds) We want to understand and fix this WebView test, but it won't block Canary.
,
Aug 27
,
Aug 28
,
Aug 28
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.
,
Aug 28
I see, will give it a try. Thanks for the explanation!
,
Aug 29
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
,
Aug 29
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.
,
Aug 29
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.
,
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
,
Aug 30
,
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
,
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 |
|||||||||||||||||||||||||
Comment 1 by chongz@chromium.org
, Aug 27