Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2368923003:
Support the Clear-Site-Data header on resource requests (Closed)
|
Created:
4 years, 2 months ago by msramek Modified:
3 years, 6 months ago CC:
blink-worker-reviews_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, kinuko+watch, loading-reviews_chromium.org, nasko+codewatch_chromium.org, Randy Smith (Not in Mondays) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport the Clear-Site-Data header on resource requests
Until now, it was only supported for navigations.
Changes in this CL:
1. Convert the NavigationThrottle to a ResourceThrottle. The two classes
are sufficiently similar that these are mostly syntactical changes,
except that the ResourceThrottle lives on the IO thread and needs to
occasionally jump to the UI thread.
2. Instantiate it in ResourceDispatcherHostImpl instead of
NavigationHandleImpl. This requires adding an explicit DEPS rule.
3. Add some restrictions - for example, we will not support service worker
requests or LOAD_DO_NOT_SET_COOKIES. These are then tested in the
unittest.
4. Add browsertests for resource requests, and some integration tests
that check not only calls to BrowsingDataRemover, but also the actual
removal of data.
BUG=607897
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2368923003
Cr-Commit-Position: refs/heads/master@{#477612}
Committed: https://chromium.googlesource.com/chromium/src/+/9bc8902cf5825d5cbcdbf2b7766241f2f9c96bbb
Patch Set 1 #
Total comments: 17
Patch Set 2 : Addressed comments. #
Total comments: 12
Patch Set 3 : Addressed comments, improved the SW test #
Total comments: 6
Patch Set 4 : Wait for SW activation. #
Total comments: 12
Patch Set 5 : Some improvements. #
Total comments: 34
Patch Set 6 : Made the class more mockable, added unittests #Patch Set 7 : Rebase (base::Value refactoring, ResourceController not exposed anymore, etc.) #Patch Set 8 : Fixed tests. #Patch Set 9 : Add integration tests. #Patch Set 10 : Fix the compilation error #
Total comments: 10
Patch Set 11 : Addressed comments, added some debug outputs. #
Total comments: 71
Patch Set 12 : Addressed comments. #
Total comments: 1
Patch Set 13 : Rebase. #
Total comments: 41
Patch Set 14 : Addressed comments. #
Total comments: 15
Patch Set 15 : Addressed comments. #
Total comments: 32
Patch Set 16 : Addressed comments #
Total comments: 4
Patch Set 17 : Addressed comments, formatted. #Messages
Total messages: 160 (103 generated)
Description was changed from ========== Support the Clear-Site-Data header on resource requests Until now, it was only supported for navigations. Changes in this CL: 1. Convert the NavigationThrottle to a ResourceThrottle. The two classes are sufficiently similar that these are mostly syntactical changes, except that the ResourceThrottle lives on the IO thread and needs to occasionally jump to the UI thread. 2. Instantiate it in ResourceDispatcherHostImpl instead of NavigationHandleImpl. This requires adding an explicit DEPS rule. 3. Add restrictions to the *Create() methods - for example, we will not support service worker requests or LOAD_DO_NOT_SET_COOKIES. These are then tested in the unittest. This requires adding CONTENT_EXPORT to UrlRequestServiceWorkerData. 4. Add browsertests for resource requests. A browsertest for a service worker request, including its .html and .js files, is currently marked DISABLED (WIP). BUG=607897 ========== to ========== Support the Clear-Site-Data header on resource requests Until now, it was only supported for navigations. Changes in this CL: 1. Convert the NavigationThrottle to a ResourceThrottle. The two classes are sufficiently similar that these are mostly syntactical changes, except that the ResourceThrottle lives on the IO thread and needs to occasionally jump to the UI thread. 2. Instantiate it in ResourceDispatcherHostImpl instead of NavigationHandleImpl. This requires adding an explicit DEPS rule. 3. Add restrictions to the *Create() methods - for example, we will not support service worker requests or LOAD_DO_NOT_SET_COOKIES. These are then tested in the unittest. This requires adding CONTENT_EXPORT to UrlRequestServiceWorkerData. 4. Add browsertests for resource requests. A browsertest for a service worker request, including its .html and .js files, is currently marked DISABLED (WIP). BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
msramek@chromium.org changed reviewers: + mkwst@chromium.org, mmenke@chromium.org
Hi Matt and Mike! Can you please take a first pass on this CL? Before I loop in other content/ owners, I primarily want to know whether content/browser/loader is the right place to put this :) Note that in the meantime I'm still working on one browsertest (currently marked DISABLED, see point #4 in the CL description), please ignore that for now. But the rest of the tests give a good coverage, so the CL is pretty much finished. Cheers, Martin
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Just looked at the ServiceWorker stuff, which I don't think this CL is handling correctly. May take another look later today, just wanted to raise these high level concerns sooner for faster turnaround time. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:91: if (load_flags & net::LOAD_DO_NOT_SAVE_COOKIES) Using this load flag isn't reliable - NetworkDelegate has another method to check if a request can save cookies, in addition to this load flag. Not sure the best way to handle this. If this were only for cookies, I'd say this would be best in URLRequestHttpJob, but it's not. :( https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:102: if (ResourceRequestInfo::OriginatedFromServiceWorker(request)) This doesn't seem right. We don't care if the request is from a ServiceWorker, but if it's going to a service worker. The way things work: renderer->URLRequest (With possibly bogus URL)->ServiceWorker->URLRequest (With real URL, assuming it doesn't go through a second ServiceWorker). We want to catch the first URLRequest, not the second, and I think this only catches the second. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:365: // of the test server is chosen randomly. The service worker should redirect requests to |origin2| to |origin1|, so we don't actually need a server at origin2, do we?
https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:161: return; Do we get here for WebSockets? I don't think we do. Should we run this code for them? https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:164: if (!IsOriginSecure(current_url_)) { Is this the right check? localhost is considered secure over HTTP, file URLs are considered secure, as are some filesystem URLs. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.h:82: const base::Closure& callback); Can we move these two static methods to an anonymous namespace in the CC file instead? https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:415: Should also check CORS and non-CORS cross-site resource requests.
Oh, and I do think this is the best place to hook this stuff up. If it were just cookies, net/ would be the right place to do it, but it's not. Worth noting that this is behind the SafeBrowsing throttle (Which is good, just thought I'd point it out - the SafeBrowsing throttle is the one member of "pre_mime_sniffing_throttles". Could add a test for that, not sure how concerned we are about that case, since it can only delete stuff). I think it's worth noting that if a response sets cookies, and has the Clear-Site-Data header, this code will set those cookies and then clear cookies for the site, not the other way around. Fixing that is non-trivial, if that's considered a bug.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hello again Matt! Thanks for all the comments and for your attempt to speed up the iteration - as you see, it didn't work out on my side :) It took me some time to learn more about service workers and CORS, play with different scenarios, run them by Mike, and find out how to implement them. It's possible that you'll tell me that I still have a naive understanding, but well, it's time for a second patchset :) I had to rebase inbetween changes, so the diff between patchsets will show a lot of changes in resource_dispatcher_host_impl.cc and navigation_handle_impl.cc; but the clear_site_data* files were not touched in the meantime, so the important changes should still be easy to review. On 2016/09/27 18:28:32, mmenke wrote: > Oh, and I do think this is the best place to hook this stuff up. If it were > just cookies, net/ would be the right place to do it, but it's not. Acknowledged. > > Worth noting that this is behind the SafeBrowsing throttle (Which is good, just > thought I'd point it out - the SafeBrowsing throttle is the one member of > "pre_mime_sniffing_throttles". Could add a test for that, not sure how > concerned we are about that case, since it can only delete stuff). I think that's desirable. Not that a malware site could do much harm with this, but still, it's reasonable if this is only executed after the user proceeds through the SB interstitial / confirms download. > > I think it's worth noting that if a response sets cookies, and has the > Clear-Site-Data header, this code will set those cookies and then clear cookies > for the site, not the other way around. Fixing that is non-trivial, if that's > considered a bug. Thanks, this might be actually also worth noting in the spec. I don't think it's a bug - hopefully web developers with this kind of setup will understand what is happening. The throttle delays the resource load and it's supported on redirects, so the correct setup would be to fetch a resource that will call Clear-Site-Data and redirect to another resource that sets new cookies.
https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:91: if (load_flags & net::LOAD_DO_NOT_SAVE_COOKIES) On 2016/09/26 12:59:16, mmenke wrote: > Using this load flag isn't reliable - NetworkDelegate has another method to > check if a request can save cookies, in addition to this load flag. Not sure > the best way to handle this. If this were only for cookies, I'd say this would > be best in URLRequestHttpJob, but it's not. :( Hmm... So I'm really not an expert on these things, but I think this check might be enough. There are several things to consider w.r.t. cookies: 1. Chrome's internal pings to services which don't want to set cookies, and those use LOAD_DO_NOT_SAVE_COOKIES. 2. As I mentioned in another comment, the 'credentials' parameter in fetch(). That one is actually also reflected as LOAD_DO_NOT_SAVE_COOKIES here. 3. Expiration timestamps. We do not have anything like that here. 4. Content settings - cookies could be blocked on a particular site or blocked for third party in general. mkwst@ has argued that this should not affect us - the intention of a user blocking cookies is that they do not want cookies, not that they want to prevent cookies from being modified, therefore deletion should be fine. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:102: if (ResourceRequestInfo::OriginatedFromServiceWorker(request)) On 2016/09/26 12:59:17, mmenke wrote: > This doesn't seem right. We don't care if the request is from a ServiceWorker, > but if it's going to a service worker. > > The way things work: renderer->URLRequest (With possibly bogus > URL)->ServiceWorker->URLRequest (With real URL, assuming it doesn't go through a > second ServiceWorker). > > We want to catch the first URLRequest, not the second, and I think this only > catches the second. Thanks for the explanation. From the name of the method, I wrongly assumed it means "originated from a URL in the scope of a service worker". But in the end, that's not what we want either; mkwst@ pointed out that we don't want to completely disable the feature for service workers - it should work at least for the update pings. I filed a bug for the spec here: https://github.com/w3c/webappsec-clear-site-data/issues/5 It seems that ServiceWorkerUrlRequestJob inserts ServiceWorkerResponseInfo into the responses it handles. So its existence should be the signal we're looking for. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:161: return; On 2016/09/27 18:24:02, mmenke wrote: > Do we get here for WebSockets? I don't think we do. Should we run this code > for them? I played with some WebSockets demos on the web and the throttle wasn't even created for them. Not sure why, since I thought that a successful request must consult each of the throttles? But in any case, Clear-Site-Data in a WebSockets handshake sounds like a corner case that we probably don't care much about (for now at least). https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:164: if (!IsOriginSecure(current_url_)) { On 2016/09/27 18:24:02, mmenke wrote: > Is this the right check? localhost is considered secure over HTTP, file URLs > are considered secure, as are some filesystem URLs. Localhost over HTTP is hopefully fine, and useful for testing. file:// URLs don't have response headers, so we should return on the previous line. I have never tried filesystem: API, but delegating the IsOriginSecure() check to the inner URL sounds reasonable. So yes, I would trust IsOriginSecure() to do the right thing. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.h:82: const base::Closure& callback); On 2016/09/27 18:24:02, mmenke wrote: > Can we move these two static methods to an anonymous namespace in the CC file > instead? Done. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:288: EXPECT_CALL(*GetContentBrowserClient(), ClearSiteData(_, _, _, _, _, _)) Removing this as it's duplicated below. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:365: // of the test server is chosen randomly. On 2016/09/26 12:59:17, mmenke wrote: > The service worker should redirect requests to |origin2| to |origin1|, so we > don't actually need a server at origin2, do we? The first part of the test makes a fetch from |origin2| before installing the service worker, and expects it to succeed - we need the server there. Technically, we don't need to test that - successful instances are well covered by the other tests. But I felt that a test which is fully negative has so many ways to accidentally pass when something is broken in the setup that I added one successful fetch to make it a bit more robust. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:415: On 2016/09/27 18:24:02, mmenke wrote: > Should also check CORS and non-CORS cross-site resource requests. I chatted with mkwst@ about this as well. While Access-Control-Allow-Origin et al. influence the visibility of the response, they do not affect whether cookies are set. And therefore, they should not affect Clear-Site-Data either. In fact, the check against Access-Control-Allow-Origin is performed in Blink after the throttle's destructor, so we can't easily do it even if we chose to. However, the 'credentials' parameter does affect how cookies are set, and therefore it should influence Clear-Site-Data as well. Again, I filed a bug against the spec: https://github.com/w3c/webappsec-clear-site-data/issues/4 and added a test here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:91: if (load_flags & net::LOAD_DO_NOT_SAVE_COOKIES) On 2016/10/13 14:26:03, msramek wrote: > On 2016/09/26 12:59:16, mmenke wrote: > > Using this load flag isn't reliable - NetworkDelegate has another method to > > check if a request can save cookies, in addition to this load flag. Not sure > > the best way to handle this. If this were only for cookies, I'd say this > would > > be best in URLRequestHttpJob, but it's not. :( > > Hmm... So I'm really not an expert on these things, but I think this check might > be enough. There are several things to consider w.r.t. cookies: > > 1. Chrome's internal pings to services which don't want to set cookies, and > those use LOAD_DO_NOT_SAVE_COOKIES. > > 2. As I mentioned in another comment, the 'credentials' parameter in fetch(). > That one is actually also reflected as LOAD_DO_NOT_SAVE_COOKIES here. > > 3. Expiration timestamps. We do not have anything like that here. > > 4. Content settings - cookies could be blocked on a particular site or blocked > for third party in general. mkwst@ has argued that this should not affect us - > the intention of a user blocking cookies is that they do not want cookies, not > that they want to prevent cookies from being modified, therefore deletion should > be fine. I'll defer to you guys here. https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... content/browser/browsing_data/clear_site_data_throttle.cc:161: return; On 2016/10/13 14:26:03, msramek wrote: > On 2016/09/27 18:24:02, mmenke wrote: > > Do we get here for WebSockets? I don't think we do. Should we run this code > > for them? > > I played with some WebSockets demos on the web and the throttle wasn't even > created for them. Not sure why, since I thought that a successful request must > consult each of the throttles? > > But in any case, Clear-Site-Data in a WebSockets handshake sounds like a corner > case that we probably don't care much about (for now at least). Websockets have an entirely different API - they don't even have bodies, for instance, so they don't go through the ResourceLoader stack. https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:71: // TODO(msramek): Find a solution without polling. Use TitleWatcher from browser_test_utils.h instead? https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:170: // We're telling the server what to serve, and the XSS auditor will nit: Avoid "we" in comments, due to ambiguity. https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:427: } test_cases[] = { const kTestCases? https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:438: for (struct TestCase& test_case : test_cases) { const TestCase& (struct isn't needed, const seems relevant) https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:444: // Fetch a resource. Note that we also handle fetch() error, as we will get nit: --we (x2) https://codereview.chromium.org/2368923003/diff/40001/content/test/data/brows... File content/test/data/browsing_data/worker.js (right): https://codereview.chromium.org/2368923003/diff/40001/content/test/data/brows... content/test/data/browsing_data/worker.js:24: })); Should we instead / additionally get a network response from origin1 and confirm that data for origin1 is cleared, but origin2 is not?
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
In this patchset, I think I finally got the service workers right. Matt, PTAL! Mike - also PTAL! Most importantly, I'd like you to verify my claim in one of the above comments that LOAD_DO_NOT_SAVE_COOKIES is the only cookie-related test we need. Thanks! https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:71: // TODO(msramek): Find a solution without polling. On 2016/10/13 17:16:31, mmenke wrote: > Use TitleWatcher from browser_test_utils.h instead? Done. Nice! I knew that I couldn't be the first one who needed something like this... https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:170: // We're telling the server what to serve, and the XSS auditor will On 2016/10/13 17:16:31, mmenke wrote: > nit: Avoid "we" in comments, due to ambiguity. Done. https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:427: } test_cases[] = { On 2016/10/13 17:16:31, mmenke wrote: > const kTestCases? Done. https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:438: for (struct TestCase& test_case : test_cases) { On 2016/10/13 17:16:31, mmenke wrote: > const TestCase& (struct isn't needed, const seems relevant) Done. Yes, that's what I wanted to write :) https://codereview.chromium.org/2368923003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:444: // Fetch a resource. Note that we also handle fetch() error, as we will get On 2016/10/13 17:16:31, mmenke wrote: > nit: --we (x2) Done. https://codereview.chromium.org/2368923003/diff/40001/content/test/data/brows... File content/test/data/browsing_data/worker.js (right): https://codereview.chromium.org/2368923003/diff/40001/content/test/data/brows... content/test/data/browsing_data/worker.js:24: })); On 2016/10/13 17:16:31, mmenke wrote: > Should we instead / additionally get a network response from origin1 and confirm > that data for origin1 is cleared, but origin2 is not? Done. Yes, that makes more sense actually. I rewrote the test to request several resources, some handled by the service worker, some sent through. This means that the one request sent before the service worker was installed that I had here is no longer needed. Hopefully the JS code is now also more readable. I also discovered that my check for ServiceWorkerResponseInfo in the throttle was not sufficient; requests that go to the service worker but are not handled there (but restarted and re-sent to network instead) still have ServiceWorkerResponseInfo attached. However, ResourceResponseInfo::was_fetched_via_service_worker is false, which distinguishes those cases.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mmenke@chromium.org changed reviewers: + falken@chromium.org
[+falken]: Mind reviewing this, from a ServiceWorker standpoint? Basically, when we get this magic header from a server, we want to clear site data, but we don't want to do it if we get the same header as output from a ServiceWorker, since it can do all sorts of funky things, like redirect the request to another domain. [msramek]: I'm going to let mkwst and falken do a pass before I do another full pass (May take a second look at tests in the meantime, though)
service worker interaction lgtm % comments https://codereview.chromium.org/2368923003/diff/80001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/80001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:403: // i.e. that it isn't always 1 as in the case of |origin1| and |origin2|. Nice documentation! https://codereview.chromium.org/2368923003/diff/80001/content/test/data/brows... File content/test/data/browsing_data/worker.js (right): https://codereview.chromium.org/2368923003/diff/80001/content/test/data/brows... content/test/data/browsing_data/worker.js:72: )); Nicely written test. https://codereview.chromium.org/2368923003/diff/80001/content/test/data/brows... File content/test/data/browsing_data/worker_setup.html (right): https://codereview.chromium.org/2368923003/diff/80001/content/test/data/brows... content/test/data/browsing_data/worker_setup.html:14: console.log('Service worker registered.'); This isn't quite right, the service worker is still being installed at this point, so it won't intercept fetches. You should wait for it to activate. You can do this using .ready: navigator.serviceWorker.register('/?file=worker.js') .then(() => navigator.serviceWorker.ready) .then(() => { document.title = 'service worker is ready'; }); .catch(err => { console.log('error'); });
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review, and mainly for the pointers you gave me in my email question a few days ago :) https://codereview.chromium.org/2368923003/diff/80001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/80001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:403: // i.e. that it isn't always 1 as in the case of |origin1| and |origin2|. On 2016/10/19 07:14:38, falken wrote: > Nice documentation! Thanks! https://codereview.chromium.org/2368923003/diff/80001/content/test/data/brows... File content/test/data/browsing_data/worker.js (right): https://codereview.chromium.org/2368923003/diff/80001/content/test/data/brows... content/test/data/browsing_data/worker.js:72: )); On 2016/10/19 07:14:38, falken wrote: > Nicely written test. Thanks again :) *blush* https://codereview.chromium.org/2368923003/diff/80001/content/test/data/brows... File content/test/data/browsing_data/worker_setup.html (right): https://codereview.chromium.org/2368923003/diff/80001/content/test/data/brows... content/test/data/browsing_data/worker_setup.html:14: console.log('Service worker registered.'); On 2016/10/19 07:14:38, falken wrote: > This isn't quite right, the service worker is still being installed at this > point, so it won't intercept fetches. You should wait for it to activate. You > can do this using .ready: > > navigator.serviceWorker.register('/?file=worker.js') > .then(() => navigator.serviceWorker.ready) > .then(() => { document.title = 'service worker is ready'; }); > .catch(err => { console.log('error'); }); Done. I just left out the catch() part, as the exception will be logged to the console automatically, and it's more informative than a generic error message.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/13 at 17:16:31, mmenke wrote: > https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... > File content/browser/browsing_data/clear_site_data_throttle.cc (right): > > https://codereview.chromium.org/2368923003/diff/1/content/browser/browsing_da... > content/browser/browsing_data/clear_site_data_throttle.cc:91: if (load_flags & net::LOAD_DO_NOT_SAVE_COOKIES) > On 2016/10/13 14:26:03, msramek wrote: > > On 2016/09/26 12:59:16, mmenke wrote: > > > Using this load flag isn't reliable - NetworkDelegate has another method to > > > check if a request can save cookies, in addition to this load flag. Not sure > > > the best way to handle this. If this were only for cookies, I'd say this > > would > > > be best in URLRequestHttpJob, but it's not. :( > > > > Hmm... So I'm really not an expert on these things, but I think this check might > > be enough. There are several things to consider w.r.t. cookies: > > > > 1. Chrome's internal pings to services which don't want to set cookies, and > > those use LOAD_DO_NOT_SAVE_COOKIES. > > > > 2. As I mentioned in another comment, the 'credentials' parameter in fetch(). > > That one is actually also reflected as LOAD_DO_NOT_SAVE_COOKIES here. > > > > 3. Expiration timestamps. We do not have anything like that here. > > > > 4. Content settings - cookies could be blocked on a particular site or blocked > > for third party in general. mkwst@ has argued that this should not affect us - > > the intention of a user blocking cookies is that they do not want cookies, not > > that they want to prevent cookies from being modified, therefore deletion should > > be fine. > > I'll defer to you guys here. Martin's explanation makes sense to me here. For the last bit, just to clarify: I do think that a user's intent when blocking cookies is to prevent modification of cookies, but I don't think that intent would preclude wiping all the data from an origin. That kind of purely negative action seems like a reasonable thing to do, and in keeping with the user's intent to "block cookies" from a given origin. > > But in any case, Clear-Site-Data in a WebSockets handshake sounds like a corner > > case that we probably don't care much about (for now at least). > > Websockets have an entirely different API - they don't even have bodies, for instance, so they don't go through the ResourceLoader stack. WebSockets should not cause `Clear-Site-Data` behavior to trigger.
The tests look very thorough. Thank you! LGTM.
On 2016/10/20 at 11:55:52, Mike West wrote: > The tests look very thorough. Thank you! > > LGTM. (But please wait for mmenke@, or anyone else who knows more about throttles than I do, to sign off on the usage).
Sorry for slowness, didn't get back to tests, and doesn't look like I'll get to them today. https://chromiumcodereview.appspot.com/2368923003/diff/100001/content/browser... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://chromiumcodereview.appspot.com/2368923003/diff/100001/content/browser... content/browser/browsing_data/clear_site_data_throttle.cc:146: DCHECK(success); This is all just request->load_flags()...May be simplest to just check this only when we see the new header. load flags can be updated / modified both between when the throttle is created and when it's first sent, and during redirects. I'm more concerned about when it's set than when it's cleared, though I'm not sure this paritucular flag is ever modified, once set. That does leave us in an odd state where in WillRedirectRequest, the flag could either have been set or cleared between receiving the headers and when |this| is called, but...erm...Let's ignore that case. https://chromiumcodereview.appspot.com/2368923003/diff/100001/content/browser... content/browser/browsing_data/clear_site_data_throttle.cc:195: *defer = clearing_in_progress_; I think the code is easier to follow if HandleHeader takes / sets defer instead. (And I'd suggest getting rid of clearing_in_progress_, as then it's no longer needed, except for DCHECKs). https://chromiumcodereview.appspot.com/2368923003/diff/100001/content/browser... content/browser/browsing_data/clear_site_data_throttle.cc:200: *defer = clearing_in_progress_; This behavior should probably be documented in the header - pausing the request long enough for relevant data to be cleared. https://chromiumcodereview.appspot.com/2368923003/diff/100001/content/browser... content/browser/browsing_data/clear_site_data_throttle.cc:239: headers->GetNormalizedHeader(kClearSiteDataHeader, &header_value); Rather than two searches for the header, can just replace HasHeader above with GetNormalizedHeader. Not a huge savings, but no need to waste CPU time. https://chromiumcodereview.appspot.com/2368923003/diff/100001/content/browser... content/browser/browsing_data/clear_site_data_throttle.cc:257: if (origin.unique()) { Should this go up with the IsOriginSecure check? Seems to make sense to have all url validation in one place.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
No worries Matt, I don't perceive your reviews as slow :) (and in any case, this must be done properly rather than fast) https://codereview.chromium.org/2368923003/diff/100001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/100001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:146: DCHECK(success); On 2016/10/20 19:03:10, mmenke wrote: > This is all just request->load_flags()...May be simplest to just check this only > when we see the new header. load flags can be updated / modified both between > when the throttle is created and when it's first sent, and during redirects. > I'm more concerned about when it's set than when it's cleared, though I'm not > sure this paritucular flag is ever modified, once set. > > That does leave us in an odd state where in WillRedirectRequest, the flag could > either have been set or cleared between receiving the headers and when |this| is > called, but...erm...Let's ignore that case. Good to know! I moved this to HandleHeader(). That also makes it possible to output a console message. Also changed it to load_flags() (somehow I didn't notice that before). https://codereview.chromium.org/2368923003/diff/100001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:195: *defer = clearing_in_progress_; On 2016/10/20 19:03:10, mmenke wrote: > I think the code is easier to follow if HandleHeader takes / sets defer instead. > (And I'd suggest getting rid of clearing_in_progress_, as then it's no longer > needed, except for DCHECKs). Done. Since the "success" of HandleHeader() is equivalent to "should defer", I just made HandleHeader() return a boolean. I think "return false" / "return true" reads better than setting "*defer=false" at the beginning of the method and then "*defer=true" at the end. But let me know if you prefer to use bool* instead of a return value anyway. It seems like pretty much all implementations of ResourceController::Resume() have the same DCHECK already, so we can safely remove it. https://codereview.chromium.org/2368923003/diff/100001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:200: *defer = clearing_in_progress_; On 2016/10/20 19:03:10, mmenke wrote: > This behavior should probably be documented in the header - pausing the request > long enough for relevant data to be cleared. It is mentioned in the class-level comment. Or do you mean to add the comment to WillRedirectRequest() and WillProcessResponse() as well? https://codereview.chromium.org/2368923003/diff/100001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:239: headers->GetNormalizedHeader(kClearSiteDataHeader, &header_value); On 2016/10/20 19:03:11, mmenke wrote: > Rather than two searches for the header, can just replace HasHeader above with > GetNormalizedHeader. Not a huge savings, but no need to waste CPU time. Done. Good catch. https://codereview.chromium.org/2368923003/diff/100001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:257: if (origin.unique()) { On 2016/10/20 19:03:10, mmenke wrote: > Should this go up with the IsOriginSecure check? Seems to make sense to have > all url validation in one place. Done. Yeah, makes sense. Now that I'm looking at it, I'm not sure if a unique origin can be secure. But IsOriginSecure takes a GURL instead of an origin so it's not easy to verify. I'd keep the check to be sure.
I like the tests, which seem admirably complete, though they do miss two things (The we wait for the clear, and that we handle redirects). Also wonder if we could make the unit tests more integrationy. https://codereview.chromium.org/2368923003/diff/100001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/100001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:200: *defer = clearing_in_progress_; On 2016/10/21 13:36:00, msramek wrote: > On 2016/10/20 19:03:10, mmenke wrote: > > This behavior should probably be documented in the header - pausing the > request > > long enough for relevant data to be cleared. > > It is mentioned in the class-level comment. Or do you mean to add the comment to > WillRedirectRequest() and WillProcessResponse() as well? Sorry, I missed that. https://codereview.chromium.org/2368923003/diff/100001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:257: if (origin.unique()) { On 2016/10/21 13:36:00, msramek wrote: > On 2016/10/20 19:03:10, mmenke wrote: > > Should this go up with the IsOriginSecure check? Seems to make sense to have > > all url validation in one place. > > Done. Yeah, makes sense. > > Now that I'm looking at it, I'm not sure if a unique origin can be secure. But > IsOriginSecure takes a GURL instead of an origin so it's not easy to verify. I'd > keep the check to be sure. File origins are secure and unique (Though they don't have any data to clear...Or headers, for that matter, though I guess blobs from file origins do have headers) https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:195: !headers->GetNormalizedHeader(kClearSiteDataHeader, &header_value)) { Should the kClearSiteDataHeader be added to kCookieResponseHeaders in http_response_headers? When we write an entry to the cache, we don't save set-cookie headers, so we don't set cookies when reading cached responses. I assume the same behavior should be done here? And should probably have a test for it, which I assume will be trivial, assuming we already test that code for other headers. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:200: if (!IsOriginSecure(current_url_)) { There's a lot of logic here which it seems like we should be testing. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:202: CONSOLE_MESSAGE_LEVEL_ERROR); Do we care about console messages enough to test this? https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:259: DCHECK_CURRENTLY_ON(BrowserThread::IO); optional: May want to put this at the start of the method...Not sure, may be best here to document where we are before the PostTask, up to you. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:278: ConsoleLog(messages, current_url_, "Must only contain ASCII characters.", I think it's cleaner to make more sense to make this method static, and make current_url_ an argument, just from a testing standpoint. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:326: base::StringPrintf("Invalid type: %s.", serialized_type.c_str()), Not really related to this CL, but while I'm reading through this, would "Unknown" or "Unrecognized" be better? New stuff is added to APIs over time. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:8: #include <memory> Not used. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:14: #include "base/values.h" Nont needed. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:15: #include "content/public/browser/resource_request_info.h" Not needed. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:27: } Not needed. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:61: FRIEND_TEST_ALL_PREFIXES(ClearSiteDataThrottleTest, InvalidHeader); Do we really need to friend the test fixture? Think just making a public ParseHeaderForTesting method is cleaner, and makes tests stick to using the public API. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:75: bool ParseHeader(const std::string& header, include <string> https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:89: GURL current_url_; Don't think we need this variable - can just grab it from the URLRequest as needed. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:93: base::TimeTicks clearing_started_; While you're here, should include base/time.h https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:62: TEST_F(ClearSiteDataThrottleTest, ParseHeader) { I suggest making all these tests more integration-y. De-friend the individual tests, create a mock ResourceController that checks if a particular cookie / whatever has been / has not been cleared on Resume, and then call the throttle's WillStartRequest / WillRedirectRequest methods, as in production code, check if we're deferred or not (as expected), and if deferred, wait for the resume call. That more fully checks all expected behaviors. If the BrowsingDataRemover can't easily be used in unit tests, then just make a virtual method that posts the task to call ClearSiteDataOnUIThread, then subclass it, and not go through ClearSiteDataThrottle::CreateThrottleForRequest for most of these tests. The browser tests do a reasonable job, but think we could have better coverage here, WDYT? Not sure about mocking out stuff like console messages, either. Could be it's just too much work. I'm not going to push too much on this, given the good coverage we're getting from your browser tests. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:62: TEST_F(ClearSiteDataThrottleTest, ParseHeader) { We don't have any tests of the deferring behavior, which I think we should have. We could have a browser test that makes sure we clear cookies before following a redirect, but that could flakily pass, while a well-written unit test would not. If we go with a browser test, I'd suggest a redirect test (Something we don't have but need somewhere). Set a cookie manually, request a resource which has the clear header and redirects us to another site, which sets another cookie. Then make sure the first cookie was cleared, wasn't set with the request, and the second one is set at the end of the test. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:138: "Must only contain ASCII characters.\n"}}; How hard would it be to write integration tests that make sure these actually get to the console, or unit tests that make sure these are sent to the method that makes sure these actually get to the console?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Matt! It took me a while again to update this. I've been mostly playing with various approaches to unit testing, and didn't end up with as elegant tests as I hoped. I left some questions in the *_unittest.cc file in case you have some advice. Please have another look! Feel free to take your time, as I'll be OOO the rest of the week (and I see that you're busy anyway). Cheers, Martin https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:195: !headers->GetNormalizedHeader(kClearSiteDataHeader, &header_value)) { On 2016/10/21 15:16:20, mmenke (OOO 10-28) wrote: > Should the kClearSiteDataHeader be added to kCookieResponseHeaders in > http_response_headers? When we write an entry to the cache, we don't save > set-cookie headers, so we don't set cookies when reading cached responses. I > assume the same behavior should be done here? And should probably have a test > for it, which I assume will be trivial, assuming we already test that code for > other headers. Yes, that's a good point. This header should be network-only. Done. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:200: if (!IsOriginSecure(current_url_)) { On 2016/10/21 15:16:20, mmenke wrote: > There's a lot of logic here which it seems like we should be testing. Added a few tests: ClearSiteDataThrottleTest.InvalidOrigin - handling insecure origins, unique origins, and their error messages ClearSiteDataThrottleTest.LoadDoNotSaveCookies - handling LOAD_DO_NOT_SAVE_COOKIES ClearSiteDataThrottleBrowserTest.CredentialsOnRedirect - essentially handling LOAD_DO_NOT_SAVE_COOKIES when it changes during the request lifetime (although the browser doesn't directly see that) The service worker parts are already covered in browsertests. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:202: CONSOLE_MESSAGE_LEVEL_ERROR); On 2016/10/21 15:16:20, mmenke (Busy Nov 2-3) wrote: > Do we care about console messages enough to test this? We don't care that much, but I tried to test them. This, however, led to a complication. The console messages are output from the destructor, and that cannot be overriden in tests (even if we made it a virtual function and overrode it in tests, the destructor will not call the overriden version as the subclass is already destroyed at that time). So I had to create a delegate class that handles console messages. That unfortunately added some boilerplate code. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:259: DCHECK_CURRENTLY_ON(BrowserThread::IO); On 2016/10/21 15:16:20, mmenke wrote: > optional: May want to put this at the start of the method...Not sure, may be > best here to document where we are before the PostTask, up to you. I extracted everything that knows about threads to separate methods so that I don't have to deal with it in unittests. Those separate methods now contain the DCHECK_CURRENTLY_ON. The rest of the code can run on both UI and IO threads. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:278: ConsoleLog(messages, current_url_, "Must only contain ASCII characters.", On 2016/10/21 15:16:20, mmenke wrote: > I think it's cleaner to make more sense to make this method static, and make > current_url_ an argument, just from a testing standpoint. Done. I actually had that in mind when writing this code originally, don't remember what prevented it then. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:326: base::StringPrintf("Invalid type: %s.", serialized_type.c_str()), On 2016/10/21 15:16:20, mmenke wrote: > Not really related to this CL, but while I'm reading through this, would > "Unknown" or "Unrecognized" be better? New stuff is added to APIs over time. Done. Here and below. Makes sense. This is in fact a non-fatal error, exactly for the purpose of forward compatibility. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:8: #include <memory> On 2016/10/21 15:16:21, mmenke wrote: > Not used. This is here because of std::unique_ptr<>. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:14: #include "base/values.h" On 2016/10/21 15:16:21, mmenke wrote: > Nont needed. Done. Removed. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:15: #include "content/public/browser/resource_request_info.h" On 2016/10/21 15:16:21, mmenke wrote: > Not needed. True, but after last changes it's needed again because of ResourceRequestInfo::WebContentsGetter. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:27: } On 2016/10/21 15:16:20, mmenke wrote: > Not needed. True, but after last changes it's needed again now because of extracting the clearing logic to ExecuteClearingTask() :) https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:61: FRIEND_TEST_ALL_PREFIXES(ClearSiteDataThrottleTest, InvalidHeader); On 2016/10/21 15:16:21, mmenke wrote: > Do we really need to friend the test fixture? Think just making a public > ParseHeaderForTesting method is cleaner, and makes tests stick to using the > public API. I made ParseHeader "public static". I don't think it even needs to be called ForTesting - there isn't any reason to call this method as standalone outside tests, or any danger if someone did. Removed the unittest friendship. Removed the fuzzer friendship and simplified the fuzzer code. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:75: bool ParseHeader(const std::string& header, On 2016/10/21 15:16:20, mmenke wrote: > include <string> Done. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:89: GURL current_url_; On 2016/10/21 15:16:21, mmenke wrote: > Don't think we need this variable - can just grab it from the URLRequest as > needed. Done. Yes, this is a remnant of the NavigationThrottle - there, we needed it during redirects, as NavigationHandle already had the new URL while we still needed to process headers from the old URL. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:93: base::TimeTicks clearing_started_; On 2016/10/21 15:16:21, mmenke wrote: > While you're here, should include base/time.h Done. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:62: TEST_F(ClearSiteDataThrottleTest, ParseHeader) { On 2016/10/21 15:16:21, mmenke (Busy Nov 2-3) wrote: > I suggest making all these tests more integration-y. De-friend the individual > tests, create a mock ResourceController that checks if a particular cookie / > whatever has been / has not been cleared on Resume, and then call the throttle's > WillStartRequest / WillRedirectRequest methods, as in production code, check if > we're deferred or not (as expected), and if deferred, wait for the resume call. > That more fully checks all expected behaviors. > > If the BrowsingDataRemover can't easily be used in unit tests, then just make a > virtual method that posts the task to call ClearSiteDataOnUIThread, then > subclass it, and not go through ClearSiteDataThrottle::CreateThrottleForRequest > for most of these tests. > > The browser tests do a reasonable job, but think we could have better coverage > here, WDYT? Not sure about mocking out stuff like console messages, either. > Could be it's just too much work. > > I'm not going to push too much on this, given the good coverage we're getting > from your browser tests. I disassembled ClearSiteDataThrottle a bit to make it better mockable, added a few tests for error messages and ClearSiteDataThrottleTest.DeferAndResume to test deferring. Regarding the the set-cookie-and-delete-cookie test, neither the unittest or browsertest can reasonably do that, as the call goes through ContentBrowserClient out to the browser. We could implement a mock cookie storage / MockContentBrowserClient, but at that point we're just testing the mock implementation, which is as good as testing EXPECT_CALL. There is one problem where I got stuck, maybe you can help me there. I wanted to write the test about deferring in the following way: Create a request, create a throttle, then keep modifying the request (e.g. redirect it somewhere) and EXPECT_CALLs on the throttle. The problem is that I don't know how to reasonably modify the request - all interesting methods are non-public. URLRequestJob is a friend that has access to them, but subclasses of URLRequestJob are not friends. We could for example run the request against an EmbeddedTestServer, but I already have this kind of stuff in the browsertest, and don't want to copy over the whole infrastructure. So instead, I just always create a new request and then just manually run WillRedirectRequest() or WillProcessResponse() and see what happens. This mostly achieves what I want, but it's not elegant and the test could still pass if we replaced URLRequest::url() with URLRequest::original_url(). Fortunately, we test that in many other places. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:138: "Must only contain ASCII characters.\n"}}; On 2016/10/21 15:16:21, mmenke (Busy Nov 2-3) wrote: > How hard would it be to write integration tests that make sure these actually > get to the console, or unit tests that make sure these are sent to the method > that makes sure these actually get to the console? The call goes to RenderFrameHostImpl. So far, I can see that a) RenderFrameHost is abstract, but there isn't any testing subclass anywhere which is suspicious b) RenderFrameHostImpl is not mockable c) RenderFrameImpl is in the renderer I'm not sure what's the best approach. Given that - they're not super important - everything around console messages is tested except that they actually make it to the console, and that part is easy to verify visually - this CL has already grown quite a bit ...I would prefer to leave this for another time :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Sorry, forgot about this CL. Want to get the "where are we going to support this new header" issue resolved before looking at the CL again. https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/120001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:62: TEST_F(ClearSiteDataThrottleTest, ParseHeader) { On 2016/10/31 19:23:36, msramek wrote: > On 2016/10/21 15:16:21, mmenke (Busy Nov 2-3) wrote: > > I suggest making all these tests more integration-y. De-friend the individual > > tests, create a mock ResourceController that checks if a particular cookie / > > whatever has been / has not been cleared on Resume, and then call the > throttle's > > WillStartRequest / WillRedirectRequest methods, as in production code, check > if > > we're deferred or not (as expected), and if deferred, wait for the resume > call. > > That more fully checks all expected behaviors. > > > > If the BrowsingDataRemover can't easily be used in unit tests, then just make > a > > virtual method that posts the task to call ClearSiteDataOnUIThread, then > > subclass it, and not go through > ClearSiteDataThrottle::CreateThrottleForRequest > > for most of these tests. > > > > The browser tests do a reasonable job, but think we could have better coverage > > here, WDYT? Not sure about mocking out stuff like console messages, either. > > Could be it's just too much work. > > > > I'm not going to push too much on this, given the good coverage we're getting > > from your browser tests. > > I disassembled ClearSiteDataThrottle a bit to make it better mockable, added a > few tests for error messages and ClearSiteDataThrottleTest.DeferAndResume to > test deferring. > > Regarding the the set-cookie-and-delete-cookie test, neither the unittest or > browsertest can reasonably do that, as the call goes through > ContentBrowserClient out to the browser. We could implement a mock cookie > storage / MockContentBrowserClient, but at that point we're just testing the > mock implementation, which is as good as testing EXPECT_CALL. Hrm...That seems a little weird. From a layering standpoint, this is part of a web API, so doesn't seem like it should be dependent on Chrome to implement it, just from a correctness standpoint. Or, alternatively, content-shell should. Beyond that, blimp, headless, views, cast, and Android WebView all fail to implement the requisite method. Do you have any plans here? Is the plan to support this feature on none of these? > There is one problem where I got stuck, maybe you can help me there. I wanted to > write the test about deferring in the following way: > > Create a request, create a throttle, then keep modifying the request (e.g. > redirect it somewhere) and EXPECT_CALLs on the throttle. The problem is that I > don't know how to reasonably modify the request - all interesting methods are > non-public. URLRequestJob is a friend that has access to them, but subclasses of > URLRequestJob are not friends. > > We could for example run the request against an EmbeddedTestServer, but I > already have this kind of stuff in the browsertest, and don't want to copy over > the whole infrastructure. > > So instead, I just always create a new request and then just manually run > WillRedirectRequest() or WillProcessResponse() and see what happens. This mostly > achieves what I want, but it's not elegant and the test could still pass if we > replaced URLRequest::url() with URLRequest::original_url(). Fortunately, we test > that in many other places. Why do you want to modify the request? You can register a request handler that repeatedly returns redirects and varying headers.
On 2016/11/08 18:23:59, mmenke wrote: > Sorry, forgot about this CL. Want to get the "where are we going to support > this new header" issue resolved before looking at the CL again. I mean before doing another pass on the code.
On 2016/11/08 18:24:36, mmenke wrote: > On 2016/11/08 18:23:59, mmenke wrote: > > Sorry, forgot about this CL. Want to get the "where are we going to support > > this new header" issue resolved before looking at the CL again. > > I mean before doing another pass on the code. Instead of pondering where we need and don't need to suport this, let me explain why the architecture looks this way and how I propose to improve it to make it work everywhere. Each of the three buckets (cookies, storage, and cache) is pretty broad. For example, "cookies" entail not only the actual cookies (which live in StoragePartition in content/), but also password manager auto-signin (which lives in chrome/). So an implementation that lives completely in content/ would fail to delete all the data that we understand under "cookies". Different embedders might have other features that belong to this bucket, and they should make sure to delete those. In that sense, this feature is inherently platform-dependent, which is not a problem as long as all platforms understand "cookies" as "data storage backends used to identify the user / keep the signin state". This means that layering cannot be done differently. But I agree that this is a layering violation to the extent that the calls to StoragePartition do not have to go through chrome/, and if we implemented that part in content/, it would be automatically supported for all embedders. Solution 1: - Make calls to StoragePartition for everything that is in content/ - ContentBrowserClient deletes everything that isn't in content/ Problem: Requires us to explicitly enumerate all backends belonging to "cookies", "storage", "cache" instead of taking advantage of BrowsingDataRemover (in chrome/) that already understands these buckets. Solution 2: - Make calls to StoragePartition for everything that is in content/ - ContentBrowserClient calls BrowsingDataRemover Problem: BrowsingDataRemover also makes calls to StoragePartition, so there would be duplicate calls. Solution 3: - Implement ContentBrowsingDataRemover for everything that is in content/ - ContentBrowserClient calls BrowsingDataRemover - BrowsingDataRemover delegates content/ deletions to ContentBrowserClient when called from UI, skips content/ deletions when called from Clear-Site-Data Problem: The per-origin and per-domain deletion filters are in chrome/browser/browsing_data, but we can put them somewhere else. I brought up Solution 3 during the previous code review of the NavigationThrottle already. nasko@ didn't support it then (or more like didn't think it was necessary), but I'm happy to reopen the discussion in a design doc. Note also that if we proceed this way, it's orthogonal to this CL - the only change would be two PostTask-s in ExecuteClearingTask() instead of one.
On 2016/11/09 10:31:30, msramek wrote: > On 2016/11/08 18:24:36, mmenke wrote: > > On 2016/11/08 18:23:59, mmenke wrote: > > > Sorry, forgot about this CL. Want to get the "where are we going to support > > > this new header" issue resolved before looking at the CL again. > > > > I mean before doing another pass on the code. > > > Instead of pondering where we need and don't need to suport this, let me explain > why the architecture looks this way and how I propose to improve it to make it > work everywhere. > > > Each of the three buckets (cookies, storage, and cache) is pretty broad. For > example, "cookies" entail not only the actual cookies (which live in > StoragePartition in content/), but also password manager auto-signin (which > lives in chrome/). > > So an implementation that lives completely in content/ would fail to delete all > the data that we understand under "cookies". Different embedders might have > other features that belong to this bucket, and they should make sure to delete > those. In that sense, this feature is inherently platform-dependent, which is > not a problem as long as all platforms understand "cookies" as "data storage > backends used to identify the user / keep the signin state". > > This means that layering cannot be done differently. But I agree that this is a > layering violation to the extent that the calls to StoragePartition do not have > to go through chrome/, and if we implemented that part in content/, it would be > automatically supported for all embedders. > > > Solution 1: > - Make calls to StoragePartition for everything that is in content/ > - ContentBrowserClient deletes everything that isn't in content/ > Problem: Requires us to explicitly enumerate all backends belonging to > "cookies", "storage", "cache" instead of taking advantage of BrowsingDataRemover > (in chrome/) that already understands these buckets. > > > Solution 2: > - Make calls to StoragePartition for everything that is in content/ > - ContentBrowserClient calls BrowsingDataRemover > Problem: BrowsingDataRemover also makes calls to StoragePartition, so there > would be duplicate calls. > > > Solution 3: > - Implement ContentBrowsingDataRemover for everything that is in content/ > - ContentBrowserClient calls BrowsingDataRemover > - BrowsingDataRemover delegates content/ deletions to ContentBrowserClient when > called from UI, skips content/ deletions when called from Clear-Site-Data > Problem: The per-origin and per-domain deletion filters are in > chrome/browser/browsing_data, but we can put them somewhere else. > > > I brought up Solution 3 during the previous code review of the > NavigationThrottle already. nasko@ didn't support it then (or more like didn't > think it was necessary), but I'm happy to reopen the discussion in a design doc. > Note also that if we proceed this way, it's orthogonal to this CL - the only > change would be two PostTask-s in ExecuteClearingTask() instead of one. I prefer solution 3. I'd change it up a little to make ContentBrowserClient return a BrowsingDataRemover, given a storage partition, and make ChromeBrowsingDataRemover wrap the default ContentBrowsingDataRemover (Which would also be returned by the default ContentBrowserClient implementation). That's similar to solution 3, it's just that ContentBrowserClient would return an object by default, so if an embedder really, really wants to do nothing on clear, it would have to implement a stub class. Also allows for more shared methods, if we need them. I'm reluctant to go ahead and just make this a Chrome feature until someone gets around to fixing this - it's been my experience that issues often aren't followed up on. I've had nothing but great experiences working with you, but it's not uncommon for significant TODOs to be left alone if favor of something higher priority on people's TODO lists. Doing this sooner rather than later would also allow for more integrationy tests. Of course, that having been said, I guess I'm not really an owned of any of this stuff, other than net/ and loader/.
On 2016/11/09 15:24:37, mmenke wrote: > On 2016/11/09 10:31:30, msramek wrote: > > On 2016/11/08 18:24:36, mmenke wrote: > > > On 2016/11/08 18:23:59, mmenke wrote: > > > > Sorry, forgot about this CL. Want to get the "where are we going to > support > > > > this new header" issue resolved before looking at the CL again. > > > > > > I mean before doing another pass on the code. > > > > > > Instead of pondering where we need and don't need to suport this, let me > explain > > why the architecture looks this way and how I propose to improve it to make it > > work everywhere. > > > > > > Each of the three buckets (cookies, storage, and cache) is pretty broad. For > > example, "cookies" entail not only the actual cookies (which live in > > StoragePartition in content/), but also password manager auto-signin (which > > lives in chrome/). > > > > So an implementation that lives completely in content/ would fail to delete > all > > the data that we understand under "cookies". Different embedders might have > > other features that belong to this bucket, and they should make sure to delete > > those. In that sense, this feature is inherently platform-dependent, which is > > not a problem as long as all platforms understand "cookies" as "data storage > > backends used to identify the user / keep the signin state". > > > > This means that layering cannot be done differently. But I agree that this is > a > > layering violation to the extent that the calls to StoragePartition do not > have > > to go through chrome/, and if we implemented that part in content/, it would > be > > automatically supported for all embedders. > > > > > > Solution 1: > > - Make calls to StoragePartition for everything that is in content/ > > - ContentBrowserClient deletes everything that isn't in content/ > > Problem: Requires us to explicitly enumerate all backends belonging to > > "cookies", "storage", "cache" instead of taking advantage of > BrowsingDataRemover > > (in chrome/) that already understands these buckets. > > > > > > Solution 2: > > - Make calls to StoragePartition for everything that is in content/ > > - ContentBrowserClient calls BrowsingDataRemover > > Problem: BrowsingDataRemover also makes calls to StoragePartition, so there > > would be duplicate calls. > > > > > > Solution 3: > > - Implement ContentBrowsingDataRemover for everything that is in content/ > > - ContentBrowserClient calls BrowsingDataRemover > > - BrowsingDataRemover delegates content/ deletions to ContentBrowserClient > when > > called from UI, skips content/ deletions when called from Clear-Site-Data > > Problem: The per-origin and per-domain deletion filters are in > > chrome/browser/browsing_data, but we can put them somewhere else. > > > > > > I brought up Solution 3 during the previous code review of the > > NavigationThrottle already. nasko@ didn't support it then (or more like didn't > > think it was necessary), but I'm happy to reopen the discussion in a design > doc. > > Note also that if we proceed this way, it's orthogonal to this CL - the only > > change would be two PostTask-s in ExecuteClearingTask() instead of one. > > I prefer solution 3. I'd change it up a little to make ContentBrowserClient > return a BrowsingDataRemover, given a storage partition, and make > ChromeBrowsingDataRemover wrap the default ContentBrowsingDataRemover (Which > would also be returned by the default ContentBrowserClient implementation). > That's similar to solution 3, it's just that ContentBrowserClient would return > an object by default, so if an embedder really, really wants to do nothing on > clear, it would have to implement a stub class. Also allows for more shared > methods, if we need them. > > I'm reluctant to go ahead and just make this a Chrome feature until someone gets > around to fixing this - it's been my experience that issues often aren't > followed up on. I've had nothing but great experiences working with you, but > it's not uncommon for significant TODOs to be left alone if favor of something > higher priority on people's TODO lists. Doing this sooner rather than later > would also allow for more integrationy tests. > > Of course, that having been said, I guess I'm not really an owned of any of this > stuff, other than net/ and loader/. Fair enough :) Let's do it! I like that this will help ensure interoperability not just with the embedders you mentioned, but also with foreign ones such as Opera, which is pretty important for a web platform feature. But we'll have to sort of saw c/b/browsing_data/ in half, which could be messy. Let me write a design doc and share it with a wider audience first. In the meantime, let's put this review on hold...
On 2016/11/09 18:43:36, msramek wrote: > Fair enough :) Let's do it! > > I like that this will help ensure interoperability not just with the embedders > you mentioned, but also with foreign ones such as Opera, which is pretty > important for a web platform feature. > > But we'll have to sort of saw c/b/browsing_data/ in half, which could be messy. > Let me write a design doc and share it with a wider audience first. In the > meantime, let's put this review on hold... Thanks for being so willing to do the right thing!
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:240001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hello folks! I'm reviving this months old CL. - Patchset #7: Rebase - Patcshet #8: Fixes after rebase (mainly ResourceThrottleController->ResourceThrottleDelegate) - Patchset #9: New improvements. mmenke@: You had asked me to write more integration-y tests. While this CL was open, I moved the browsing_data infrastructure into content/, which finally made that possible. Instead of ClearSiteDataThrottle->ChromeContentBrowserClient->BrowsingDataRemover, we now call directly ClearSiteDataThrottle->BrowsingDataRemover, as both now live in content/. The tests which we had in ChromeContentBrowserClient to verify that the three booleans (cookies, storage, cache) are correctly translated to BrowsingDataRemover masks were now folded into ClearSiteDataThrottleBrowserTest, which now mocks BrowsingDataRemoverDelegate to test the masks directly. The MockBrowsingDataDelegate class has been moved to content/public/test to facilitate that. All the tests for the behavior on secure contexts, redirects etc. were kept as is. However, I added three integration tests for the deletion of cookies, service workers, and cache. But really, it's probably easier to start reviewing from scratch. falken@: You previously reviewed the logic in ClearSiteDataThrottle which verifies that the throttle is not respected on service worker responses, because service workers could impersonate any origin and clear its data; and the tests of that logic. Nothing changed in that aspect, but feel free to have another look. mkwst@: Please review everything again :) Thanks! Martin
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
service worker integration still lgtm (didn't scrutinize too closely since you said not much changed from the last review) I couldn't find this test mentioned in the CL description: "A browsertest for a service worker request, including its .html and .js files, is currently marked DISABLED (WIP)." I assume this test will be for requesting a service worker script (during register or update), in which case the resources would be a main JS file and JS files included as importScripts(). Web exposed features like this are increasingly encouraged to be tested as web-platform-tests. Is that on your roadmap? https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:330: // any website. Maybe worth linking to https://w3c.github.io/webappsec-clear-site-data/#service-workers https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:337: if (extra_response_info.was_fetched_via_service_worker) { Does the spec care if the response came from a foreign fetch service worker? If so, you must check extra_response_info.was_fetched_via_foreign_fetch here too. A foreign fetch service worker can only intercept requests to its origin, so the third-party considerations don't apply. https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:37: // at https://www.w3.org/TR/clear-site-data/. Typically Chrome web platform implementation work uses the editor's draft, not TR links (which tend to be out of date). Does it make sense to link to this instead: https://w3c.github.io/webappsec-clear-site-data/ https://codereview.chromium.org/2368923003/diff/280001/content/test/data/brow... File content/test/data/browsing_data/worker.js (right): https://codereview.chromium.org/2368923003/diff/280001/content/test/data/brow... content/test/data/browsing_data/worker.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017 :)
Still digging through, but: https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:541: IN_PROC_BROWSER_TEST_F(ClearSiteDataThrottleBrowserTest, ServiceWorker) { This is failing on most platforms. Might be worth looking into. :)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/17 07:37:01, falken wrote: > service worker integration still lgtm (didn't scrutinize too closely since you > said not much changed from the last review) > > I couldn't find this test mentioned in the CL description: > "A browsertest for a service worker request, including its .html and .js files, > is currently marked DISABLED (WIP)." > > I assume this test will be for requesting a service worker script (during > register or update), in which case the resources would be a main JS file and JS > files included as importScripts(). > > Web exposed features like this are increasingly encouraged to be tested as > web-platform-tests. Is that on your roadmap? > The test in question is no longer WIP - let me update the CL description :) It's ClearSiteDataThrottleBrowserTest.ServiceWorker. It navigates to a page that installs a service worker. Then navigates again, this time the service worker serves a response which contains several subresources. The subresources are again requested through the service worker. It responds to some of them, and lets some through. The ones that were let through will then be handled by an EmbeddedTestServer. All responses contain Clear-Site-Data, and the test verifies that only the ones from the EmbeddedTestServer were actually executed. Coincidentally, that test seems to fail now on some trybots (but works on my machine - still investigating). And I agree that this would live better among WPTs - I have already added some in the meantime. I will convert some of these browsertests later, but for now, I wanted to pick up the CL without huge changes.
https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:330: // any website. On 2017/05/17 07:37:00, falken wrote: > Maybe worth linking to > https://w3c.github.io/webappsec-clear-site-data/#service-workers Done. https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:337: if (extra_response_info.was_fetched_via_service_worker) { On 2017/05/17 07:37:00, falken wrote: > Does the spec care if the response came from a foreign fetch service worker? If > so, you must check extra_response_info.was_fetched_via_foreign_fetch here too. > > A foreign fetch service worker can only intercept requests to its origin, so the > third-party considerations don't apply. Yes, that was my understanding too - thanks for confirming that this hasn't changed since your last review. In that case we trust a foreign fetch SW response from example.org the same way as we trust a server response from example.org, and don't need to disable Clear-Site-Data. https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:37: // at https://www.w3.org/TR/clear-site-data/. On 2017/05/17 07:37:00, falken wrote: > Typically Chrome web platform implementation work uses the editor's draft, not > TR links (which tend to be out of date). Does it make sense to link to this > instead: https://w3c.github.io/webappsec-clear-site-data/ Good point. Updated. https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/280001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:541: IN_PROC_BROWSER_TEST_F(ClearSiteDataThrottleBrowserTest, ServiceWorker) { On 2017/05/17 08:38:32, Mike West wrote: > This is failing on most platforms. Might be worth looking into. :) It's consistently green on my local machine :( Looks like a problem with synchronization. I added some debug outputs, please ignore them for now. https://codereview.chromium.org/2368923003/diff/280001/content/test/data/brow... File content/test/data/browsing_data/worker.js (right): https://codereview.chromium.org/2368923003/diff/280001/content/test/data/brow... content/test/data/browsing_data/worker.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/17 07:37:00, falken wrote: > 2017 :) Done. Yeah, this normally doesn't happen in May :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I'll plan to get to this tomorrow. Worth noting that the plan is that ResourceThrottles will go away at some point, but don't think we need to worry about that in this CL.
Description was changed from ========== Support the Clear-Site-Data header on resource requests Until now, it was only supported for navigations. Changes in this CL: 1. Convert the NavigationThrottle to a ResourceThrottle. The two classes are sufficiently similar that these are mostly syntactical changes, except that the ResourceThrottle lives on the IO thread and needs to occasionally jump to the UI thread. 2. Instantiate it in ResourceDispatcherHostImpl instead of NavigationHandleImpl. This requires adding an explicit DEPS rule. 3. Add restrictions to the *Create() methods - for example, we will not support service worker requests or LOAD_DO_NOT_SET_COOKIES. These are then tested in the unittest. This requires adding CONTENT_EXPORT to UrlRequestServiceWorkerData. 4. Add browsertests for resource requests. A browsertest for a service worker request, including its .html and .js files, is currently marked DISABLED (WIP). BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Support the Clear-Site-Data header on resource requests Until now, it was only supported for navigations. Changes in this CL: 1. Convert the NavigationThrottle to a ResourceThrottle. The two classes are sufficiently similar that these are mostly syntactical changes, except that the ResourceThrottle lives on the IO thread and needs to occasionally jump to the UI thread. 2. Instantiate it in ResourceDispatcherHostImpl instead of NavigationHandleImpl. This requires adding an explicit DEPS rule. 3. Add some restrictions - for example, we will not support service worker requests or LOAD_DO_NOT_SET_COOKIES. These are then tested in the unittest. 4. Add browsertests for resource requests, and some integration tests that check not only calls to BrowsingDataRemover, but also the actual removal of data. BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
On 2017/05/17 13:45:52, msramek wrote:
> It's ClearSiteDataThrottleBrowserTest.ServiceWorker. It navigates to a page
that
> installs a service worker.
Ah, what I meant was at this point. Do you have a test that the header is
processed here, i.e., when the page installs a service worker by calling
register('sw.js') and the response for sw.js itself has the Clear-Site-Data
header? Similarly, when the browser does an automated service worker update and
the response has the header.
I don't think we need to add the test to this CL, but it'd be good to have a
test eventually. It's the use case described at
https://w3c.github.io/webappsec-clear-site-data/#example-killswitch
Sorry for the unreasonable delay. I'll start reviewing tests today, too, but may not get back to you on them until tomorrow. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:74: int count) count seems too ambiguous here. expected_completion_calls, maybe? Open to other names. Same for the member variable. Or, better, maybe make this take the three clear_ bools from ClearSiteDataOnUIThread, and move all of its logic here? Then you don't have the somewhat silly OnBrowsingDataRemoverDone() calls in that method, instead, you just increment count as needed...Or can browsing data remover invoke callbacks synchronously? If so, may need to code around that, but still think it's cleaner to have the logic in here. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:195: if (message.url == last_seen_url) { Any reason to do these all at the end, instead of on a per-request basis? i.e., just dump messages after each HandleHeader call, and use the current URL (If there are any messages to pass). That also makes the "Clearing" in strings not a lie - currently, unless the request is cancelled while clearing, when we print out "clearing foo", we've already long since cleared "foo". https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:236: messages_.clear(); Is this needed, or is std::move guaranteed to clear the source vector? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:302: if (!IsOriginSecure(request_->url())) { Hrm....bizarrely, we don't have a version of IsOriginSecure that takes or origin, though it does compute the GURL version of origin (As opposed to the Origin version of an origin) internally. No action needed, this just makes me sad. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:324: CONSOLE_MESSAGE_LEVEL_ERROR); Hrm...We don't log this for normal requests with Set-cookie headers and LOAD_DO_NOT_SAVE_COOKIES, I assume? Should we log it in this much less common case, then? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:353: if (!ParseHeader(header_value, &clear_cookies, &clear_storage, &clear_cache, Know this is old code, but can we use a less generic name for it? It's not clear from context this is a ClearSideDataThrottle method, and it sounds like a general method name. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:372: weak_ptr.get(); I don't think this is needed - GetWeakPtr already binds the weak ptr yo this thread, no? Per spec, the weak ptr can only be dereferenced on the thread GetWeakPtr is called on: // Note that GetWeakPtr must be called on one and only one TaskQueue or thread // and the WeakPtr must only be dereferenced and invalidated on that same // TaskQueue/thread. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:376: base::Bind(&ClearSiteDataThrottle::TaskFinished, weak_ptr)); base::BindOnce + use std::move everywhere to pass the callback is now preferred, I think? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:382: bool ClearSiteDataThrottle::ParseHeader(const std::string& header, This should be in the same order it's declared in in the header file. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:421: value.GetAsString(&type); if (!value.GetAsString(&type)) continue; (With maybe a log message)? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:429: } else if (type == "cache") { optional: Suggest making these 3 constants up top - they're only used once, but so are kClearingOneType, and think it's nice to make it clear what types are supported in a list of constants up top. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:443: DCHECK(datatype); This seems wrong - we shouldn't DCHECK if a site sends us bad data, and since this is just a list, a server can send us a list with duplicate values, no? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:470: type_names[1].c_str(), type_names[2].c_str()); Since this isn't internationalized (I assume nothing in dev tools is?), suggest just a loop - it's more general, and adding a case every time another option is added seems silly, and likely to be something someone forgets to do, resulting in a regression. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:499: void ClearSiteDataThrottle::TaskFinished() { DCHECK_CURRENTLY_ON(BrowserThread::IO); https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:499: void ClearSiteDataThrottle::TaskFinished() { DCHECK(!clearing_started_.is_null()); https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:505: LOG(ERROR) << "*** DEBUG OUTPUT FOR TRYBOTS *** Defer."; Remove this? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:90: const GURL& current_url); I'd suggest making this a public *ForTest / ForTesting wrapper around a private method, to make it clear this is only exposed for testing. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:105: const base::Closure& callback); These can both be private (private virtual methods can be overridden by subclasses, they just can't be invoked by them).
I'll look at browser tests tomorrow. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:70: static std::unique_ptr<ResourceThrottle> CreateThrottleForRequest( MaybeCreateThrottleForRequest? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:31: static const char* kClearCookiesHeader = nit: static not needed, "const char kClearCookiesHeader[]" is preferred, I believe (Since that means neither the pointer nor what it's pointing at can be modified) https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:46: : public ClearSiteDataThrottle::ConsoleMessagesDelegate { Is this class needed? The production one already handles a WebContentsGetter that returns nullptr (Since it's possible the WebContents was destroyed) https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:66: void SetResponseHeaders(std::string headers) { nit: const std::string& https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:101: base::MessageLoop message_loop_; I think there's now a more task-runner friendly way of doing this? base::test::ScopedTaskEnvironment? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:227: } optional: Hrm...There's no testing of the output of ClearSiteDataOnUIThread. Would it make sense to have the Delegate thing create the full sctrings, then pass them over to the UI thread, just for better testing? Not going to insist on it. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:304: EXPECT_EQ(console_delegate->messages().size(), 1u); Why is the size 1 on secure origins, and not 0? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:407: testing::Expectation e = EXPECT_CALL( This name violates the google style guide - should give some information about the name. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:409: ClearSiteData(url::Origin(GURL(test_origin.origin)), _, _, _)); I'm not going to block on this, but GMOCK syntax is very convoluted and unclear - the network stack team, at least, generally avoids any use of GMOCK for this reason, and instead implements our own mock classes. This also makes it easier for third parties to start hacking on Chrome, and more generally makes tests more debuggable. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:412: EXPECT_CALL(throttle, ClearSiteData(_, _, _, _)).Times(0); I don't think we ever check that ClearSiteData is called with the right parameters in any of these tests? Suggest doing it in this one, or one like it, and setting each one individually (ParseHeader makes sure we're parsing them correctly, but I don't think anything is making sure we're passing them correctly to the ClearSiteData call, other than the browser_tests).
I talked with msramek@ about this yesterday, and the broad strokes still LGTM. That said, I agree with falken@ that we need to start upstreaming some of these browser tests to WPT so that we can share them with other vendors. I'll help msramek@ make that happen in a subsequent patch; I don't believe it needs to block this one.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #12 (id:320001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for the thorough review so far :) I addressed your comments and rebased (due to a merge conflict). https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:74: int count) On 2017/05/22 18:35:35, mmenke wrote: > count seems too ambiguous here. expected_completion_calls, maybe? Open to > other names. Same for the member variable. > > Or, better, maybe make this take the three clear_ bools from > ClearSiteDataOnUIThread, and move all of its logic here? Then you don't have > the somewhat silly OnBrowsingDataRemoverDone() calls in that method, instead, > you just increment count as needed...Or can browsing data remover invoke > callbacks synchronously? If so, may need to code around that, but still think > it's cleaner to have the logic in here. Done - merged ClearSiteDataOnUIThread() and this Observer into one class. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:195: if (message.url == last_seen_url) { On 2017/05/22 18:35:35, mmenke wrote: > Any reason to do these all at the end, instead of on a per-request basis? i.e., > just dump messages after each HandleHeader call, and use the current URL (If > there are any messages to pass). > > That also makes the "Clearing" in strings not a lie - currently, unless the > request is cancelled while clearing, when we print out "clearing foo", we've > already long since cleared "foo". That's a good point - changed "Clearing..." To "Cleared...". The reason why we only output this at the end are navigations. When this was a NavigationThrottle, the most reasonable usage was a navigation redirect chain: Deletion request -> URL that returns the header -> landing page. ...in which case, the output would never be visible, as the DevTools would only show messages from the landing page. Since we support subresource requests with the CL, maybe we don't care about that case anymore, I'm not sure. Or we could handle it separately. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:236: messages_.clear(); On 2017/05/22 18:35:34, mmenke wrote: > Is this needed, or is std::move guaranteed to clear the source vector? I'm not sure that std::vector<> is safe to use after move. What I did find is that "clear()" is always safe to call, and always brings the vector to valid empty state. (I think that in practice, it's safe to omit this, but I'd like to play it safe.) https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:302: if (!IsOriginSecure(request_->url())) { On 2017/05/22 18:35:35, mmenke wrote: > Hrm....bizarrely, we don't have a version of IsOriginSecure that takes or > origin, though it does compute the GURL version of origin (As opposed to the > Origin version of an origin) internally. No action needed, this just makes me > sad. Acknowledged. I'd expect that we can just move it to url::Origin (Mike?). https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:324: CONSOLE_MESSAGE_LEVEL_ERROR); On 2017/05/22 18:35:34, mmenke wrote: > Hrm...We don't log this for normal requests with Set-cookie headers and > LOAD_DO_NOT_SAVE_COOKIES, I assume? Should we log it in this much less common > case, then? We wouldn't log anything to the console, but I think we would display a crossed cookie icon in the omnibox. We don't have that here, so I wanted to give at least some feedback to the developer. I added a TODO to find a proper treatment. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:353: if (!ParseHeader(header_value, &clear_cookies, &clear_storage, &clear_cache, On 2017/05/22 18:35:34, mmenke wrote: > Know this is old code, but can we use a less generic name for it? It's not > clear from context this is a ClearSideDataThrottle method, and it sounds like a > general method name. How about simply prefixing it with the class name here? It's a static method, so everyone who would use the method outside this class will have to do so anyway. And then ClearSiteDataThrottle::ParseClearSiteDataThrottle would look like an unnecessary duplication. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:372: weak_ptr.get(); On 2017/05/22 18:35:34, mmenke wrote: > I don't think this is needed - GetWeakPtr already binds the weak ptr yo this > thread, no? Per spec, the weak ptr can only be dereferenced on the thread > GetWeakPtr is called on: > > // Note that GetWeakPtr must be called on one and only one TaskQueue or thread > // and the WeakPtr must only be dereferenced and invalidated on that same > // TaskQueue/thread. But there is also this: // To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory // is dereferenced, the factory and its WeakPtrs become bound to the calling // thread or current SequencedWorkerPool token, and cannot be dereferenced or // invalidated on any other task runner. So it's bound to the thread when dereferenced for the first time. I have learned this recently when debugging a threading issue - the stack trace from DCHECK was confusing, because the weak pointer was first dereferenced on the wrong thread. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:376: base::Bind(&ClearSiteDataThrottle::TaskFinished, weak_ptr)); On 2017/05/22 18:35:34, mmenke wrote: > base::BindOnce + use std::move everywhere to pass the callback is now preferred, > I think? Done. Yes, I think so too. Changed here and in tests, except where other classes' interfaces required RepeatedCallback. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:382: bool ClearSiteDataThrottle::ParseHeader(const std::string& header, On 2017/05/22 18:35:34, mmenke wrote: > This should be in the same order it's declared in in the header file. Done. Reordered. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:421: value.GetAsString(&type); On 2017/05/22 18:35:35, mmenke wrote: > if (!value.GetAsString(&type)) > continue; > (With maybe a log message)? This is handled in the "} else {" branch below, together with valid but unknown strings. We do serialize |type| in the console message, so it should be clear enough where was the syntax error, regardless of what |type| is. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:429: } else if (type == "cache") { On 2017/05/22 18:35:35, mmenke wrote: > optional: Suggest making these 3 constants up top - they're only used once, but > so are kClearingOneType, and think it's nice to make it clear what types are > supported in a list of constants up top. Done. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:443: DCHECK(datatype); On 2017/05/22 18:35:34, mmenke wrote: > This seems wrong - we shouldn't DCHECK if a site sends us bad data, and since > this is just a list, a server can send us a list with duplicate values, no? I moved the comment one line below, where it's hopefully less confusing :) Earlier in this method, |datatype| is initialized to nullptr. This DCHECK just verifies that it has a value by now (bool |cookies|, bool |storage|, or bool |cache|). Then the "if" on the next line makes sure that we don't process something twice. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:470: type_names[1].c_str(), type_names[2].c_str()); On 2017/05/22 18:35:35, mmenke wrote: > Since this isn't internationalized (I assume nothing in dev tools is?), suggest > just a loop - it's more general, and adding a case every time another option is > added seems silly, and likely to be something someone forgets to do, resulting > in a regression. Done. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:499: void ClearSiteDataThrottle::TaskFinished() { On 2017/05/22 18:35:34, mmenke wrote: > DCHECK(!clearing_started_.is_null()); Done. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:499: void ClearSiteDataThrottle::TaskFinished() { On 2017/05/22 18:35:34, mmenke wrote: > DCHECK_CURRENTLY_ON(BrowserThread::IO); I didn't add this on purpose, because the unittest is single-threaded (only the actual deletion, which is mocked in the unittest, has these DCHECKs). I didn't have time to look into this further today, but if you think the DCHECK should be here, I can add real threads to the unittest in the next iteration. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:505: LOG(ERROR) << "*** DEBUG OUTPUT FOR TRYBOTS *** Defer."; On 2017/05/22 18:35:34, mmenke wrote: > Remove this? Done. (Here and elsewhere) I still haven't debugged the red test, but this didn't help. It seems that TitleWatcher doesn't work correctly on some platforms :/ https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:70: static std::unique_ptr<ResourceThrottle> CreateThrottleForRequest( On 2017/05/22 19:36:08, mmenke wrote: > MaybeCreateThrottleForRequest? Done. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:90: const GURL& current_url); On 2017/05/22 18:35:35, mmenke wrote: > I'd suggest making this a public *ForTest / ForTesting wrapper around a private > method, to make it clear this is only exposed for testing. Done. (Although I'd say that since the method is static, misusing this method can't bring the throttle to an invalid state or anything.) https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:105: const base::Closure& callback); On 2017/05/22 18:35:35, mmenke wrote: > These can both be private (private virtual methods can be overridden by > subclasses, they just can't be invoked by them). Done. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:31: static const char* kClearCookiesHeader = On 2017/05/22 19:36:09, mmenke wrote: > nit: static not needed, "const char kClearCookiesHeader[]" is preferred, I > believe (Since that means neither the pointer nor what it's pointing at can be > modified) Done. Changed here as well as in clear_site_data_throttle.cc. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:46: : public ClearSiteDataThrottle::ConsoleMessagesDelegate { On 2017/05/22 19:36:09, mmenke wrote: > Is this class needed? The production one already handles a WebContentsGetter > that returns nullptr (Since it's possible the WebContents was destroyed) Removed. Thanks for noticing. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:66: void SetResponseHeaders(std::string headers) { On 2017/05/22 19:36:09, mmenke wrote: > nit: const std::string& Done. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:101: base::MessageLoop message_loop_; On 2017/05/22 19:36:09, mmenke wrote: > I think there's now a more task-runner friendly way of doing this? > base::test::ScopedTaskEnvironment? Done. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:227: } On 2017/05/22 19:36:09, mmenke wrote: > optional: Hrm...There's no testing of the output of ClearSiteDataOnUIThread. > Would it make sense to have the Delegate thing create the full sctrings, then > pass them over to the UI thread, just for better testing? Not going to insist > on it. Do you mean OutputMessagesOnUIThread()? I must admit I don't fully remember why (as I wrote this test long ago), but I found unittesting the actual console output to be nontrivial. I can try again, but I'd like to not add much more code in this CL. A mitigation is that a bug where the strings don't make it to the console is visually discoverable. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:304: EXPECT_EQ(console_delegate->messages().size(), 1u); On 2017/05/22 19:36:09, mmenke wrote: > Why is the size 1 on secure origins, and not 0? The throttle also outputs a message on successful deletion. This is encouraged by the spec: https://www.w3.org/TR/clear-site-data/#clear-response I added that expectation to this test explicitly. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:407: testing::Expectation e = EXPECT_CALL( On 2017/05/22 19:36:09, mmenke wrote: > This name violates the google style guide - should give some information about > the name. Fixed. Sorry for that. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:409: ClearSiteData(url::Origin(GURL(test_origin.origin)), _, _, _)); On 2017/05/22 19:36:09, mmenke wrote: > I'm not going to block on this, but GMOCK syntax is very convoluted and unclear > - the network stack team, at least, generally avoids any use of GMOCK for this > reason, and instead implements our own mock classes. This also makes it easier > for third parties to start hacking on Chrome, and more generally makes tests > more debuggable. In that case I'd prefer to keep it as is - as usual, in order not to expand this already large CL. I agree with you that GMOCK errors are hard to debug, but custom mock classes typically cost a lot of boilerplate code if you want them to support things like testing::_, Times(0), verbose outputs etc., so I'm actually surprised to hear that. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:412: EXPECT_CALL(throttle, ClearSiteData(_, _, _, _)).Times(0); On 2017/05/22 19:36:09, mmenke wrote: > I don't think we ever check that ClearSiteData is called with the right > parameters in any of these tests? Suggest doing it in this one, or one like it, > and setting each one individually (ParseHeader makes sure we're parsing them > correctly, but I don't think anything is making sure we're passing them > correctly to the ClearSiteData call, other than the browser_tests). I expanded the ParseHeader test, since that one tests different datatypes, so it seemed to fit there the best. Although I don't think it's super valuable, since even if we verify that we get that far, we still don't see if BrowsingDataRemover is called correctly. That's exactly why we now have these new integration-y browsertests, which you asked me to write a few months ago :) https://codereview.chromium.org/2368923003/diff/340001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/340001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:309: {"data:unique-origin;", false, "Not supported for unique origins."}, The improvement of the test below actually revealed that this test case was wrong. file:// origins are secure and non-unique, so Clear-Site-Data should theoretically work on them. In practice it doesn't, because they don't have response headers, but this test adds them explicitly.
Quick responses to your comment. I will actually review the browser tests today. Sorry for slowness - built up a bit of a backlog last week. And while I did indeed request browser tests, I don't exactly enjoy actually reviewing them. :) https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:195: if (message.url == last_seen_url) { On 2017/05/24 22:59:51, msramek wrote: > On 2017/05/22 18:35:35, mmenke wrote: > > Any reason to do these all at the end, instead of on a per-request basis? > i.e., > > just dump messages after each HandleHeader call, and use the current URL (If > > there are any messages to pass). > > > > That also makes the "Clearing" in strings not a lie - currently, unless the > > request is cancelled while clearing, when we print out "clearing foo", we've > > already long since cleared "foo". > > That's a good point - changed "Clearing..." To "Cleared...". > > The reason why we only output this at the end are navigations. When this was a > NavigationThrottle, the most reasonable usage was a navigation redirect chain: > > Deletion request -> URL that returns the header -> landing page. > > ...in which case, the output would never be visible, as the DevTools would only > show messages from the landing page. > > Since we support subresource requests with the CL, maybe we don't care about > that case anymore, I'm not sure. Or we could handle it separately. Hrm....But for subresources, clearing will affect future resource requests, so delaying it seems incorrect. I'll defer to you here, but not a fan of the current logic. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:236: messages_.clear(); On 2017/05/24 22:59:51, msramek wrote: > On 2017/05/22 18:35:34, mmenke wrote: > > Is this needed, or is std::move guaranteed to clear the source vector? > > I'm not sure that std::vector<> is safe to use after move. What I did find is > that "clear()" is always safe to call, and always brings the vector to valid > empty state. > > (I think that in practice, it's safe to omit this, but I'd like to play it > safe.) Googling for an answer, looks like it may not be guaranteed, so this makes sense. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:60: TitleWatcher title_watcher(shell->web_contents(), expected_title_16); I don't think we want to depend on the implementation of navigation - i.e., creating the watcher after we start navigation is relying on squatting on the UI thread blocking the title from being updated. This is probably going to always be safe to do, but I don't think we want to make that a dependency of this test. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:79: const int origin_type_mask = kOriginTypeMatch https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:227: } On 2017/05/24 22:59:52, msramek wrote: > On 2017/05/22 19:36:09, mmenke wrote: > > optional: Hrm...There's no testing of the output of ClearSiteDataOnUIThread. > > Would it make sense to have the Delegate thing create the full sctrings, then > > pass them over to the UI thread, just for better testing? Not going to insist > > on it. > > Do you mean OutputMessagesOnUIThread()? I must admit I don't fully remember why > (as I wrote this test long ago), but I found unittesting the actual console > output to be nontrivial. I can try again, but I'd like to not add much more code > in this CL. Sorry, I did indeed mean OutputMessagesOnUIThread. But what I mean we should test is not whether it outputs to console, but the whole last_seen_url thing. I don't think a single test relies on that behavior working. > A mitigation is that a bug where the strings don't make it to the console is > visually discoverable. It's really not. Suppose the last_seen_url logic was broken in the case of multiple different URLs resetting cookies. What do you think are the odds someone would actually notice that? https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:409: ClearSiteData(url::Origin(GURL(test_origin.origin)), _, _, _)); On 2017/05/24 22:59:52, msramek wrote: > On 2017/05/22 19:36:09, mmenke wrote: > > I'm not going to block on this, but GMOCK syntax is very convoluted and > unclear > > - the network stack team, at least, generally avoids any use of GMOCK for this > > reason, and instead implements our own mock classes. This also makes it > easier > > for third parties to start hacking on Chrome, and more generally makes tests > > more debuggable. > > In that case I'd prefer to keep it as is - as usual, in order not to expand this > already large CL. > > I agree with you that GMOCK errors are hard to debug, but custom mock classes > typically cost a lot of boilerplate code if you want them to support things like > testing::_, Times(0), verbose outputs etc., so I'm actually surprised to hear > that. Not going to advocate any more for getting rid of gmock after this statement, but.... MockResourceThrottleDelegate gets nothing from gmock, since you never use it in any EXPECT_CALLs, so that just leaves TestThrottle. TestThrottle only has one mock method, which just needs to track number of times called, and last values passed in to it, so does seem like not a whole lot of extra code or complexity, here. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:412: EXPECT_CALL(throttle, ClearSiteData(_, _, _, _)).Times(0); On 2017/05/24 22:59:52, msramek wrote: > On 2017/05/22 19:36:09, mmenke wrote: > > I don't think we ever check that ClearSiteData is called with the right > > parameters in any of these tests? Suggest doing it in this one, or one like > it, > > and setting each one individually (ParseHeader makes sure we're parsing them > > correctly, but I don't think anything is making sure we're passing them > > correctly to the ClearSiteData call, other than the browser_tests). > > I expanded the ParseHeader test, since that one tests different datatypes, so it > seemed to fit there the best. > > Although I don't think it's super valuable, since even if we verify that we get > that far, we still don't see if BrowsingDataRemover is called correctly. > > That's exactly why we now have these new integration-y browsertests, which you > asked me to write a few months ago :) True. My feeling is just that we should test individual classes as much as possible in unit tests. If something breaks, we'd really prefer a unit test failure to debug, instead of a browser test one. Of course, we also need to test things are plugged in correctly, and for other things, unit tests just don't work that well, but I feel that unit tests should be as comprehensive as we can manage, though that's certainly a debatable point.
Really like those browser tests! https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:113: return; This isn't handling the case web_contents_getter returns nullptr correctly - we'll call RunAndDestroySelfWhenDone, which will try and dereference remover_, which will be null. Should have a test that covers that. My suggested fix is to make this method take a WebContents*, and use the getter (With null check) in the static Run() method. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:61: ASSERT_EQ(expected_title_16, title_watcher.WaitAndGetTitle()); I don't think we want to depend on the implementation of navigation - i.e., creating the watcher after we start navigation is relying on squatting on the UI thread blocking the title from being updated. This is probably going to always be safe to do, but I don't think we want to make that a dependency of this test. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:79: const int origin_type_mask = kOriginTypeMatch https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:162: waitable_event.Wait(); optional: I think RunLoops are a lot more common in tests than events. Also seems strange the sometimes events are used in this file, sometimes RunLoops are, for pretty much the same thing. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:169: void InitializeCookieStore( Why is this needed? Can't AddCookie/GetCookie take the URLRequestContextGetter themselves? https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:289: // Example: "https://localhost/?file=file.html +" https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:343: void AddCookieCallback(base::WaitableEvent* waitable_event, bool success) { static? https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:350: void GetCookiesCallback(base::WaitableEvent* waitable_event, static? https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:422: GURL base_urls[3] = { These don't mean the same thing as https_server()->base_url(), so perhaps another name? Do we even need different paths for these? Without paths, they are analogous to base_url, actually. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:425: https_server()->GetURL("origin3.com", "/actual-image.png"), Worth noting that actual-image actually just ends up being an HTTP response with an empty body and no content-type, instead of an actual image? https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:477: // and ignored for insecure ones. This doesn't check the header is ignored for insecure resource loads, does it? Just secure resource loads from secure and insecure pages. Should it (Or another test) check for insecure resource loads, instead of just checking for insecure navigations? Could add an insecure resource to the above test pretty easily. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:568: // origin4.com/resource (-> server; should respect header) Out of paranoia, and since we check ordering, would it make sense to reorder these as: origin1.com/resource origin2.com/resource_from_sw origin3.com/resource_from_sw origin4.com/resource origin1.com/resource_from_sw origin2.com/resource origin3.com/resource_from_sw origin4.com/resource This may make the test slightly more robust, as we're checking the order in which ClearSiteDataCookies is called - I can't see how it would happen, but it would be slightly more robust in the case we somehow managed to flip which of the two requests to an origin cleared the cookie. Anyhow, even if we ran into that bug, this test should still catch it, just being paranoid. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:746: EXPECT_EQ(2u, cookies.size()); ASSERT_EQ? https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:748: EXPECT_EQ(cookies[0].Domain(), "origin2.com"); nit: Why the flipped order? https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:762: GURL url = https_server()->GetURL("origin1.com", "/clear-site-data"); Why doesn't origin1's SW intercept this and return the default response? https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:768: EXPECT_EQ(1u, service_workers.size()); ASSERT_EQ? https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:196: TestThrottle throttle(request.get(), Should all TestThrottles use StrictMock? I don't think there are any cases where non-EXPECTED calls are expected? https://codereview.chromium.org/2368923003/diff/360001/content/test/data/brow... File content/test/data/browsing_data/worker_setup.html (right): https://codereview.chromium.org/2368923003/diff/360001/content/test/data/brow... content/test/data/browsing_data/worker_setup.html:13: .then(() => { Going to just assume this does what it sounds like it does. Not familiar enough with promises or SWs to evaluate it.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #14 (id:380001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I addressed your comments, but in two cases I didn't understand the concern. I have improved the test coverage again, but I don't have the test for null WebContents yet. PTAL :) https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:195: if (message.url == last_seen_url) { On 2017/05/25 15:19:01, mmenke wrote: > On 2017/05/24 22:59:51, msramek wrote: > > On 2017/05/22 18:35:35, mmenke wrote: > > > Any reason to do these all at the end, instead of on a per-request basis? > > i.e., > > > just dump messages after each HandleHeader call, and use the current URL (If > > > there are any messages to pass). > > > > > > That also makes the "Clearing" in strings not a lie - currently, unless the > > > request is cancelled while clearing, when we print out "clearing foo", we've > > > already long since cleared "foo". > > > > That's a good point - changed "Clearing..." To "Cleared...". > > > > The reason why we only output this at the end are navigations. When this was a > > NavigationThrottle, the most reasonable usage was a navigation redirect chain: > > > > Deletion request -> URL that returns the header -> landing page. > > > > ...in which case, the output would never be visible, as the DevTools would > only > > show messages from the landing page. > > > > Since we support subresource requests with the CL, maybe we don't care about > > that case anymore, I'm not sure. Or we could handle it separately. > > Hrm....But for subresources, clearing will affect future resource requests, so > delaying it seems incorrect. I'll defer to you here, but not a fan of the > current logic. But we're not delaying the clearing itself, just the console output? The clearing actually happens at each redirect. The developer always sees what was cleared, but only when the request ends, not in real time. This might matter in some cases, but it's mostly a cosmetic problem. Perhaps a good solution would be to keep the current behavior if the request type is navigation, and flush with each redirect if it's a subresource request. But again - at this point I'm trying to make the header work at all. Pretty console outputs can be done in a followup CL, especially with the feedback from developers. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:236: messages_.clear(); On 2017/05/25 15:19:01, mmenke wrote: > On 2017/05/24 22:59:51, msramek wrote: > > On 2017/05/22 18:35:34, mmenke wrote: > > > Is this needed, or is std::move guaranteed to clear the source vector? > > > > I'm not sure that std::vector<> is safe to use after move. What I did find is > > that "clear()" is always safe to call, and always brings the vector to valid > > empty state. > > > > (I think that in practice, it's safe to omit this, but I'd like to play it > > safe.) > > Googling for an answer, looks like it may not be guaranteed, so this makes > sense. Acknowledged. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:499: void ClearSiteDataThrottle::TaskFinished() { On 2017/05/24 22:59:51, msramek wrote: > On 2017/05/22 18:35:34, mmenke wrote: > > DCHECK_CURRENTLY_ON(BrowserThread::IO); > > I didn't add this on purpose, because the unittest is single-threaded (only the > actual deletion, which is mocked in the unittest, has these DCHECKs). > > I didn't have time to look into this further today, but if you think the DCHECK > should be here, I can add real threads to the unittest in the next iteration. Fixed this in the latest patchset. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:79: const int origin_type_mask = On 2017/05/25 15:19:01, mmenke wrote: > kOriginTypeMatch Done. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:227: } On 2017/05/25 15:19:01, mmenke wrote: > On 2017/05/24 22:59:52, msramek wrote: > > On 2017/05/22 19:36:09, mmenke wrote: > > > optional: Hrm...There's no testing of the output of > ClearSiteDataOnUIThread. > > > Would it make sense to have the Delegate thing create the full sctrings, > then > > > pass them over to the UI thread, just for better testing? Not going to > insist > > > on it. > > > > Do you mean OutputMessagesOnUIThread()? I must admit I don't fully remember > why > > (as I wrote this test long ago), but I found unittesting the actual console > > output to be nontrivial. I can try again, but I'd like to not add much more > code > > in this CL. > > Sorry, I did indeed mean OutputMessagesOnUIThread. But what I mean we should > test is not whether it outputs to console, but the whole last_seen_url thing. I > don't think a single test relies on that behavior working. > > > A mitigation is that a bug where the strings don't make it to the console is > > visually discoverable. > > It's really not. Suppose the last_seen_url logic was broken in the case of > multiple different URLs resetting cookies. What do you think are the odds > someone would actually notice that? Oh, that's what you mean. OK, I added a unittest for the formatting of the console messages, This required changes: --> ConsoleMessagesDelegate requires the output method to be mockable, so that we can read it in the tests but not pass it to a real WebContents. However, since the class lives on the IO thread, the output method must be on the UI thread, so to avoid lifetime problems I want to keep it static. Therefore, I now pass the output method as a base::Callback. Unittests supply a different base::Callback. --> In the unittest, I need to simulate the Throttle redirecting over different URLs. Again, changing the URL of a URLRequest for testing purposes seems to be nontrivial, so instead, I defined virtual GetCurrentURL() which is overriden in the tests. An alternative would be to have the throttle keep track of the URL itself by not calling URLRequest::url(), but instead watching RedirectInfo instances passed in WillRedirectRequest(). I liked the virtual method approach a bit better. https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:412: EXPECT_CALL(throttle, ClearSiteData(_, _, _, _)).Times(0); On 2017/05/25 15:19:01, mmenke wrote: > On 2017/05/24 22:59:52, msramek wrote: > > On 2017/05/22 19:36:09, mmenke wrote: > > > I don't think we ever check that ClearSiteData is called with the right > > > parameters in any of these tests? Suggest doing it in this one, or one like > > it, > > > and setting each one individually (ParseHeader makes sure we're parsing them > > > correctly, but I don't think anything is making sure we're passing them > > > correctly to the ClearSiteData call, other than the browser_tests). > > > > I expanded the ParseHeader test, since that one tests different datatypes, so > it > > seemed to fit there the best. > > > > Although I don't think it's super valuable, since even if we verify that we > get > > that far, we still don't see if BrowsingDataRemover is called correctly. > > > > That's exactly why we now have these new integration-y browsertests, which you > > asked me to write a few months ago :) > > True. My feeling is just that we should test individual classes as much as > possible in unit tests. If something breaks, we'd really prefer a unit test > failure to debug, instead of a browser test one. Of course, we also need to > test things are plugged in correctly, and for other things, unit tests just > don't work that well, but I feel that unit tests should be as comprehensive as > we can manage, though that's certainly a debatable point. Acknowledged. Fair enough. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:113: return; On 2017/05/25 19:23:09, mmenke wrote: > This isn't handling the case web_contents_getter returns nullptr correctly - > we'll call RunAndDestroySelfWhenDone, which will try and dereference remover_, > which will be null. Should have a test that covers that. > > My suggested fix is to make this method take a WebContents*, and use the getter > (With null check) in the static Run() method. Done. Thanks for catching! I fixed this, but I'll need to think a bit about how to properly test this. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:61: ASSERT_EQ(expected_title_16, title_watcher.WaitAndGetTitle()); On 2017/05/25 19:23:09, mmenke wrote: > I don't think we want to depend on the implementation of navigation - i.e., > creating the watcher after we start navigation is relying on squatting on the UI > thread blocking the title from being updated. This is probably going to always > be safe to do, but I don't think we want to make that a dependency of this test. Sorry, I don't understand. TitleWatcher constructor doesn't really do anything, it just stores the expected title in a member variable. It's WaitAndGetTitle() that does the actual work, and that one obviously must be called after the navigation is executed. It checks the title first and then spins a RunLoop waiting for the title update event. This will always work if the title update happens on the UI thread, which it should, because that's where WebContents lives. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:79: const int origin_type_mask = On 2017/05/25 19:23:09, mmenke wrote: > kOriginTypeMatch Done. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:162: waitable_event.Wait(); On 2017/05/25 19:23:10, mmenke wrote: > optional: I think RunLoops are a lot more common in tests than events. Also > seems strange the sometimes events are used in this file, sometimes RunLoops > are, for pretty much the same thing. Done. (Changed to RunLoops) https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:169: void InitializeCookieStore( On 2017/05/25 19:23:10, mmenke wrote: > Why is this needed? Can't AddCookie/GetCookie take the URLRequestContextGetter > themselves? We need to do two things on the IO thread: 1. Call GetURLRequestContext() in order to get to CookieStore. 2. Execute a method on CookieStore (SetCookieWithOptionsAsync()/GetAllCookiesAsync() in AddCookie()/GetCookie() respectively). It seems to me that the cleanest solution is to do 1. separately in a helper function, then just pass the pointer to CookieStore in both instances of 2. Alternatively, AddCookie() and GetCookies() could contain two PostTask()s on IO thread to execute both operations. We would end up with something like: PostTask(CookieStore::operator=, base::Unretained(cookie_store_), ...) PostTask(SetCookieWithOptionsAsync, base::Unretained(cookie_store), ...) Or we could have two helper methods AddCookieOnIOThread()/GetCookiesOnIOThread that both execute both operations. But the current solution only requires one helper method. So it seems cleaner. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:289: // Example: "https://localhost/?file=file.html On 2017/05/25 19:23:10, mmenke wrote: > +" Done. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:343: void AddCookieCallback(base::WaitableEvent* waitable_event, bool success) { On 2017/05/25 19:23:10, mmenke wrote: > static? Done. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:350: void GetCookiesCallback(base::WaitableEvent* waitable_event, On 2017/05/25 19:23:09, mmenke wrote: > static? Done. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:422: GURL base_urls[3] = { On 2017/05/25 19:23:09, mmenke wrote: > These don't mean the same thing as https_server()->base_url(), so perhaps > another name? Do we even need different paths for these? Without paths, they > are analogous to base_url, actually. I added the paths here partially to document the meaning of the three URLs (redirect chain start, middle, and end), and partially to test that they don't matter (i.e. the header is respected from any path on the origin). Let me rename them to make it clearer. I also renamed "base_urls" to "page_urls/resource_urls". Maybe there's a better name, but I want to convey that these URLs represent pages we navigate to / resources that we fetch, and then below we add the query parameter to adjust their behavior. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:425: https_server()->GetURL("origin3.com", "/actual-image.png"), On 2017/05/25 19:23:09, mmenke wrote: > Worth noting that actual-image actually just ends up being an HTTP response with > an empty body and no content-type, instead of an actual image? As mentioned in the above comment, I renamed the paths to avoid confusion. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:477: // and ignored for insecure ones. On 2017/05/25 19:23:10, mmenke wrote: > This doesn't check the header is ignored for insecure resource loads, does it? > Just secure resource loads from secure and insecure pages. Should it (Or > another test) check for insecure resource loads, instead of just checking for > insecure navigations? Could add an insecure resource to the above test pretty > easily. It does? This test tries all four combinations - insecure/secure resource on insecure/secure navigation. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:568: // origin4.com/resource (-> server; should respect header) On 2017/05/25 19:23:09, mmenke wrote: > Out of paranoia, and since we check ordering, would it make sense to reorder > these as: > > origin1.com/resource > origin2.com/resource_from_sw > origin3.com/resource_from_sw > origin4.com/resource > origin1.com/resource_from_sw > origin2.com/resource > origin3.com/resource_from_sw > origin4.com/resource > > This may make the test slightly more robust, as we're checking the order in > which ClearSiteDataCookies is called - I can't see how it would happen, but it > would be slightly more robust in the case we somehow managed to flip which of > the two requests to an origin cleared the cookie. > > Anyhow, even if we ran into that bug, this test should still catch it, just > being paranoid. Done. That's fine, I like paranoid tests :) Interestingly, changing the order of the non-SW fetches in worker.js to 1-4-2-4 resulted in the EmbeddedTestServer seeing requests 1-4-4-2. My assumption was that the last fetch to origin4 was sped up by the fact that there was already a previous connection to origin4? Preconnect or something like that? In any case, this is unrelated to Clear-Site-Data, so I just changed worker.js to fetch the files sequentially. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:746: EXPECT_EQ(2u, cookies.size()); On 2017/05/25 19:23:10, mmenke wrote: > ASSERT_EQ? Done. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:748: EXPECT_EQ(cookies[0].Domain(), "origin2.com"); On 2017/05/25 19:23:10, mmenke wrote: > nit: Why the flipped order? Done. Flipped, no reason :) https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:762: GURL url = https_server()->GetURL("origin1.com", "/clear-site-data"); On 2017/05/25 19:23:09, mmenke wrote: > Why doesn't origin1's SW intercept this and return the default response? That's a great question :) Apparently I confused the first parameter to ServiceWorkerContextWrapper::RegisterServiceWorker(), which was supposed to be the scope, with the URL that registered it. Fixed. I also added an observer to wait until the worker is actually activated, and expanded the test to verify that navigation inside the scope will not delete it, but outside will. This is practically already covered by the other test which checks service worker fetches, but still... a nice addition :) https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:768: EXPECT_EQ(1u, service_workers.size()); On 2017/05/25 19:23:10, mmenke wrote: > ASSERT_EQ? Done. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:196: TestThrottle throttle(request.get(), On 2017/05/25 19:23:10, mmenke wrote: > Should all TestThrottles use StrictMock? I don't think there are any cases > where non-EXPECTED calls are expected? There are some, for example InvalidOrigin only tested the result of WillProcessResponse but not the actual call. But there's a bigger problem with changing TestThrottle to StrictMock - one of the parameters of ClearSiteDataThrottle is std::unique_ptr, which is non-copyable; and that doesn't work well with GMock (specifically, StrictMock doesn't compile because it calls the copy constructor somewhere). https://codereview.chromium.org/2368923003/diff/360001/content/test/data/brow... File content/test/data/browsing_data/worker_setup.html (right): https://codereview.chromium.org/2368923003/diff/360001/content/test/data/brow... content/test/data/browsing_data/worker_setup.html:13: .then(() => { On 2017/05/25 19:23:10, mmenke wrote: > Going to just assume this does what it sounds like it does. Not familiar enough > with promises or SWs to evaluate it. Acknowledged. This part was reviewed by falken@ (back then...). If there was a bug in this JS, the corresponding browsertest would timeout.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
quick responses https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:195: if (message.url == last_seen_url) { On 2017/05/30 21:58:44, msramek wrote: > On 2017/05/25 15:19:01, mmenke wrote: > > On 2017/05/24 22:59:51, msramek wrote: > > > On 2017/05/22 18:35:35, mmenke wrote: > > > > Any reason to do these all at the end, instead of on a per-request basis? > > > i.e., > > > > just dump messages after each HandleHeader call, and use the current URL > (If > > > > there are any messages to pass). > > > > > > > > That also makes the "Clearing" in strings not a lie - currently, unless > the > > > > request is cancelled while clearing, when we print out "clearing foo", > we've > > > > already long since cleared "foo". > > > > > > That's a good point - changed "Clearing..." To "Cleared...". > > > > > > The reason why we only output this at the end are navigations. When this was > a > > > NavigationThrottle, the most reasonable usage was a navigation redirect > chain: > > > > > > Deletion request -> URL that returns the header -> landing page. > > > > > > ...in which case, the output would never be visible, as the DevTools would > > only > > > show messages from the landing page. > > > > > > Since we support subresource requests with the CL, maybe we don't care about > > > that case anymore, I'm not sure. Or we could handle it separately. > > > > Hrm....But for subresources, clearing will affect future resource requests, so > > delaying it seems incorrect. I'll defer to you here, but not a fan of the > > current logic. > > But we're not delaying the clearing itself, just the console output? > > The clearing actually happens at each redirect. The developer always sees what > was cleared, but only when the request ends, not in real time. This might matter > in some cases, but it's mostly a cosmetic problem. > > Perhaps a good solution would be to keep the current behavior if the request > type is navigation, and flush with each redirect if it's a subresource request. > > But again - at this point I'm trying to make the header work at all. Pretty > console outputs can be done in a followup CL, especially with the feedback from > developers. My main concern is subresources - events will be logged in orders other than the order in which they occurred. This can be a bit confusing, and I'd rather not have Internals>network>cookies bugs about this, particularly as no one owns the label, which means there's a fair chance I'll be the one looking at them, despite never having owned the cookie system, because no one else looks at such bugs... And I'd rather not have to deal with investigating them. So this is primarily motivated by a desire to save Chrome developer time (Especially my own), by making the interface less confusing. Fact is that errors in this sort of thing will most often end up either in front of the network team, or perhaps the DevTools team, so I'm trying to minimize our maintenance burden. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:61: ASSERT_EQ(expected_title_16, title_watcher.WaitAndGetTitle()); On 2017/05/30 21:58:45, msramek wrote: > On 2017/05/25 19:23:09, mmenke wrote: > > I don't think we want to depend on the implementation of navigation - i.e., > > creating the watcher after we start navigation is relying on squatting on the > UI > > thread blocking the title from being updated. This is probably going to > always > > be safe to do, but I don't think we want to make that a dependency of this > test. > > Sorry, I don't understand. TitleWatcher constructor doesn't really do anything, > it just stores the expected title in a member variable. > > It's WaitAndGetTitle() that does the actual work, and that one obviously must be > called after the navigation is executed. > > It checks the title first and then spins a RunLoop waiting for the title update > event. This will always work if the title update happens on the UI thread, which > it should, because that's where WebContents lives. TitleWatcher starts watching, which is rather important - i.e., if the title changes before you start watching, you could have a problem. That having been said, it looks like TitleWatcher::WaitAndGetTitle now checks the current title of the tab - it didn't always do that, so if the title had already changed by the time you created the watcher, the test would fail. But looks like this has long since been fixed. Now it's just if the title changes away from the one you want that you have a problem, which shouldn't happen here. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:477: // and ignored for insecure ones. On 2017/05/30 21:58:45, msramek wrote: > On 2017/05/25 19:23:10, mmenke wrote: > > This doesn't check the header is ignored for insecure resource loads, does it? > > > Just secure resource loads from secure and insecure pages. Should it (Or > > another test) check for insecure resource loads, instead of just checking for > > insecure navigations? Could add an insecure resource to the above test pretty > > easily. > > It does? This test tries all four combinations - insecure/secure resource on > insecure/secure navigation. Hrm...not sure what I was thinking. I'll take another look tomorrow, but unless I comment again, consider it resolved.
May give this another once over, but I'm happy with this. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:568: // origin4.com/resource (-> server; should respect header) On 2017/05/30 21:58:44, msramek wrote: > On 2017/05/25 19:23:09, mmenke wrote: > > Out of paranoia, and since we check ordering, would it make sense to reorder > > these as: > > > > origin1.com/resource > > origin2.com/resource_from_sw > > origin3.com/resource_from_sw > > origin4.com/resource > > origin1.com/resource_from_sw > > origin2.com/resource > > origin3.com/resource_from_sw > > origin4.com/resource > > > > This may make the test slightly more robust, as we're checking the order in > > which ClearSiteDataCookies is called - I can't see how it would happen, but it > > would be slightly more robust in the case we somehow managed to flip which of > > the two requests to an origin cleared the cookie. > > > > Anyhow, even if we ran into that bug, this test should still catch it, just > > being paranoid. > > Done. That's fine, I like paranoid tests :) > > Interestingly, changing the order of the non-SW fetches in worker.js to 1-4-2-4 > resulted in the EmbeddedTestServer seeing requests 1-4-4-2. > > My assumption was that the last fetch to origin4 was sped up by the fact that > there was already a previous connection to origin4? Preconnect or something like > that? > > In any case, this is unrelated to Clear-Site-Data, so I just changed worker.js > to fetch the files sequentially. I didn't realize it wasn't fetching sequentially...Yea, that seems like a really good idea. https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:212: OutputFormattedMessageFunction& output_formatted_message_function) { Are those spaces around the :: valid google style? I've never seen that before. https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:216: web_contents_getter.is_null() ? nullptr : web_contents_getter.Run(); I guess this is just for tests, but can we instead pass in a WebContentsGetter that returns nullptr? Don't like test code in production, unless it's the only reasonable option. https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:224: message.text); Can't remember if I raised this earlier - thought I did, but don't see the comment. If we have the same URL requested twice, this will merge their log messages. Should it? Suppose we have a redirected request http://foo/clear-stuff -> http://foo/clear-stuff -> http://foo/do-not-stuff/ -> http://foo/clear-stuff We'd only have one prefix, despite having 3 requests that clear stuff. The simplest workaround would be to add a header as a message, each time we see a clear-data header (Each time we see the header, we're going to output some message, anyways - either a claim we cleared something or an error). Unlikely to matter much, but seems trivial to change, and even makes the code a little simpler, as then we no longer need to track the URLs, right? https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:223: run_loop.QuitClosure()))); QuitClosure has to be called on the same thread the RunLoop is on, doesn't it? (i.e, these should be PostTaskAndReplies, instead of passing in the callbacks. Also makes the methods marginally simpler) https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:459: https_server()->GetURL("origin3.com", "/actual-image.png"), On 2017/05/30 21:58:45, msramek wrote: > On 2017/05/25 19:23:09, mmenke wrote: > > Worth noting that actual-image actually just ends up being an HTTP response > with > > an empty body and no content-type, instead of an actual image? > > As mentioned in the above comment, I renamed the paths to avoid confusion. I don't think that addresses the issue? None of these are actual images. I looked around for image files when I first saw this CL, and was confused before I realized that there are no images. https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:808: // deleted. Would it give better cover to request a URL that the SW maps to a real network request, and make sure that request clears the SW?
One other thing: You should probably add the new browser test fixture to testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter, since it won't work with the nascent network service.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #15 (id:420001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #15 (id:440001) has been deleted
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:195: if (message.url == last_seen_url) { On 2017/05/30 22:58:35, mmenke wrote: > On 2017/05/30 21:58:44, msramek wrote: > > On 2017/05/25 15:19:01, mmenke wrote: > > > On 2017/05/24 22:59:51, msramek wrote: > > > > On 2017/05/22 18:35:35, mmenke wrote: > > > > > Any reason to do these all at the end, instead of on a per-request > basis? > > > > i.e., > > > > > just dump messages after each HandleHeader call, and use the current URL > > (If > > > > > there are any messages to pass). > > > > > > > > > > That also makes the "Clearing" in strings not a lie - currently, unless > > the > > > > > request is cancelled while clearing, when we print out "clearing foo", > > we've > > > > > already long since cleared "foo". > > > > > > > > That's a good point - changed "Clearing..." To "Cleared...". > > > > > > > > The reason why we only output this at the end are navigations. When this > was > > a > > > > NavigationThrottle, the most reasonable usage was a navigation redirect > > chain: > > > > > > > > Deletion request -> URL that returns the header -> landing page. > > > > > > > > ...in which case, the output would never be visible, as the DevTools would > > > only > > > > show messages from the landing page. > > > > > > > > Since we support subresource requests with the CL, maybe we don't care > about > > > > that case anymore, I'm not sure. Or we could handle it separately. > > > > > > Hrm....But for subresources, clearing will affect future resource requests, > so > > > delaying it seems incorrect. I'll defer to you here, but not a fan of the > > > current logic. > > > > But we're not delaying the clearing itself, just the console output? > > > > The clearing actually happens at each redirect. The developer always sees what > > was cleared, but only when the request ends, not in real time. This might > matter > > in some cases, but it's mostly a cosmetic problem. > > > > Perhaps a good solution would be to keep the current behavior if the request > > type is navigation, and flush with each redirect if it's a subresource > request. > > > > But again - at this point I'm trying to make the header work at all. Pretty > > console outputs can be done in a followup CL, especially with the feedback > from > > developers. > > My main concern is subresources - events will be logged in orders other than the > order in which they occurred. This can be a bit confusing, and I'd rather not > have Internals>network>cookies bugs about this, particularly as no one owns the > label, which means there's a fair chance I'll be the one looking at them, > despite never having owned the cookie system, because no one else looks at such > bugs... And I'd rather not have to deal with investigating them. So this is > primarily motivated by a desire to save Chrome developer time (Especially my > own), by making the interface less confusing. Fact is that errors in this sort > of thing will most often end up either in front of the network team, or perhaps > the DevTools team, so I'm trying to minimize our maintenance burden. Fair enough. For the record, of course, please do route related bugs to me. I implemented what I proposed in the previous comment - flushing the console output with each redirect if it's a subresource request. Also added a test for that. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:113: return; On 2017/05/30 21:58:44, msramek wrote: > On 2017/05/25 19:23:09, mmenke wrote: > > This isn't handling the case web_contents_getter returns nullptr correctly - > > we'll call RunAndDestroySelfWhenDone, which will try and dereference remover_, > > which will be null. Should have a test that covers that. > > > > My suggested fix is to make this method take a WebContents*, and use the > getter > > (With null check) in the static Run() method. > > Done. Thanks for catching! > > I fixed this, but I'll need to think a bit about how to properly test this. Just to follow up on this, we can't test this with the current unittest setup, since that part is mocked out. But I added a short browsertest to sanity check closing the Shell. It's platform dependent and I don't really know what that does, but I would assume that it closes WebContents, which should cancel requests, which should destroy the throttle, but not this class which maintains its own lifetime. Then, if this class accesses something it shouldn't, the test would crash. https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/360001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:61: ASSERT_EQ(expected_title_16, title_watcher.WaitAndGetTitle()); On 2017/05/30 22:58:35, mmenke wrote: > On 2017/05/30 21:58:45, msramek wrote: > > On 2017/05/25 19:23:09, mmenke wrote: > > > I don't think we want to depend on the implementation of navigation - i.e., > > > creating the watcher after we start navigation is relying on squatting on > the > > UI > > > thread blocking the title from being updated. This is probably going to > > always > > > be safe to do, but I don't think we want to make that a dependency of this > > test. > > > > Sorry, I don't understand. TitleWatcher constructor doesn't really do > anything, > > it just stores the expected title in a member variable. > > > > It's WaitAndGetTitle() that does the actual work, and that one obviously must > be > > called after the navigation is executed. > > > > It checks the title first and then spins a RunLoop waiting for the title > update > > event. This will always work if the title update happens on the UI thread, > which > > it should, because that's where WebContents lives. > > TitleWatcher starts watching, which is rather important - i.e., if the title > changes before you start watching, you could have a problem. > > That having been said, it looks like TitleWatcher::WaitAndGetTitle now checks > the current title of the tab - it didn't always do that, so if the title had > already changed by the time you created the watcher, the test would fail. But > looks like this has long since been fixed. Now it's just if the title changes > away from the one you want that you have a problem, which shouldn't happen here. Acknowledged. That's true, but indeed isn't the case in these tests. https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:212: OutputFormattedMessageFunction& output_formatted_message_function) { On 2017/05/31 16:25:33, mmenke wrote: > Are those spaces around the :: valid google style? I've never seen that before. I don't think so. I ran git cl format, but it might have been confused by a trailing whitespace or something. Fixed. https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:216: web_contents_getter.is_null() ? nullptr : web_contents_getter.Run(); On 2017/05/31 16:25:33, mmenke wrote: > I guess this is just for tests, but can we instead pass in a WebContentsGetter > that returns nullptr? Don't like test code in production, unless it's the only > reasonable option. Done. Yes, this was just for the tests, and it's actually not needed anymore, since I change the test to not call this directly. https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:224: message.text); On 2017/05/31 16:25:33, mmenke wrote: > Can't remember if I raised this earlier - thought I did, but don't see the > comment. If we have the same URL requested twice, this will merge their log > messages. Should it? > > Suppose we have a redirected request http://foo/clear-stuff -> > http://foo/clear-stuff -> http://foo/do-not-stuff/ -> http://foo/clear-stuff > > We'd only have one prefix, despite having 3 requests that clear stuff. The > simplest workaround would be to add a header as a message, each time we see a > clear-data header (Each time we see the header, we're going to output some > message, anyways - either a claim we cleared something or an error). Unlikely > to matter much, but seems trivial to change, and even makes the code a little > simpler, as then we no longer need to track the URLs, right? The main reason for this |last_seen_url| was that a header can output more than one line (e.g. it can complain that it doesn't recognize one datatype, but still delete other ones - this is for forward compatibility). So we get: """ Clear-Site-Data header on foo: blah blah blah blah blah blah """ My intention was to avoid spamming the prefix ("Clear-Site-Data header"), but admittedly, this does look a bit ugly. We could join messages from one origin into one multiline string, but then we lose the information about message level. So it might really be easiest to just add the prefix everywhere. Note that this doesn't fully address your concern, since if we have three messages from two origins, you can't tell if it's 2+1, 1+2 from the syntax (you probably can from their meaning though). https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:223: run_loop.QuitClosure()))); On 2017/05/31 16:25:33, mmenke wrote: > QuitClosure has to be called on the same thread the RunLoop is on, doesn't it? > (i.e, these should be PostTaskAndReplies, instead of passing in the callbacks. > Also makes the methods marginally simpler) PostTaskAndReply in this case would quit the RunLoop as soon as SetCookieWithOptionsAsync is finished on the IO thread, which is too soon. SetCookieWithOptionsAsync itself is asynchronous, so we have to wait until it calls its callback. It's true that RunLoop::Quit() would be called from a different thread. That's the reason why I originally had WaitableEvent here (I generally synchronize using RunLoop on the same thread and WaitableEvent on a different thread). You commented earlier that I should use RunLoop consistently. And that actually works fine: https://cs.chromium.org/chromium/src/base/run_loop.cc?type=cs&q=RunLoop::Quit... So I think this can stay. https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:459: https_server()->GetURL("origin3.com", "/actual-image.png"), On 2017/05/31 16:25:33, mmenke wrote: > On 2017/05/30 21:58:45, msramek wrote: > > On 2017/05/25 19:23:09, mmenke wrote: > > > Worth noting that actual-image actually just ends up being an HTTP response > > with > > > an empty body and no content-type, instead of an actual image? > > > > As mentioned in the above comment, I renamed the paths to avoid confusion. > > I don't think that addresses the issue? None of these are actual images. I > looked around for image files when I first saw this CL, and was confused before > I realized that there are no images. Yes! Sorry. I changed the name of the array and then forgot about the strings. Renamed not to reference images. Done (this time really). https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:808: // deleted. On 2017/05/31 16:25:33, mmenke wrote: > Would it give better cover to request a URL that the SW maps to a real network > request, and make sure that request clears the SW? Done. Yes, I guess that's a more general (and interesting) case.
On 2017/05/31 21:10:29, mmenke wrote: > One other thing: You should probably add the new browser test fixture to > testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter, since it > won't work with the nascent network service. Done. Good point, and I will be looking into that soonish: crbug.com/721398.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Going to punt this until Monday, just because you're OOO. Hope to sign off on it then (Or make a final round of comments, and sign off on it immediately on when they're addressed). Sorry for the repeated slowness on this CL - know that can make things more painful.
Response to an old comment (Sending it separately to reduce chance it's overlooked) https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:224: message.text); On 2017/06/01 22:02:27, msramek (OOO until June 6) wrote: > On 2017/05/31 16:25:33, mmenke wrote: > > Can't remember if I raised this earlier - thought I did, but don't see the > > comment. If we have the same URL requested twice, this will merge their log > > messages. Should it? > > > > Suppose we have a redirected request http://foo/clear-stuff -> > > http://foo/clear-stuff -> http://foo/do-not-stuff/ -> http://foo/clear-stuff > > > > We'd only have one prefix, despite having 3 requests that clear stuff. The > > simplest workaround would be to add a header as a message, each time we see a > > clear-data header (Each time we see the header, we're going to output some > > message, anyways - either a claim we cleared something or an error). Unlikely > > to matter much, but seems trivial to change, and even makes the code a little > > simpler, as then we no longer need to track the URLs, right? > > The main reason for this |last_seen_url| was that a header can output more than > one line (e.g. it can complain that it doesn't recognize one datatype, but still > delete other ones - this is for forward compatibility). > > So we get: > > """ > Clear-Site-Data header on foo: blah blah blah > blah blah blah > """ > > My intention was to avoid spamming the prefix ("Clear-Site-Data header"), but > admittedly, this does look a bit ugly. > > We could join messages from one origin into one multiline string, but then we > lose the information about message level. > > So it might really be easiest to just add the prefix everywhere. Note that this > doesn't fully address your concern, since if we have three messages from two > origins, you can't tell if it's 2+1, 1+2 from the syntax (you probably can from > their meaning though). I'm not following. Couldn't we do: bool ClearSiteDataThrottle::HandleHeader() { if (/* header not present */) return false; delegate_->AddMessage("Processing Clear-Site-Data header from %s:"); ...
Still need a final pass on the browser tests https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:224: message.text); On 2017/06/05 21:05:50, mmenke wrote: > On 2017/06/01 22:02:27, msramek (OOO until June 6) wrote: > > On 2017/05/31 16:25:33, mmenke wrote: > > > Can't remember if I raised this earlier - thought I did, but don't see the > > > comment. If we have the same URL requested twice, this will merge their log > > > messages. Should it? > > > > > > Suppose we have a redirected request http://foo/clear-stuff -> > > > http://foo/clear-stuff -> http://foo/do-not-stuff/ -> http://foo/clear-stuff > > > > > > We'd only have one prefix, despite having 3 requests that clear stuff. The > > > simplest workaround would be to add a header as a message, each time we see > a > > > clear-data header (Each time we see the header, we're going to output some > > > message, anyways - either a claim we cleared something or an error). > Unlikely > > > to matter much, but seems trivial to change, and even makes the code a > little > > > simpler, as then we no longer need to track the URLs, right? > > > > The main reason for this |last_seen_url| was that a header can output more > than > > one line (e.g. it can complain that it doesn't recognize one datatype, but > still > > delete other ones - this is for forward compatibility). > > > > So we get: > > > > """ > > Clear-Site-Data header on foo: blah blah blah > > blah blah blah > > """ > > > > My intention was to avoid spamming the prefix ("Clear-Site-Data header"), but > > admittedly, this does look a bit ugly. > > > > We could join messages from one origin into one multiline string, but then we > > lose the information about message level. > > > > So it might really be easiest to just add the prefix everywhere. Note that > this > > doesn't fully address your concern, since if we have three messages from two > > origins, you can't tell if it's 2+1, 1+2 from the syntax (you probably can > from > > their meaning though). > > I'm not following. Couldn't we do: > > bool ClearSiteDataThrottle::HandleHeader() { > if (/* header not present */) > return false; > > delegate_->AddMessage("Processing Clear-Site-Data header from %s:"); > ... (Note: I'm happy with the current approach) https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:50: const char kConsoleMessagePrefix[] = "Clear-Site-Data header on '%s': %s"; nit: Not actually a prefix https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:62: info->GetResourceType() == RESOURCE_TYPE_SUB_FRAME); info && ResourceType::IsResourceTypeFrame(info->GetResourceType());? https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:80: // the actual clearing of data for |origin|. The datatypes to be deleted nit: data types https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:180: } DCHECK_GT(pending_task_count_, 0); https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:183: // BrowsingDataRemover::Observer; nit: ; -> : (At least I assume that was a typo) https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:278: return std::unique_ptr<ResourceThrottle>(); While I'm sure this isn't exactly a huge consumer of resources, could we just do this once, and stash its value in RDH? We do similar things elsewhere to avoid searching through the command line on a per-request basis. Some code even stashes values in globals to avoid it, but that seems icky. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:462: delegate->AddMessage(current_url, "Not a valid JSON.", "Not valid JSON." or just "Invalid JSON." or, to match the next message "Expecting valid JSON." https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:472: "Expecting a JSON dictionary with a 'types' field.", optional: I think "Expected" may be a little more common for error messages? https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:561: OutputConsoleMessages(); optional: Could make this into a utility function, since you duplicate it 3 times. Don't see the logic changing much, but you never know. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:89: std::move(callback).Run(); This violates the ResourceThrottle spec (Throttles can't resume synchronously), which seems a bit weird. I know the tests don't use a real ResourceThrottle::Delegate, but seems a bit odd. Anyhow, may be simplest to keep as is, just thought I'd bring it up. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:333: console_delegate->messages().rbegin()->level); EXPECT that the throttle's ClearSiteData isn't called? https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:384: } Again, EXPECT_CALLed / not calleds as appropriate? https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:563: bool navigation = kThrottleTypeIsNavigation[throttle_type]; This is a bit funky. throttle_type is not an index, it's false or true (True first, then false, since that's the order of the array). So throttle_type and navigation should be the same variable, and should just get rid of this line.
Forgot one nit. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:283: return std::unique_ptr<ResourceThrottle>(); These two returns can just be "return nullptr;"
Just one question on the browser test, and we're done at long last. :) https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:805: url = https_server()->GetURL("origin1.com", "/resource"); Why does requesting /resource delete the service worker? I'm not seeing where it gets the clear-site-data header?
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I addressed the latest round of comments, PTAL! (Also, no worries about delays - this is a complicated CL, so I don't expect daily turnover) https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:224: message.text); On 2017/06/05 22:39:36, mmenke wrote: > On 2017/06/05 21:05:50, mmenke wrote: > > On 2017/06/01 22:02:27, msramek (OOO until June 6) wrote: > > > On 2017/05/31 16:25:33, mmenke wrote: > > > > Can't remember if I raised this earlier - thought I did, but don't see the > > > > comment. If we have the same URL requested twice, this will merge their > log > > > > messages. Should it? > > > > > > > > Suppose we have a redirected request http://foo/clear-stuff -> > > > > http://foo/clear-stuff -> http://foo/do-not-stuff/ -> > http://foo/clear-stuff > > > > > > > > We'd only have one prefix, despite having 3 requests that clear stuff. > The > > > > simplest workaround would be to add a header as a message, each time we > see > > a > > > > clear-data header (Each time we see the header, we're going to output some > > > > message, anyways - either a claim we cleared something or an error). > > Unlikely > > > > to matter much, but seems trivial to change, and even makes the code a > > little > > > > simpler, as then we no longer need to track the URLs, right? > > > > > > The main reason for this |last_seen_url| was that a header can output more > > than > > > one line (e.g. it can complain that it doesn't recognize one datatype, but > > still > > > delete other ones - this is for forward compatibility). > > > > > > So we get: > > > > > > """ > > > Clear-Site-Data header on foo: blah blah blah > > > blah blah blah > > > """ > > > > > > My intention was to avoid spamming the prefix ("Clear-Site-Data header"), > but > > > admittedly, this does look a bit ugly. > > > > > > We could join messages from one origin into one multiline string, but then > we > > > lose the information about message level. > > > > > > So it might really be easiest to just add the prefix everywhere. Note that > > this > > > doesn't fully address your concern, since if we have three messages from two > > > origins, you can't tell if it's 2+1, 1+2 from the syntax (you probably can > > from > > > their meaning though). > > > > I'm not following. Couldn't we do: > > > > bool ClearSiteDataThrottle::HandleHeader() { > > if (/* header not present */) > > return false; > > > > delegate_->AddMessage("Processing Clear-Site-Data header from %s:"); > > ... > > (Note: I'm happy with the current approach) I played a bit with the console, and it doesn't look that good to have the "prefix" as a separate message, because we don't know which logging level to use. It could happen that "Processing..." will be INFO, but then the next line will be ERROR. The current approach will only output one line, which will be ERROR. For mixed cases (like the one with unrecognized type), we would output one line with ERROR, one with INFO, and here it's desirable that they both have the prefix. So I think the current solution works quite well. And again, we could do a seek-forward during the output to evaluate the following messages, decide whether the prefix should be LOG or ERROR, join them into one multiline message with offsets if possible, etc., but I don't want to overcomplicate it. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:50: const char kConsoleMessagePrefix[] = "Clear-Site-Data header on '%s': %s"; On 2017/06/05 22:39:36, mmenke wrote: > nit: Not actually a prefix Done. Renamed to "template". https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:62: info->GetResourceType() == RESOURCE_TYPE_SUB_FRAME); On 2017/06/05 22:39:36, mmenke wrote: > info && ResourceType::IsResourceTypeFrame(info->GetResourceType());? Done. Good to know :) https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:80: // the actual clearing of data for |origin|. The datatypes to be deleted On 2017/06/05 22:39:36, mmenke wrote: > nit: data types Done. Actually, we've been using "datatype" everywhere, and I always assumed it's interchangeable, but Google suggests only "data type" as the correct spelling. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:180: } On 2017/06/05 22:39:36, mmenke wrote: > DCHECK_GT(pending_task_count_, 0); Done. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:183: // BrowsingDataRemover::Observer; On 2017/06/05 22:39:36, mmenke wrote: > nit: ; -> : (At least I assume that was a typo) Done. Yes :) https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:278: return std::unique_ptr<ResourceThrottle>(); On 2017/06/05 22:39:36, mmenke wrote: > While I'm sure this isn't exactly a huge consumer of resources, could we just do > this once, and stash its value in RDH? We do similar things elsewhere to avoid > searching through the command line on a per-request basis. Some code even > stashes values in globals to avoid it, but that seems icky. Done. Sure, I was just trying to contain the knowledge of requirements for this throttle in one file. If you're fine with adding it to RDH, we can do that too. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:283: return std::unique_ptr<ResourceThrottle>(); On 2017/06/05 22:42:36, mmenke wrote: > These two returns can just be "return nullptr;" Done. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:462: delegate->AddMessage(current_url, "Not a valid JSON.", On 2017/06/05 22:39:36, mmenke wrote: > "Not valid JSON." or just "Invalid JSON." or, to match the next message > "Expecting valid JSON." Done. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:472: "Expecting a JSON dictionary with a 'types' field.", On 2017/06/05 22:39:36, mmenke wrote: > optional: I think "Expected" may be a little more common for error messages? Done. Yes, I just realized that when I was addressing the above comment :) https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:561: OutputConsoleMessages(); On 2017/06/05 22:39:36, mmenke wrote: > optional: Could make this into a utility function, since you duplicate it 3 > times. Don't see the logic changing much, but you never know. OutputConsoleMessages() is called 4 times actually, 2 times with one condition and 2 times with a slightly different one. I think handling the condition inside the function and parameterizing the two cases would decrease readability here. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:805: url = https_server()->GetURL("origin1.com", "/resource"); On 2017/06/06 16:16:09, mmenke wrote: > Why does requesting /resource delete the service worker? I'm not seeing where > it gets the clear-site-data header? This test conveniently reuses the infrastructure from the "ServiceWorker" test above. You can see that AddServiceWorker() installs "worker.js", the new JS file added in this CL. We then request /resource?header=... (the header is added on the next line). worker.js is instructed to let "/resource" through (it only handles "resource_from_sw". So the request falls back to the network, namely to the EmbeddedTestServer at the beginning of this file. That server recognizes the header and responds with Clear-Site-Data. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:866: shell()->CloseAllWindows(); I changed this to Close(), as this test was failing on Linux Aura. The problem was that CloseAllWindows() calls PlatformExit(), and the browsertest framework called it once more after the test finished, but shell_aura.cc expects it to only be called once. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:89: std::move(callback).Run(); On 2017/06/05 22:39:36, mmenke wrote: > This violates the ResourceThrottle spec (Throttles can't resume synchronously), > which seems a bit weird. I know the tests don't use a real > ResourceThrottle::Delegate, but seems a bit odd. Anyhow, may be simplest to > keep as is, just thought I'd bring it up. Acknowledged. I documented this fact, but didn't fix it, as that would require us to wait for the asynchronous call in many places. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:333: console_delegate->messages().rbegin()->level); On 2017/06/05 22:39:36, mmenke wrote: > EXPECT that the throttle's ClearSiteData isn't called? Done. Also added the positive call expectation above. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:384: } On 2017/06/05 22:39:36, mmenke wrote: > Again, EXPECT_CALLed / not calleds as appropriate? Done. https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:563: bool navigation = kThrottleTypeIsNavigation[throttle_type]; On 2017/06/05 22:39:37, mmenke wrote: > This is a bit funky. throttle_type is not an index, it's false or true (True > first, then false, since that's the order of the array). So throttle_type and > navigation should be the same variable, and should just get rid of this line. But it works, doesn't it? :-D Yeah, I was trying to avoid writing something like for (bool navigation = false; navigation <= true; navigation++) and mixed it up somehow. Done.
LGTM! https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2368923003/diff/460001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:805: url = https_server()->GetURL("origin1.com", "/resource"); On 2017/06/06 18:20:57, msramek (slow) wrote: > On 2017/06/06 16:16:09, mmenke wrote: > > Why does requesting /resource delete the service worker? I'm not seeing where > > it gets the clear-site-data header? > > This test conveniently reuses the infrastructure from the "ServiceWorker" test > above. You can see that AddServiceWorker() installs "worker.js", the new JS file > added in this CL. > > We then request /resource?header=... (the header is added on the next line). > > worker.js is instructed to let "/resource" through (it only handles > "resource_from_sw". So the request falls back to the network, namely to the > EmbeddedTestServer at the beginning of this file. That server recognizes the > header and responds with Clear-Site-Data. Oops, completely missed the header part on the next line.
msramek@chromium.org changed reviewers: + mmoroz@chromium.org
Thanks! \o/
msramek@chromium.org changed reviewers: + jam@chromium.org
+Max, +John, please have a look as well. Max: Changes in the fuzzer. The tested method is now static, so we don't need the class which used to hold the throttle. John: Changes in the content/public interface. - MockBrowsingDataRemoverDelegate was moved there from chrome/browser. Note that the base class BrowsingDataRemoverDelegate is already in content/public. This is needed in content_browsertests. - Removed the now no longer needed interface in ContentBrowserClient. - Removed the throttle from NavigationHandleImpl (as it now lives in ResourceDispatcherImpl) Thanks, Martin
LGTM for fuzzer changes https://codereview.chromium.org/2368923003/diff/480001/content/test/fuzzer/cl... File content/test/fuzzer/clear_site_data_fuzzer.cc (right): https://codereview.chromium.org/2368923003/diff/480001/content/test/fuzzer/cl... content/test/fuzzer/clear_site_data_fuzzer.cc:7: #include <vector> nit: #include <string> instead of <vector>
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2368923003/diff/480001/content/public/test/mo... File content/public/test/mock_browsing_data_remover_delegate.h (right): https://codereview.chromium.org/2368923003/diff/480001/content/public/test/mo... content/public/test/mock_browsing_data_remover_delegate.h:19: : public content::BrowsingDataRemoverDelegate { nit: this whole file remove "content::", also in cc file
Thanks everyone! Landing this now :) https://codereview.chromium.org/2368923003/diff/480001/content/public/test/mo... File content/public/test/mock_browsing_data_remover_delegate.h (right): https://codereview.chromium.org/2368923003/diff/480001/content/public/test/mo... content/public/test/mock_browsing_data_remover_delegate.h:19: : public content::BrowsingDataRemoverDelegate { On 2017/06/07 01:19:16, jam wrote: > nit: this whole file remove "content::", also in cc file Done. https://codereview.chromium.org/2368923003/diff/480001/content/test/fuzzer/cl... File content/test/fuzzer/clear_site_data_fuzzer.cc (right): https://codereview.chromium.org/2368923003/diff/480001/content/test/fuzzer/cl... content/test/fuzzer/clear_site_data_fuzzer.cc:7: #include <vector> On 2017/06/06 18:53:43, mmoroz wrote: > nit: #include <string> instead of <vector> Done.
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org, mkwst@chromium.org, mmoroz@chromium.org, jam@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2368923003/#ps500001 (title: "Addressed comments, formatted.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 500001, "attempt_start_ts": 1496829234042710,
"parent_rev": "f31a9f767ad6e5a7be6ba96c920230f2e1e34864", "commit_rev":
"9bc8902cf5825d5cbcdbf2b7766241f2f9c96bbb"}
Message was sent while issue was closed.
Description was changed from ========== Support the Clear-Site-Data header on resource requests Until now, it was only supported for navigations. Changes in this CL: 1. Convert the NavigationThrottle to a ResourceThrottle. The two classes are sufficiently similar that these are mostly syntactical changes, except that the ResourceThrottle lives on the IO thread and needs to occasionally jump to the UI thread. 2. Instantiate it in ResourceDispatcherHostImpl instead of NavigationHandleImpl. This requires adding an explicit DEPS rule. 3. Add some restrictions - for example, we will not support service worker requests or LOAD_DO_NOT_SET_COOKIES. These are then tested in the unittest. 4. Add browsertests for resource requests, and some integration tests that check not only calls to BrowsingDataRemover, but also the actual removal of data. BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Support the Clear-Site-Data header on resource requests Until now, it was only supported for navigations. Changes in this CL: 1. Convert the NavigationThrottle to a ResourceThrottle. The two classes are sufficiently similar that these are mostly syntactical changes, except that the ResourceThrottle lives on the IO thread and needs to occasionally jump to the UI thread. 2. Instantiate it in ResourceDispatcherHostImpl instead of NavigationHandleImpl. This requires adding an explicit DEPS rule. 3. Add some restrictions - for example, we will not support service worker requests or LOAD_DO_NOT_SET_COOKIES. These are then tested in the unittest. 4. Add browsertests for resource requests, and some integration tests that check not only calls to BrowsingDataRemover, but also the actual removal of data. BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2368923003 Cr-Commit-Position: refs/heads/master@{#477612} Committed: https://chromium.googlesource.com/chromium/src/+/9bc8902cf5825d5cbcdbf2b77662... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:500001) as https://chromium.googlesource.com/chromium/src/+/9bc8902cf5825d5cbcdbf2b77662... |
