BrowsingDataRemover owners: can you help with this task? Thanks

Sure :)

I'll take care of the BrowsingDataRemover<->disk_cache::Backend calls.

Regarding CookieStore, is someone already implementing this between StoragePartition<->CookieStore? If so, we can just reuse that in BrowsingDataRemover. If not, I could take it as well.

(just saw this)

Thanks for helping. I'm not sure sure I understand the question in comment 3, perhaps we can have a quick VC to chat about what this work entails? 

Randy's working on hooking up the out-of-process cookie store ( issue 721395 ).  That presumably includes hooking up the cookie-related BrowsingDataRemover calls.

+Randy

Yep.  See https://docs.google.com/document/d/1s1vL8hNDPvOhssgsp8HjgL9yKZMzwo50yY5Hy8X6iE4/edit for the cookie design.  msramek@: Toss a pointer to your design at me when you have it, and I'll comment.  I presume this is just making sure that when our communication pipes meet, they fit :-}.

I'll note that in contrast to the initial post, it's my intent to switch cookie access over without regard to the network service flag.  When we have network service configuration installed, we'll integrate cookie configuration with that--until then, I intend to put a cookie interface on the storage partition for use by the current cookie consumers.  See the design.

unassigning due to lack of movement

Apologies, applied the wrong component in bulk.

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

commit 674513ec5b6af4e8da73e5b23ddd94a97fd565c0
Author: Matt Falkenhagen <falken@chromium.org>
Date: Fri Dec 08 02:11:34 2017

Network Service: Sheriffing: Disable a newly enabled ClearSiteData test.

r522414 re-enabled ClearSiteDataThrottleBrowserTest.CacheIntegrationTest,
which is failing in NetworkService, probably for the same reason as the other
ClearSiteData tests.

Build: https://ci.chromium.org/buildbot/chromium.fyi/Mojo%20Linux/8035

Output:
../../content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:880: Failure
Expected equality of these values:
  2u
    Which is: 2
  cache_keys.size()
    Which is: 4
[  FAILED  ] ClearSiteDataThrottleBrowserTest.CacheIntegrationTest, where TypeParam =  and GetParam() =  (2363 ms)

Bug:  721398 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Ibec3b45ab790a41a00abc17e7b9aef00da138574
TBR: kinuko
NOTRY: true
Reviewed-on: https://chromium-review.googlesource.com/816454
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522679}
[modify] https://crrev.com/674513ec5b6af4e8da73e5b23ddd94a97fd565c0/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter

Eric: I believe you're working on this right?

Right.  Been working off of more specific bugs, but I suppose it make sense to own this overall bug too.

great, thanks

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

commit e7e11b59a8b3fcc274bd29c7d8a80cbfb27a30db
Author: Eric Orth <ericorth@chromium.org>
Date: Wed May 16 21:06:37 2018

Remove net/ imports from BrowsingDataRemoval.

Usage of net/ code has been moved to NetworkContext IPCs as part of
servicification.

Bug:  721398 
Change-Id: I2fdcac208d5e402b2c53fb62f86e426c122cae1b
Reviewed-on: https://chromium-review.googlesource.com/1053839
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559284}
[modify] https://crrev.com/e7e11b59a8b3fcc274bd29c7d8a80cbfb27a30db/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate.cc
[modify] https://crrev.com/e7e11b59a8b3fcc274bd29c7d8a80cbfb27a30db/content/browser/browsing_data/browsing_data_filter_builder_impl.cc
[modify] https://crrev.com/e7e11b59a8b3fcc274bd29c7d8a80cbfb27a30db/content/browser/browsing_data/browsing_data_remover_impl.cc

Hey Eric, I'm reopening this as there are still some failures:

content_browsertests:
-ClearSiteDataThrottleBrowserTest.CacheIntegrationTest
-ClearSiteDataThrottleBrowserTest.CookiesIntegrationTest
-ClearSiteDataThrottleBrowserTest.Credentials
-ClearSiteDataThrottleBrowserTest.CredentialsOnRedirect
-ClearSiteDataThrottleBrowserTest.RedirectNavigation
-ClearSiteDataThrottleBrowserTest.RedirectResourceLoad
-ClearSiteDataThrottleBrowserTest.SecureAndInsecureResourceLoad
-ClearSiteDataThrottleBrowserTest.ServiceWorker
-ClearSiteDataThrottleBrowserTest.StorageServiceWorkersIntegrationTest
-ClearSiteDataThrottleBrowserTest.Types

browser_tests:
-WebViewTest.ClearData
-WebViewTest.ClearDataCache

I think these are because storage_partition_http_cache_data_remover.cc is still calling net/ directly, i.e. StoragePartitionHttpCacheDataRemover::DoClearCache

Thanks

Yes, looks like I missed the stuff in StoragePartitionHttpCacheDataRemover, although I would argue that, while clearly very related, that's not directly part of BrowsingDataRemover.  Maybe a separate bug to cover those would be appropriate?

I'm not sure I understand comment 21. My understanding is that BrowsingDataRemover is split across content and chrome; the StoragePartitionHttpCacheDataRemover code is called by the content part.

I would be very wary of trusting anything from the following tests until they are reworked,
as those all access the cache backend directly with network service on via CacheTestUtil class 
in content::; I think they're probably getting the in-memory fallback backend which is completely
separate from the cache actually used by the network service process.
(Hopefully, as sharing the cache dir would be a recipe for data corruption).

ConditionalCacheDeletionHelperBrowserTest
    ConditionalCacheDeletionHelper is what backs StoragePartitionHttpCacheDataRemover.

    It seems to only be used as the fallback for NetworkContext.ClearHttpCache for the media 
    cache from BrowsingDataRemoverImpl:
       https://cs.chromium.org/chromium/src/content/browser/browsing_data/browsing_data_remover_impl.cc?rcl=9aeecc40704b65ff20988cdcb1f32c76c28b0184&l=429
       (I think the comment there is a mistake, when network service is enabled this should not be 
        called at all).

    and from here:
       https://cs.chromium.org/chromium/src/extensions/browser/guest_view/web_view/web_view_guest.cc?rcl=f448fd4ed19a01120a82166ba3f59190ceb9635a&l=768 

    (Is there something preventing the latter from using BrowsingDataRemover?)

    Given there won't be a media cache in network service, I think removing that second site would 
    render those tests moot for network service, right, since they would be testing code that's dead 
    for the config?

ClearSiteDataThrottleBrowserTest 
     What was mentioned in comment #19 above. 

ConditionalCacheCountingHelperBrowserTest
    What I've been porting that got me looking around for context on this stuff,
    in particular trying to figure out what to do with CacheTestUtil.
                        

Sorry, missed the latest updates here.

BrowsingDataRemover is split across content and chrome (with the chrome side being called from BrowsingDataRemover via ChromeBrowsingDataRemoverDelegate).  While StoragePartitionHttpCacheDataRemover is clearly an important part of removing browser data, and it should be servicified, I do not believe it is directly part of the responsibility of the BrowsingDataRemover class.

So I did not consider that covered by this bug and recommend opening a separate one for other browsing data removals not under BrowsingDataRemover.  Or it's not a big deal if we want to just expand the scope of this bug to cover the other stuff.

Either way, I have completed the part of this that I intended to do, so I'll unassign myself to leave this open for somebody else to handle if we want to do more work as part of this bug.

Will take a look.

FWIW, I think I just ported the one test use of StoragePartitionHttpCacheDataRemover/ConditionalCacheDeletionHelper to just use the NetworkContext method.

(In context of a pending, not yet-mailed-out, CL that is).

Just an update: I noticed that "Clear-Site-Data" header (for both cookie and cache) was handled by `ClearSiteDataThrottle : public ResourceThrottle`, which won't work with Network Service.

My assumption is that we want to change them to using WebContentsObserver and NavigationHandle as well.

jam@ Just want to double check what we are trying to fix here:

1. All |ClearSiteDataThrottleBrowserTest.*| tests mentioned in #c19 are actually caused by the Clear-Site-Data feature which was not migrated yet (i.e. We are not actually handling the response header). And it may take > 2 weeks to fully migrate the feature (|ResourceThrottle| => |URLLoaderThrottle| on both navigation and subresources).
  * Clear-Site-Data: https://www.w3.org/TR/clear-site-data/
  * Sample CL: https://crrev.com/c/1136728

2. The |WebViewTest.*| tests failed before they ever tried to clear the cache, and I'm still trying to figure out the cause.

3. The method |StoragePartitionHttpCacheDataRemover::DoClearCache()| mentioned in #c19 seems to be used in multiple tests but they are actually not failing. Maybe it could be removed or needs more test coverage.
  * Test patch: https://crrev.com/c/1144474

My Questions:
Do we really consider 1. as a Canary blocker, or do we actually care more about 3. (and maybe 2. as well)?

Thanks!

Given that this is privacy related, I think marking it as canary blocking sounds good. I would expect 1 & 3 to work. 3 might not show up as test failures because of bugs, but we definitely want that to work (e.g. users manually removing data).

Re #c30: Sounds reasonable, thanks for the clarification!

Also one other option for subresources is that we pass the "Clear-Site-Data" in resource_load_info.mojom's ResourceLoadInfo. Then the browser will automatically see this, and you can skip having a urlloaderthrottle in the renderer.

mkwst@ msramek@: Can I have some more comments on the requirements for Clear-Site-Data?

More specifically can we clear the data async without deferring the requests?
e.g. Currently requests are deferred in 'clear_site_data_throttle.cc' but I don't find such requirements in the spec[2].

As mentioned in #c32 it could make the implementation much easier if we could pass the header and handle them inside:
Navigation:
 - |WebContentsObserver::DidRedirectNavigation(NavigationHandle*)|
 - |WebContentsObserver::ReadyToCommitNavigation(NavigationHandle*)| (or |DidFinishNavigation(NavigationHandle*)|?)
Subresources:
 - |WebContentsObserver::ResourceLoadComplete(ResourceLoadInfo)|

[1] https://cs.chromium.org/chromium/src/content/browser/browsing_data/clear_site_data_throttle.cc?l=310&rcl=7a3fc24348616ad5126a1f9f508523abffb6be72
[2] https://www.w3.org/TR/clear-site-data/

This is captured in spec, in 3.2: https://www.w3.org/TR/clear-site-data/#fetch-integration

"data MUST be cleared before rendering the response"

We had long discussions about this design aspect. If the website only wants to clear data on logout, it doesn't matter much if the deletion is synchronous or not. However, if the website wants the user session to continue after the deletion (e.g. after resetting itself after being compromised), it must know when the deletion has finished. With a JS API, this could be a callback, but with a header, we need to block. Note also that an asynchronous behavior can be simulated by synchronous one, by simply fetching a subresource which includes Clear-Site-Data and does nothing else. So synchronous behavior is superior to asynchronous.

Re #c34: Ah I see, thanks for the pointer and detailed explanation! I guess I will have to use |URLLoaderThrottle| on both browser and renderer to retain current behavior then.

Others may want to simulate the asynchronous / synchronous behavior but that'll be orthogonal to this task.

 issue 877639  tracks the |StoragePartition::ClearHttpAndMediaCaches()| issue and doesn't block Canary.

The patch to migrate Clear-Site-Data is landing soon.

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

commit fdbb7e9d9e56bbcec88157b539f0418c1fb744fa
Author: Chong Zhang <chongz@chromium.org>
Date: Fri Aug 24 23:40:54 2018

NetworkService: Migrate Clear-Site-Data

This patch copied 'clear_site_data_throttle.h/.cc' into
'clear_site_data_handler.h/.cc' with the following differences:
1. The new handler will be called through the |NetworkServiceClient|
   pipeline to work with Network Service.
2. The new handler doesn't have special handling for Service Worker.
  * The assumption is that the response from network process
    (NetworkServiceClient) won't be modified by Service Worker.
3. The new handler won't defer console messages for navigations. Future
   works could fix this to match current behavior (marked as TODO).
4. The new handler will always defer the request if it has
   'Clear-Site-Data' header, and resume after the browser finished
   processing.

We have to duplicate the codepath because
|NetworkServiceNetworkDelegate| currently doesn't work without Network
Service, and the new handler interface has no information about Service
Worker.

This patch fixed all Clear-Site-Data related tests (10 browser tests
and 2 layout tests).

crbug.com/876931 tracks remaining non-Canary blocking TODOs.

Bug:  721398 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I065140ab49ab3647cba0e64717440d0f1bf42095
Reviewed-on: https://chromium-review.googlesource.com/1170200
Commit-Queue: Chong Zhang <chongz@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586058}
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/browser/BUILD.gn
[add] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/browser/browsing_data/clear_site_data_handler.cc
[add] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/browser/browsing_data/clear_site_data_handler.h
[rename] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/browser/browsing_data/clear_site_data_handler_browsertest.cc
[add] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/browser/browsing_data/clear_site_data_handler_unittest.cc
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/browser/browsing_data/clear_site_data_throttle.cc
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/browser/browsing_data/clear_site_data_throttle.h
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/browser/network_service_client.cc
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/browser/network_service_client.h
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/public/test/browser_test_utils.cc
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/public/test/browser_test_utils.h
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/content/test/BUILD.gn
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/net/url_request/url_request.cc
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/net/url_request/url_request.h
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/services/network/network_service_network_delegate.cc
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/services/network/network_service_network_delegate.h
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/services/network/network_service_unittest.cc
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/services/network/public/mojom/network_service.mojom
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/services/network/test/test_network_service_client.cc
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/services/network/test/test_network_service_client.h
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/services/network/url_loader_unittest.cc
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter
[modify] https://crrev.com/fdbb7e9d9e56bbcec88157b539f0418c1fb744fa/third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService