Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2025683003:
First experimental implementation of the Clear-Site-Data header (Closed)
DescriptionFirst experimental implementation of the Clear-Site-Data header
We add a ClearSiteDataThrottle to the list of ResourceThrottle-s
on a main frame navigation. If the response contains a "Clear-Site-Data"
header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover
to execute deletion with the specified parameters.
The deletion is asynchronous and does not delay or block the navigation.
This is an early proof-of-concept implementation of the working draft
https://www.w3.org/TR/clear-site-data/ which does not yet comply with
its full specification.
Tests:
- ClearSiteDataThrottleUnittest
- parsing
- error messages
- ClearSiteDataThrottleBrowsertest
- secure origins
- redirects,
- integration with the embedder
- ChromeContentBrowserClientTest
- removal parameters for BrowsingDataRemover
Note that the effect of data being asynchronously deleted while possibly
also written by the website in the same time has NOT yet been tested.
BUG=607897
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/1f4746d285ff62f0747ef4ce00c72a74f91f92cb
Cr-Commit-Position: refs/heads/master@{#413247}
Patch Set 1 #
Total comments: 29
Patch Set 2 : Moved to content/ #Patch Set 3 : Rebase and fix ListValue iteration. #
Total comments: 30
Patch Set 4 : Addressed comments, added browsertest. #Patch Set 5 : Rebase #Patch Set 6 : Compilation fixes. #
Total comments: 2
Patch Set 7 : Comment, file URLs #
Total comments: 26
Patch Set 8 : Rebase over https://codereview.chromium.org/2097083002/ #Patch Set 9 : Rebase fix (https://codereview.chromium.org/2084903002/) #Patch Set 10 : Addressed comments. #
Total comments: 4
Patch Set 11 : Fixed tests, extracted c/b/browsing_data #
Total comments: 4
Patch Set 12 : Rebased. #Patch Set 13 : Moved channel IDs under 'cookies' #
Total comments: 10
Patch Set 14 : Moved the throttle to the end, added TODO #Patch Set 15 : Don't spam the console. #Patch Set 16 : Null response headers #Patch Set 17 : WebContentsObserver #
Total comments: 4
Patch Set 18 : Rebase #Patch Set 19 : ClearSiteDataHeaderObserver (no longer a throttle) #Patch Set 20 : Fix URL in comments, remove accidental change. #
Total comments: 33
Patch Set 21 : Addressed comments, git cl format #
Total comments: 1
Patch Set 22 : Rebase #Patch Set 23 : Rebase, custom mock in ChromeContentBrowserClient #Patch Set 24 : Many changes, most importantly synchronous deletion. #
Total comments: 55
Patch Set 25 : Addressed comments. #
Total comments: 2
Patch Set 26 : Rebase #Patch Set 27 : Addressed comments. #Patch Set 28 : REMOVE_PLUGIN_DATA for eTLD+1 #Messages
Total messages: 163 (99 generated)
msramek@chromium.org changed reviewers: + mkwst@chromium.org
Hi Mike, Please have a look! This is work in progress, but before I get too deep into it, I'd like to hear your feedback. I'll also have to revive the discussion in blink-dev before proceeding, as it seemed to me that the reaction was sort of lukewarm... but in the meantime, here's a prototype :) Thanks, Martin
On 2016/05/30 at 19:14:33, msramek wrote: > Please have a look! This is work in progress, but before I get too deep into it, I'd like to hear your feedback. I left some comments, let's chat! > I'll also have to revive the discussion in blink-dev before proceeding, as it seemed to me that the reaction was sort of lukewarm... but in the meantime, here's a prototype :) Sure, but that shouldn't be a blocker on the implementation. Thanks!
https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:47: domain = GetDomainAndRegistry( This is going to strip `www.example.com` down to `example.com`, isn't it? That doesn't seem correct, if we want to be able to target subdomains (like `plus.google.com` for clearing). https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:52: if (domain.empty()) Another implication is that folks on an intranet wouldn't be able to use this for their internal names; `https://fileserver/` or whatever should be clearable. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:85: safe_json::SafeJsonParser::Parse( Why do we need to parse JSON on the UI thread? Can we do this in `WillProcessResponse` as well? https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:100: DVLOG(1) << "Not a valid dictionary value."; // TODO(you): Write a console error. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:107: DVLOG(1) << "No 'types' field present."; // TODO(you): Write a console error. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:112: for (const base::Value* value : *types) { Nit: These are `unique_ptr` aren't they? https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:115: DVLOG(1) << "Invalid type: " << *value; // TODO(you): Write a console error. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:126: remove_mask |= BrowsingDataRemover::REMOVE_SITE_DATA; This includes cookies; it shouldn't. I think you'll probably need to do something like `remove_mask |= BrowsingDataRemover::REMOVE_SITE_DATA ^ BrowsingDataRemover::REMOVE_COOKIES` to exclude them (or define a new constant). https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:132: DVLOG(2) << "No valid types specified for clearing."; // TODO(you): Write a console error. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:141: filter_builder.AddRegisterableDomain(domain); Again, we should talk about the registerable domain bits. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:143: browsing_data_remover->RemoveWithFilter( This eventually hops back to the IO thread, right? Can you add a comment to that effect? https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:153: DVLOG(1) << error; // TODO(you): write a console error. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2025683003/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:20: #include "chrome/browser/browsing_data/clear_site_data_throttle.h" All the throttle machinery should be in //content, not //chrome (because this is a ~standardized, web-facing API). See `//content/browser/frame_host/navigation_handle_impl.cc` in https://codereview.chromium.org/1617043002/ for an example of what that might look like. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:623: // Experimental Clear-Site-Data throttle. Create only for the main frame. This is fine to start with, but an eventual goal should probably be to support various kinds of requests, not just navigations. See https://w3c.github.io/webappsec-clear-site-data/#example-targeted, for example. We should either decide not to support that use case, or come up with a plan for eventually adding it in.
Description was changed from ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the resource (main frame) load. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. *** WORK IN PROGRESS *** - Successfully tested on a small testing server; the header is parsed and BrowsingDataRemover::RemoveImpl is executed. - The effect of data being asynchronously deleted while possibly also written by the website in the same time NOT yet tested. - Unittest NOT yet finished. BUG=607897 ========== to ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the resource (main frame) load. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. *** WORK IN PROGRESS *** - Successfully tested on a small testing server; the header is parsed and BrowsingDataRemover::RemoveImpl is executed. - The effect of data being asynchronously deleted while possibly also written by the website in the same time NOT yet tested. - Unittest NOT yet finished. BUG=607897 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Thanks Mike! I moved everything to //content; it actually also made the implementation a bit simpler. I also added tests for header parsing. As the next step, I'll have to test whether we're deleting data for the correct origin in case of redirects and cancelled navigation. In ChromeContentBrowserClient::ClearSiteData, I described how I imagine origin- and eTLD+1- scoped deletion. But we can chat about this tomorrow if you disagree :) I'll also want to discuss RegistrableDomainFilterBuilder with you and dmurph@ again. I also noticed that you ran into the same problems with GetRenderFrameHost() in https://codereview.chromium.org/1617043002. As I wasn't sure whether it's correct to weaken the precondition from READY_TO_COMMIT to WILL_PROCESS_RESPONSE (you ended up reverting after all), I solved it differently (by waiting for the navigation to finish before writing anything to console). PTAL! :) https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:47: domain = GetDomainAndRegistry( On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > This is going to strip `www.example.com` down to `example.com`, isn't it? That > doesn't seem correct, if we want to be able to target subdomains (like > `plus.google.com` for clearing). Done. Sorry, forgot to add a comment about this. I'm of course going to use OriginFilterBuilder, just wanted to start out with RegisterableDomainFilterBuilder since it supports more datatypes. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:52: if (domain.empty()) On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > Another implication is that folks on an intranet wouldn't be able to use this > for their internal names; `https://fileserver/` or whatever should be clearable. Yes, or even a simpler example: localhost. I think this is an oversight in the design of RegisterableDomainFilterBuilder. I'll start a discussion about this in an email. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:85: safe_json::SafeJsonParser::Parse( On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > Why do we need to parse JSON on the UI thread? Can we do this in > `WillProcessResponse` as well? N/A, since after moving to content/ were using the simpler JSONReader. Anyway, SafeJSONParser has its own thread anyway, so it shouldn't matter where it's initiated from? I put it here to avoid having two asynchronous calls at the same time to wait for. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:100: DVLOG(1) << "Not a valid dictionary value."; On 2016/05/31 05:35:52, Mike West (OOO until 30th) wrote: > // TODO(you): Write a console error. Done. (actually implemented the console error) https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:107: DVLOG(1) << "No 'types' field present."; On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > // TODO(you): Write a console error. Done. (actually implemented the console error) https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:112: for (const base::Value* value : *types) { On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > Nit: These are `unique_ptr` aren't they? types->begin() is std::vector<base::Value*>::const_iterator, although I have currently no idea why. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:115: DVLOG(1) << "Invalid type: " << *value; On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > // TODO(you): Write a console error. Done. (actually implemented the console error) https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:126: remove_mask |= BrowsingDataRemover::REMOVE_SITE_DATA; On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > This includes cookies; it shouldn't. I think you'll probably need to do > something like `remove_mask |= BrowsingDataRemover::REMOVE_SITE_DATA ^ > BrowsingDataRemover::REMOVE_COOKIES` to exclude them (or define a new constant). Done. Thanks for paying attention to this :) https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:132: DVLOG(2) << "No valid types specified for clearing."; On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > // TODO(you): Write a console error. Done. (actually implemented the console error) https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:141: filter_builder.AddRegisterableDomain(domain); On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > Again, we should talk about the registerable domain bits. Please see my current implementation. I'm using origin-scoped deletion for every data type that supports it. I'm using eTLD+1-scoped deletion for cookies and channel IDs, in both cases it was an explicit request during the code review not to support origins. But as I mentioned earlier, I will send out an email to discuss some details. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:143: browsing_data_remover->RemoveWithFilter( On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > This eventually hops back to the IO thread, right? Can you add a comment to that > effect? N/A, since we're not doing any more thread hopping after moving to content/. https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:153: DVLOG(1) << error; On 2016/05/31 05:35:52, Mike West (OOO until 30th) wrote: > // TODO(you): write a console error. Done. (actually implemented the console error) https://codereview.chromium.org/2025683003/diff/1/chrome/browser/renderer_hos... File chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2025683003/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:20: #include "chrome/browser/browsing_data/clear_site_data_throttle.h" On 2016/05/31 05:35:52, Mike West (OOO until 30th) wrote: > All the throttle machinery should be in //content, not //chrome (because this is > a ~standardized, web-facing API). See > `//content/browser/frame_host/navigation_handle_impl.cc` in > https://codereview.chromium.org/1617043002/ for an example of what that might > look like. Done. Moved to content. It actually made some things easier (no more jumping around threads needed now). https://codereview.chromium.org/2025683003/diff/1/chrome/browser/renderer_hos... chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc:623: // Experimental Clear-Site-Data throttle. Create only for the main frame. On 2016/05/31 05:35:52, Mike West (OOO until 30th) wrote: > This is fine to start with, but an eventual goal should probably be to support > various kinds of requests, not just navigations. See > https://w3c.github.io/webappsec-clear-site-data/#example-targeted, for example. > We should either decide not to support that use case, or come up with a plan for > eventually adding it in. I removed the condition for now. My intention was not to contradict the spec, but to limit the impact in the early experimentation stage.
https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... File chrome/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/1/chrome/browser/browsing_dat... chrome/browser/browsing_data/clear_site_data_throttle.cc:112: for (const base::Value* value : *types) { On 2016/06/01 14:20:40, msramek wrote: > On 2016/05/31 05:35:51, Mike West (OOO until 30th) wrote: > > Nit: These are `unique_ptr` aren't they? > > types->begin() is std::vector<base::Value*>::const_iterator, although I have > currently no idea why. Ah. Because this is a recent change and I have an old checkout (with base::Value*). I was reading the code in code search and could not comprehend where is the pointer unwrapped. Will rebase and fix the compilation error shortly...
On 2016/06/01 at 14:20:41, msramek wrote: > I also noticed that you ran into the same problems with GetRenderFrameHost() in https://codereview.chromium.org/1617043002. As I wasn't sure whether it's correct to weaken the precondition from READY_TO_COMMIT to WILL_PROCESS_RESPONSE (you ended up reverting after all), I solved it differently (by waiting for the navigation to finish before writing anything to console). I didn't revert because of that change; I reverted because of the ordering issues that clamy@ is addressing in https://codereview.chromium.org/2005273002. But the way you've ended up with is pretty reasonable too (though as you note above, I'm not sure it works in the case of redirects).
On 2016/06/01 at 14:20:41, msramek wrote: > I also noticed that you ran into the same problems with GetRenderFrameHost() in https://codereview.chromium.org/1617043002. As I wasn't sure whether it's correct to weaken the precondition from READY_TO_COMMIT to WILL_PROCESS_RESPONSE (you ended up reverting after all), I solved it differently (by waiting for the navigation to finish before writing anything to console). I didn't revert because of that change; I reverted because of the ordering issues that clamy@ is addressing in https://codereview.chromium.org/2005273002. But the way you've ended up with is pretty reasonable too (though as you note above, I'm not sure it works in the case of redirects).
https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2507: if (remove_storage || remove_cache) { This should be `remove_cookies`, right? Actually, I don't understand the logic here. Why do you check for cookies and storage inside this `if`? What do you intend for this block to do, and what do you intend the next to take care of? https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2520: remove_mask |= BrowsingDataRemover::REMOVE_CHANNEL_IDS; 1. Can you file a bug against the spec (or send a PR? :) ) to note that we treat channel ID/token binding like cookies? 2. Why don't you actually treat them like cookies here? Why tie them to storage and not cookies? https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2526: remover->RemoveWithFilter( If folks want to clear both cookies and DOM storage, we're going to end up calling into BDR twice. That's hugely expensive... https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2530: BrowsingDataHelper::UNPROTECTED_WEB, Hrm. I forget what we actually used PROTECTED_WEB for now; have we killed off hosted apps? If not, how does this interact with them? https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:61: static_cast<NavigationHandleImpl*>(navigation_handle()); We should skip all of this if the URL is insecure. https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:65: handle->GetResponseHeaders()->GetNormalizedHeader( This will return a comma separated list if more than one header is present in the response. That is: Clear-Site-Data: a Clear-Site-Data: b will be normalized to: Clear-Site-Data: a,b I don't think your parser handles this? Or maybe it does. Would you mind adding tests in any event? https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:99: ConsoleLog("The value is not a valid JSON.", true /* error */); Can you output the header value so that it's clear what the message is referring to (ditto for the other errors). https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:125: ConsoleLog("Clearing cookies.", false /* error */); Can you do the extra work to combine these into one "Clearing X, Y, and Z" message? https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:148: void ClearSiteDataThrottle::ConsoleLog(const std::string& text, bool error) { Sprinkling `/* error */` throughout the code is pretty ugly. Could you make this an enum instead? Or just use `CONSOLE_MESSAGE_LEVEL_{ERROR,LOG}` outright? https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.h:24: // It does not block navigation. Aside: I think we might need to reconsider this once we see how it works in the wild. I suspect the race conditions will be super-annoying, and that it might be preferable to delay the navigation. https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:57: // One data type. I think this would be simpler if you did something like: ``` struct TestCase { const char* header; bool cookies; bool storage; bool cache; } cases[] = { {"{\"types\": ... ", true, false, false}, }; for (const auto& test : cases) { SCOPED_TRACE(test.header); EXPECT_TRUE(GetThrottle()->ParseHeader(test.header,&cookies,&storage,&cache)); EXPECT_EQ(test.cookies, cookies); ... } ``` https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:88: InvalidParseHeaderTestCase("{ \"types\" : [ \"no_valid_types\" ] }"); Can you add checks for the console messages as well? At least examining the `messages_` vector seems like a reasonable thing to do. https://codereview.chromium.org/2025683003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2025683003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:311: throttles_.push_back(std::move(clear_site_data_throttle)); Can you tie the creation of this throttle to the `experimental-web-platform-features` flag? I don't think we're ready to ship this yet. :)
mkwst@chromium.org changed reviewers: + bratell@opera.com
+bratell@opera for a question about how y'all implement the BrowsingDataRemove type of functionality. :) https://codereview.chromium.org/2025683003/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2025683003/diff/40001/content/public/browser/... content/public/browser/content_browser_client.h:563: virtual void ClearSiteData( Daniel, is this a structure that Opera would be able to implement as well? It's not clear to me how much of //chrome y'all use, so I don't really know how best to make room for you to eventually implement this call into //chrome/browser/browsing_data.
mkwst@chromium.org changed reviewers: + sigbjornf@opera.com
+sof as well, for good measure. :)
https://codereview.chromium.org/2025683003/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2025683003/diff/40001/content/public/browser/... content/public/browser/content_browser_client.h:563: virtual void ClearSiteData( On 2016/06/02 07:05:44, Mike West (OOO until 30th) wrote: > Daniel, is this a structure that Opera would be able to implement as well? It's > not clear to me how much of //chrome y'all use, so I don't really know how best > to make room for you to eventually implement this call into > //chrome/browser/browsing_data. I did a quick check and it seems this should be fine for us. Thanks for asking. The risk would be that it slips past without anyone noticing. As for the usage of //chrome, I think the idea is "preferably not" but with enough exceptions to make the long answer complicated. :-)
Ping?
bratell@: Thanks for taking a look! Martin, can you remember to make sure that we CC Opera folks on the Intent to Ship if/when we get to the point of shipping this feature? :)
Patchset #4 (id:60001) has been deleted
Patchset #7 (id:140001) has been deleted
Patchset #6 (id:120001) has been deleted
Hi again Mike! Sorry for the delays, I was OOO most of the time. I have addressed your comments and made additional changes, so it might be actually easier to review from start than just the diff. Additional changes made: - Added a thorough browsertest to test redirects, http vs https, and whether we call the correct method after parsing the header. - Removed the WebContentsObserver, as outputting the console logs on DidFinishNavigation() did not work properly anyway. I'm outputting them on WillProcessResponse() now. By coincidence, Tanja has been investigating this recently and assured me it's safe to relax the condition in NavigationHandleImpl. - Now processing all three NavigationThrottle events, otherwise I wouldn't handle redirects correctly. - Improved the unittests. TODO: - Stop by your desk soon to discuss the spec again. - Support hosts such that don't have a valid registrable domain, such as localhost (will probably need changes to RegistrableDomainFilterBuilder, so in a different CL). PTAL! Thanks, Martin https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2507: if (remove_storage || remove_cache) { On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > This should be `remove_cookies`, right? > > Actually, I don't understand the logic here. Why do you check for cookies and > storage inside this `if`? What do you intend for this block to do, and what do > you intend the next to take care of? Sorry, I've been rewriting this a few times, apparently I messed up the condition again. This block is for eTLD+1-scoped deletion. We do that for cookies (which are included in remove_cookies) and for channel IDs (which are part of storage), so the condition should be (remove_cookies || remove_storage). But we need to check again inside the block, because it could be only one of them. https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2520: remove_mask |= BrowsingDataRemover::REMOVE_CHANNEL_IDS; On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > 1. Can you file a bug against the spec (or send a PR? :) ) to note that we > treat channel ID/token binding like cookies? > 2. Why don't you actually treat them like cookies here? Why tie them to storage > and not cookies? BrowsingDataRemover treats them as a part of site data, so I think they fit more to the storage. If we're going to tie them to cookies, I would prefer to do that in our backend as well. I'll have to find out if it makes sense at first though. https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2526: remover->RemoveWithFilter( On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > If folks want to clear both cookies and DOM storage, we're going to end up > calling into BDR twice. That's hugely expensive... Not necessarily. BDR is full of if()-s that are only executed if the removal mask contains given datatypes. So we wont end up doing more deletion tasks, or more thread-jumping etc., just more overhead with the method calls and initializing the filters which shouldn't be that bad? https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2530: BrowsingDataHelper::UNPROTECTED_WEB, On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > Hrm. I forget what we actually used PROTECTED_WEB for now; have we killed off > hosted apps? If not, how does this interact with them? Heh, good point. In my mind, I am still mapping this to the checkboxes "cookies and site data" and "cache". The former deletes only UNPROTECTED_WEB. The "hosted apps" checkbox then does the same for PROTECTED_WEB. But the header has no reason to respect this distinction. If a website requests clearing, then we should simply clear the data, period. https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:61: static_cast<NavigationHandleImpl*>(navigation_handle()); On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > We should skip all of this if the URL is insecure. Done. https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:65: handle->GetResponseHeaders()->GetNormalizedHeader( On 2016/06/02 07:00:08, Mike West wrote: > This will return a comma separated list if more than one header is present in > the response. That is: > > Clear-Site-Data: a > Clear-Site-Data: b > > will be normalized to: > > Clear-Site-Data: a,b > > I don't think your parser handles this? Or maybe it does. Would you mind adding > tests in any event? EnumerateHeader() will cut the JSON on every comma, which I don't want. GetNormalizedHeader() will merge all lines into one, which I also don't want. Well, I could cut it back into a vector of valid JSONs manually with a push-down automaton, if you think we should support that. But it's easiest to declare that we want one valid JSON per line, and use EnumerateHeaderLines(). https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:99: ConsoleLog("The value is not a valid JSON.", true /* error */); On 2016/06/02 07:00:08, Mike West wrote: > Can you output the header value so that it's clear what the message is referring > to (ditto for the other errors). Done. https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:125: ConsoleLog("Clearing cookies.", false /* error */); On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > Can you do the extra work to combine these into one "Clearing X, Y, and Z" > message? Done. https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.cc:148: void ClearSiteDataThrottle::ConsoleLog(const std::string& text, bool error) { On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > Sprinkling `/* error */` throughout the code is pretty ugly. Could you make this > an enum instead? Or just use `CONSOLE_MESSAGE_LEVEL_{ERROR,LOG}` outright? Done. https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle.h:24: // It does not block navigation. On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > Aside: I think we might need to reconsider this once we see how it works in the > wild. I suspect the race conditions will be super-annoying, and that it might be > preferable to delay the navigation. Agreed. I have no idea right now which one is the right thing to do, so I would try to gather a few real-life customers first and judge by their experience. We should definitely start with the non-blocking version though, as the blocking one might need some UI to explain why the page is unresponsive (I heard we do that with the webRequest API?). https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:57: // One data type. On 2016/06/02 07:00:08, Mike West wrote: > I think this would be simpler if you did something like: > > ``` > struct TestCase { > const char* header; > bool cookies; > bool storage; > bool cache; > } cases[] = { > {"{\"types\": ... ", true, false, false}, > }; > > for (const auto& test : cases) { > SCOPED_TRACE(test.header); > > EXPECT_TRUE(GetThrottle()->ParseHeader(test.header,&cookies,&storage,&cache)); > EXPECT_EQ(test.cookies, cookies); > ... > } > ``` Done. https://codereview.chromium.org/2025683003/diff/40001/content/browser/browsin... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:88: InvalidParseHeaderTestCase("{ \"types\" : [ \"no_valid_types\" ] }"); On 2016/06/02 07:00:08, Mike West wrote: > Can you add checks for the console messages as well? At least examining the > `messages_` vector seems like a reasonable thing to do. Done. https://codereview.chromium.org/2025683003/diff/40001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2025683003/diff/40001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:311: throttles_.push_back(std::move(clear_site_data_throttle)); On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > Can you tie the creation of this throttle to the > `experimental-web-platform-features` flag? I don't think we're ready to ship > this yet. :) Done. https://codereview.chromium.org/2025683003/diff/40001/content/public/browser/... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2025683003/diff/40001/content/public/browser/... content/public/browser/content_browser_client.h:563: virtual void ClearSiteData( On 2016/06/02 09:15:46, Daniel Bratell wrote: > On 2016/06/02 07:05:44, Mike West (OOO until 30th) wrote: > > Daniel, is this a structure that Opera would be able to implement as well? > It's > > not clear to me how much of //chrome y'all use, so I don't really know how > best > > to make room for you to eventually implement this call into > > //chrome/browser/browsing_data. > > I did a quick check and it seems this should be fine for us. Thanks for asking. > > The risk would be that it slips past without anyone noticing. > > As for the usage of //chrome, I think the idea is "preferably not" but with > enough exceptions to make the long answer complicated. :-) Acknowledged.
msramek@chromium.org changed reviewers: + melandory@chromium.org
And yes, I'll keep Opera folks in loop (and thanks for looking! :) ). Also, actually adding Tanja to sanity-check the change in NavigationHandleImpl::GetRenderFrameHost() as discussed above. PTAL!
https://codereview.chromium.org/2025683003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2025683003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:194: CHECK(state_ >= WILL_PROCESS_RESPONSE) also fix comment in navigation_handle.h (see here https://codereview.chromium.org/2060313002/)
I have updated the comment, and also fixed the failing tests. They were failing on "file:///" url navigations, since there NavigationHandleImpl::GetResponseHeaders() is uninitialized. https://codereview.chromium.org/2025683003/diff/160001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2025683003/diff/160001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:194: CHECK(state_ >= WILL_PROCESS_RESPONSE) On 2016/06/15 08:12:34, melandory wrote: > also fix comment in navigation_handle.h (see here > https://codereview.chromium.org/2060313002/) Done. I have updated the comment, but I didn't exactly follow the example of your CL, since it seems to still be in discussion. I will update it once it's resolved, or rebase on your CL if you land first.
Hey Martin! I'm at BlinkOn the rest of the week; I'll try to get to this soon, but it might be next week. :/ CCing jsbell, who's also interested in this feature (but who's also at BlinkOn).
Thanks for taking another pass. I've added some comments; mostly nits and testing questions. Getting closer! https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2025683003/diff/40001/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:2520: remove_mask |= BrowsingDataRemover::REMOVE_CHANNEL_IDS; On 2016/06/14 at 20:12:07, msramek wrote: > On 2016/06/02 07:00:08, Mike West (OOO until 30th) wrote: > > 1. Can you file a bug against the spec (or send a PR? :) ) to note that we > > treat channel ID/token binding like cookies? > > 2. Why don't you actually treat them like cookies here? Why tie them to storage > > and not cookies? > > BrowsingDataRemover treats them as a part of site data Cookies are part of site data too. :) > If we're going to tie them to cookies I think we need to tie them to cookies, as that's how they're used. For instance, Google uses channel ID to bind cookies to an origin (see http://www.browserauth.net/channel-bound-cookies for an old, but relevant description). If we cleared the ID, we'd also invalidate the cookies. My understanding is that this is pretty much the entire point of channel ID. > I would prefer to do that in our backend as well. I'll have to find out if it makes sense at first though. I think that's worth doing. https://codereview.chromium.org/2025683003/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2025683003/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2458: if (remove_cookies || remove_storage) { Nit: I think this might end up simpler to understand if you drop this outer if, and do something like: ``` std::string domain = ...; int remove_mask = 0; if (remove_cookies) remove_mask |= ...; if (remove_storage) remove_mask |= ...; if (remove_mask && !domain.empty()) { RegistrableDomainFilterBuilder(...); ... } ``` https://codereview.chromium.org/2025683003/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2465: return; Returning here is probably wrong. We can still clear the origin data below even if we can't get a registry here. https://codereview.chromium.org/2025683003/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2500: remover->RemoveWithFilter( It would be excellent to add some tests which verify that this method is being called with the expected arguments. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:35: static const char* kClearingThreeTypes = "Clearing %s, %s and %s."; Nit: Oxford comma, plz. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:148: } It doesn't seem worthwhile to distinguish between this error case and the following; I'd suggest just dropping this `if`, and changing the error message on line 155 to something like "%s is not a JavaScript object with a 'types' field." https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:180: serializer.Serialize(*value); Nit: It doesn't look like you use |serialized_type| or |serializer| for anything. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:187: // Each data type should only be Nit: Should only be... ? https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:188: DCHECK(datatype != nullptr); Nit: `DCHECK(datatype)` https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:222: ConsoleLog(messages, output, CONSOLE_MESSAGE_LEVEL_LOG); Nit: Would you mind pulling this out into a static helper in the anonymous namespace above? https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:55: // Run three HTTPS servers, each serving 127.0.0.1 on a different port. Please add some tests that test the registrable domain bits and pieces (`host_resolver()->AddRule("*", "127.0.0.1");`). https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:86: // middle, or end of the chain sends the header. What about when two (or all three) send the header? I vaguely remember blocking simultaneous clearing from the extension API. Do we need to worry about that here? https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:41: {"{ \"types\": [\"cookies\", \"cache\"] }", true, false, true}, You might as well do all of the options. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:44: {"{ \"types\": [\"storage\", \"cache\", \"cookies\"] }", true, true, true}, Might as well add a check that the order doesn't matter.
Ping?
On 2016/06/24 at 09:25:56, Mike West (OOO through 4th) wrote: > Ping? ^^ :)
On 2016/06/29 05:59:28, Mike West (OOO through 4th) wrote: > On 2016/06/24 at 09:25:56, Mike West (OOO through 4th) wrote: > > Ping? > > ^^ :) I'm getting there! Sorry for the delay :) I've just been playing whack-a-mole with "why I am working on this CL when I should be working on that CL" lately...
Patchset #8 (id:200001) has been deleted
On 2016/06/29 at 08:51:30, msramek wrote: > On 2016/06/29 05:59:28, Mike West (OOO through 4th) wrote: > > On 2016/06/24 at 09:25:56, Mike West (OOO through 4th) wrote: > > > Ping? > > > > ^^ :) > > I'm getting there! Sorry for the delay :) > > I've just been playing whack-a-mole with "why I am working on this CL when I should be working on that CL" lately... So. How are the moles?
Patchset #10 (id:260001) has been deleted
Patchset #10 (id:280001) has been deleted
All whacked! :) But it took me some time to get the tests right. - I pretty much rewrote ClearSiteDataBrowsertest, but it should be better readable now and with better coverage. - I added tests to ChromeContentClientUnittest; this was difficult, because BrowsingDataRemover wasn't mockable and BrowsingDataFilterBuilder-s weren't comparable to each other. The test coverage is good, but I'll think about whether this test could be written more aesthetically. In any case, please take another look! https://codereview.chromium.org/2025683003/diff/180001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2025683003/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2458: if (remove_cookies || remove_storage) { On 2016/06/20 07:57:37, Mike West (OOO through 4th) wrote: > Nit: I think this might end up simpler to understand if you drop this outer if, > and do something like: > > ``` > std::string domain = ...; > int remove_mask = 0; > if (remove_cookies) > remove_mask |= ...; > if (remove_storage) > remove_mask |= ...; > if (remove_mask && !domain.empty()) { > RegistrableDomainFilterBuilder(...); > ... > } > ``` Done. https://codereview.chromium.org/2025683003/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2465: return; On 2016/06/20 07:57:37, Mike West (OOO through 4th) wrote: > Returning here is probably wrong. We can still clear the origin data below even > if we can't get a registry here. Good catch :) Fortunately, this is not needed anymore as we're handling localhost et al. correctly after https://codereview.chromium.org/2127603002/ . https://codereview.chromium.org/2025683003/diff/180001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2500: remover->RemoveWithFilter( On 2016/06/20 07:57:37, Mike West (OOO through 4th) wrote: > It would be excellent to add some tests which verify that this method is being > called with the expected arguments. I was going to say that two layers of testing should be enough, but given that I made like 10 mistakes in this method already, that's a fair advice :) Added unittests. This required making BrowsingDataFilters comparable, and BrowsingDataRemover mockable. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:35: static const char* kClearingThreeTypes = "Clearing %s, %s and %s."; On 2016/06/20 07:57:37, Mike West (OOO through 4th) wrote: > Nit: Oxford comma, plz. Done. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:148: } On 2016/06/20 07:57:38, Mike West (OOO through 4th) wrote: > It doesn't seem worthwhile to distinguish between this error case and the > following; I'd suggest just dropping this `if`, and changing the error message > on line 155 to something like "%s is not a JavaScript object with a 'types' > field." Done. But I would still call it a JSON dictionary rather than a JavaScript object. They're not the same thing (e.g. JSON doesn't allow trailing commas while JS does), and we're really using a JSON parser here. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:180: serializer.Serialize(*value); On 2016/06/20 07:57:38, Mike West (OOO through 4th) wrote: > Nit: It doesn't look like you use |serialized_type| or |serializer| for > anything. Done. Thanks for catching. If |value| is e.g. another dictionary, GetAsString() above returns false, so |type| will be empty. In that case, I need to serialize it to output it below. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:187: // Each data type should only be On 2016/06/20 07:57:37, Mike West (OOO through 4th) wrote: > Nit: Should only be... ? Correct. Every data type should exist, and nothing more! :) ( But really - it was supposed to say "...processed once". So that I don't have to deal with duplicates. ) https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:188: DCHECK(datatype != nullptr); On 2016/06/20 07:57:37, Mike West (OOO through 4th) wrote: > Nit: `DCHECK(datatype)` Done. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:222: ConsoleLog(messages, output, CONSOLE_MESSAGE_LEVEL_LOG); On 2016/06/20 07:57:37, Mike West (OOO through 4th) wrote: > Nit: Would you mind pulling this out into a static helper in the anonymous > namespace above? Done. That also requires ConsoleMessage to be public. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:55: // Run three HTTPS servers, each serving 127.0.0.1 on a different port. On 2016/06/20 07:57:38, Mike West wrote: > Please add some tests that test the registrable domain bits and pieces > (`host_resolver()->AddRule("*", "127.0.0.1");`). Done. I was originally avoiding host_resolver(), because I wasn't able to connect to hosts other than localhost on HTTPS anyway. Apparently switches::kIgnoreCertificateErrors is the solution that other browsertests in my situation use for this. However! This doesn't help us to test registrable domain bits. We only inform the embedder of the requesting origin - it is the embedder (specifically ChromeContentBrowserClient) that decides which datatypes are deleted in which scope. Therefore, I tested it in ChromeContentBrowserClient. If you think that that decision should be made in //content, we will have to change to API to something like: ClearDomainData(string domain, bool cookies) ClearOriginData(Origin origin, bool storage, bool cache) or something similar. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:86: // middle, or end of the chain sends the header. On 2016/06/20 07:57:38, Mike West wrote: > What about when two (or all three) send the header? I vaguely remember blocking > simultaneous clearing from the extension API. Do we need to worry about that > here? Added tests for that. I'll implement the serialization in BrowsingDataRemover in parallel, let me just respond to the comments in https://docs.google.com/document/d/1oIbmZJlq5m5FvHcN7trCpp0h4mtydA0UfZasxEIe3P8/ first. For the rest of this CL, I'll assume that this is already done under the hood. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:41: {"{ \"types\": [\"cookies\", \"cache\"] }", true, false, true}, On 2016/06/20 07:57:38, Mike West (OOO through 4th) wrote: > You might as well do all of the options. Done. https://codereview.chromium.org/2025683003/diff/180001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:44: {"{ \"types\": [\"storage\", \"cache\", \"cookies\"] }", true, true, true}, On 2016/06/20 07:57:38, Mike West (OOO through 4th) wrote: > Might as well add a check that the order doesn't matter. Done.
Oh, and - since a lot of time has passed, I had to rebase first, and address your comments later. Sorry for that!
Looking at this now, but first I'll note that the failures in `ChromeContentBrowserClientClearSiteDataTest.Parameters` look real and relevant, just in case you haven't seen them.
https://codereview.chromium.org/2025683003/diff/300001/chrome/browser/browsin... File chrome/browser/browsing_data/registrable_domain_filter_builder.cc (right): https://codereview.chromium.org/2025683003/diff/300001/chrome/browser/browsin... chrome/browser/browsing_data/registrable_domain_filter_builder.cc:113: } To shrink this CL, and give you pieces to land right away, would you mind pulling out `//chrome/browser/browsing_data` into a separate patch? Those look great, and we could put them into the CQ right now. :) https://codereview.chromium.org/2025683003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2025683003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:54: // in tests that need a valid header but do not depend about its value. Nit: s/depend about/depend on/
Patchset #11 (id:320001) has been deleted
I extracted the //chrome/browser/browsing_data part into a separate CL and fixed the tests (just typos in the boolean algebra; of course, a result of "optimizations" right before uploading the CL...) https://codereview.chromium.org/2025683003/diff/300001/chrome/browser/browsin... File chrome/browser/browsing_data/registrable_domain_filter_builder.cc (right): https://codereview.chromium.org/2025683003/diff/300001/chrome/browser/browsin... chrome/browser/browsing_data/registrable_domain_filter_builder.cc:113: } On 2016/07/18 08:48:18, Mike West wrote: > To shrink this CL, and give you pieces to land right away, would you mind > pulling out `//chrome/browser/browsing_data` into a separate patch? Those look > great, and we could put them into the CQ right now. :) Extracted to https://codereview.chromium.org/2161583002/. PTAL :) https://codereview.chromium.org/2025683003/diff/300001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2025683003/diff/300001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:54: // in tests that need a valid header but do not depend about its value. On 2016/07/18 08:48:18, Mike West wrote: > Nit: s/depend about/depend on/ Done. (I originally wrote "care about")
Patchset #11 (id:340001) has been deleted
Rebase?
This LGTM with the exception of the Channel ID handling. Let's discuss that while you loop folks in for the //content reviews. I'd recommend clamy@, as she knows the navigation/throttling code like the back of her hand. https://codereview.chromium.org/2025683003/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2025683003/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2504: if (remove_storage) Clearing channel ID and cookies based on separate flags still feels wrong to me. Did you take a look at my comments in https://codereview.chromium.org/2025683003#msg27? I didn't see a response... https://codereview.chromium.org/2025683003/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:400: if (origin_filter_builder_) { Nit: DCHECK_FALSE(origin_filter_builder_ && domain_filter_builder_);
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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
For more details, see http://crbug.com/617627.
Description was changed from ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the resource (main frame) load. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. *** WORK IN PROGRESS *** - Successfully tested on a small testing server; the header is parsed and BrowsingDataRemover::RemoveImpl is executed. - The effect of data being asynchronously deleted while possibly also written by the website in the same time NOT yet tested. - Unittest NOT yet finished. BUG=607897 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the resource (main frame) load. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. *** WORK IN PROGRESS *** - Successfully tested on a small testing server; the header is parsed and BrowsingDataRemover::RemoveImpl is executed. - The effect of data being asynchronously deleted while possibly also written by the website in the same time NOT yet tested. - Unittest NOT yet finished. 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
The extracted part has landed now, so I rebased over it, and addressed your comment about the channel IDs :) https://codereview.chromium.org/2025683003/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2025683003/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2504: if (remove_storage) On 2016/07/18 10:20:34, Mike West wrote: > Clearing channel ID and cookies based on separate flags still feels wrong to me. > Did you take a look at my comments in > https://codereview.chromium.org/2025683003#msg27? I didn't see a response... I moved REMOVE_CHANNEL_IDS to the "cookies" bucket. My original plan, as I mentioned in an earlier comment, was to bundle it with REMOVE_COOKIES. However, that is unnecessary - REMOVE_CHANNEL_IDS and REMOVE_COOKIES are in the exact same situation in that they're both part of REMOVE_SITE_DATA, but not a part of the "storage" bucket. I still can make that particular change if you believe that there is no reason *in general*, not just in this API, to delete cookies without channel IDs. I can see that you also bundled them together in the extension API. But that doesn't have to block this CL. https://codereview.chromium.org/2025683003/diff/360001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/360001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:400: if (origin_filter_builder_) { On 2016/07/18 10:20:34, Mike West wrote: > Nit: DCHECK_FALSE(origin_filter_builder_ && domain_filter_builder_); Done. I actually made it a XOR.
msramek@chromium.org changed reviewers: + clamy@chromium.org
Hi Camille, Can you please have a look at the navigation handle & throttle parts? You may have already seen parts of what I'm trying to do in the context of Mike's CLs: https://codereview.chromium.org/2097083002/ https://codereview.chromium.org/2101323002/ Thanks, Martin
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM again, except that I don't like the aesthetics of your XOR, but I won't stop you from landing it even though it's weird and strange. :) https://codereview.chromium.org/2025683003/diff/400001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/400001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:401: DCHECK(!origin_filter_builder_ || !domain_filter_builder_); Hrm. It's not clear to me that this is better than what I suggested. Or the XOR you promised. But whatever. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
Thanks! A few comments. https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:38: void ConsoleLog(std::vector<ClearSiteDataThrottle::ConsoleMessage>* messages, @mkwst: Didn't you have a patch that enables this feature at the NavigationHandle level? If we plan to have it used by several WebContentsObservers and NavigationThrottles, this should be put in the NavigationHandle. https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:77: navigation_handle()->GetRenderFrameHost()->AddMessageToConsole( This won't work when the navigation is transferred. The messages will be outputted to the wrong RFH. We plan to fix this, but the patches haven't landed yet. https://codereview.chromium.org/2025683003/diff/400001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2025683003/diff/400001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:540: throttles_.push_back(std::move(clear_site_data_throttle)); If the throttle is put there, it means it will execute before other throttles that can cancel the navigation (potentially for security reasons). I'm not sure this is the behavior we want. Also considering that the throttle doesn't actually pause or cancel the navigation, can we add a TODO to move this to a WebContentsObserver once we call WCO::ReadyToCommitNavigation in the current architecture (see crbug.com/621856). I don't see a good reason to have it as a throttle once we get the WCO call after the response.
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! I answered your questions and asked more questions :) https://codereview.chromium.org/2025683003/diff/400001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/400001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client_unittest.cc:401: DCHECK(!origin_filter_builder_ || !domain_filter_builder_); On 2016/07/18 13:35:48, Mike West wrote: > Hrm. It's not clear to me that this is better than what I suggested. Or the XOR > you promised. > > But whatever. :) Well, there is no DCHECK_FALSE macro :) And I don't want to use the bitwise xor (^) for a boolean operation. So... (A or B) and (~A or ~B) = A xor B. https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:38: void ConsoleLog(std::vector<ClearSiteDataThrottle::ConsoleMessage>* messages, On 2016/07/18 14:47:49, clamy wrote: > @mkwst: Didn't you have a patch that enables this feature at the > NavigationHandle level? If we plan to have it used by several > WebContentsObservers and NavigationThrottles, this should be put in the > NavigationHandle. For the record, it was this one: https://codereview.chromium.org/2101323002/ But it's not landed yet. If I can assume that it's close to the final state, I could commit it locally and rebase on top of it to see how this CL is going to look like with it? https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:77: navigation_handle()->GetRenderFrameHost()->AddMessageToConsole( On 2016/07/18 14:47:49, clamy wrote: > This won't work when the navigation is transferred. The messages will be > outputted to the wrong RFH. We plan to fix this, but the patches haven't landed > yet. My original solution was actually using WebContentsObserver::DidFinishNavigation - should I return to that solution? Do you have estimates for when the fix comes in? And am I informed correctly that this is only about web<->extension navigations? https://codereview.chromium.org/2025683003/diff/400001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2025683003/diff/400001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:540: throttles_.push_back(std::move(clear_site_data_throttle)); On 2016/07/18 14:47:49, clamy wrote: > If the throttle is put there, it means it will execute before other throttles > that can cancel the navigation (potentially for security reasons). I'm not sure > this is the behavior we want. > > Also considering that the throttle doesn't actually pause or cancel the > navigation, can we add a TODO to move this to a WebContentsObserver once we call > WCO::ReadyToCommitNavigation in the current architecture (see crbug.com/621856). > I don't see a good reason to have it as a throttle once we get the WCO call > after the response. Done. Good point - moved it to the end. We haven't yet decided whether the data should be cleared synchronously or not. The former might be a horrible user experience, the latter can lead to wrong assumptions or unexpected behavior on the website if it starts writing data before clearing was finished. We started with the asynchronous version, as it's simpler and we want to see the how it works in the wild. It is possible that will yet decide that we want synchronous clearing, in which case the throttle should delay navigation. I mentioned that in the TODO.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
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_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:77: navigation_handle()->GetRenderFrameHost()->AddMessageToConsole( On 2016/07/18 17:03:20, msramek wrote: > On 2016/07/18 14:47:49, clamy wrote: > > This won't work when the navigation is transferred. The messages will be > > outputted to the wrong RFH. We plan to fix this, but the patches haven't > landed > > yet. > > My original solution was actually using WebContentsObserver::DidFinishNavigation > - should I return to that solution? > > Do you have estimates for when the fix comes in? And am I informed correctly > that this is only about web<->extension navigations? WebContentsObserver::DidFinishNavigation will give you the correct RenderFrameHost. Currently this is only used for web<->extension navigations as part of isolated extensions. The fix may take a few weeks to come in, since it's blocked on landing 2 rather complex CLs.
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...
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
Patchset #17 (id:480001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #17 (id:480001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In the last three patchsets, I have fixed the layout tests failures: 1. Complaining to the console that we're not on HTTPS even if there is no Clear-Site-Data-header -> moved that lower. 2. Null response headers with "data:" scheme -> now checking if the headers are null instead of checking scheme. ...and addressed your comment about WebContentsObserver. PTAL! https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/400001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:77: navigation_handle()->GetRenderFrameHost()->AddMessageToConsole( On 2016/07/19 13:27:54, clamy wrote: > On 2016/07/18 17:03:20, msramek wrote: > > On 2016/07/18 14:47:49, clamy wrote: > > > This won't work when the navigation is transferred. The messages will be > > > outputted to the wrong RFH. We plan to fix this, but the patches haven't > > landed > > > yet. > > > > My original solution was actually using > WebContentsObserver::DidFinishNavigation > > - should I return to that solution? > > > > Do you have estimates for when the fix comes in? And am I informed correctly > > that this is only about web<->extension navigations? > > WebContentsObserver::DidFinishNavigation will give you the correct > RenderFrameHost. > > Currently this is only used for web<->extension navigations as part of isolated > extensions. The fix may take a few weeks to come in, since it's blocked on > landing 2 rather complex CLs. Ok, then I guess it's better if we don't wait for it. I implemented the solution with WebContentsObserver. One thing that I found surprising though is that: 1. I have a navigation handle. 2. I take its WebContents and use a WebContentsObserver on it. 3. WebContentsObserver::DidFinishNavigation() passes me a navigation handle. 4. Expected: The handle in #3 is the same as in #1. Actual: Not always. Is this also related to transfer navigations? I thought that it's the point of the handle that it stays the same?
Description was changed from ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the resource (main frame) load. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. *** WORK IN PROGRESS *** - Successfully tested on a small testing server; the header is parsed and BrowsingDataRemover::RemoveImpl is executed. - The effect of data being asynchronously deleted while possibly also written by the website in the same time NOT yet tested. - Unittest NOT yet finished. BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the navigation. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. Tests: - ClearSiteDataThrottleUnittest - parsing - error messages - ClearSiteDataThrottleBrowsertest - secure origins - redirects, - integration with the embedder - ChromeContentBrowserClientTest - removal parameters for BrowsingDataRemover Note that the effect of data being asynchronously deleted while possibly also written by the website in the same time has NOT yet been tested. BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Friendly ping! Camille, do you have more comments on this CL?
A few more comments. I find a class being a NavigationThrottle and a WCO very weird, I think it should be one or the other. https://codereview.chromium.org/2025683003/diff/500001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/500001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:40: messages->push_back({text, level}); a) Are we okay if those never get delivered? Ex: we cleared something during a redirect, but the navigation was aborted before it could commit. b) If we navigate to a, that redirects us to b and asks us to clear site data, is it okay to send messages about site clearing from a to the renderer that is displaying b? https://codereview.chromium.org/2025683003/diff/500001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2025683003/diff/500001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:28: class CONTENT_EXPORT ClearSiteDataThrottle : public NavigationThrottle, It seems very weird to me to have a class that is both a throttle and an observer. Considering that you don't need to cancel anything, you probably just need a WebContentsObserver (and not a NavigationThrottle). WebContentsObserver has a method that is called on redirects (DidRedirectNavigation), so there is no need of a throttle for that.
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: 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 #19 (id:540001) 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 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! I addressed your comments. Unfortunately the rename/copy detection was not as good as I hoped, or it only works if the base file is already committed. The renaming between patchsets 18->19 looks like a new file :( It's supposed to be 1. Remove the inheritance from NavigationThrottle 2. Replace methods overridden from NavigationThrottle with those from WebContentsObserver 3. Rename to ClearSiteDataHeaderObserver, s/throttle/observer in comments 4. Change header files, build files etc. 5. Instantiate ClearSiteDataHeaderObserver from the constructor of NavigationThrottle (otherwise we'd miss the DidStartNavigation call) https://codereview.chromium.org/2025683003/diff/500001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/500001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:40: messages->push_back({text, level}); On 2016/07/26 16:48:13, clamy wrote: > a) Are we okay if those never get delivered? Ex: we cleared something during a > redirect, but the navigation was aborted before it could commit. > b) If we navigate to a, that redirects us to b and asks us to clear site data, > is it okay to send messages about site clearing from a to the renderer that is > displaying b? a) According to the description of WebContentsObserver::DidFinishNavigation, that method is called even if the navigation is aborted. So it sounds to me that we will always have the chance to output the console logs? b) Yep, that's a bug! We now store the URL in the ConsoleMessage. I also changed the logic so that we only output the URL once per a set of messages to avoid spam. https://codereview.chromium.org/2025683003/diff/500001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2025683003/diff/500001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:28: class CONTENT_EXPORT ClearSiteDataThrottle : public NavigationThrottle, On 2016/07/26 16:48:13, clamy wrote: > It seems very weird to me to have a class that is both a throttle and an > observer. Considering that you don't need to cancel anything, you probably just > need a WebContentsObserver (and not a NavigationThrottle). WebContentsObserver > has a method that is called on redirects (DidRedirectNavigation), so there is no > need of a throttle for that. Done. Renamed to ClearSiteDataHeaderObserver, and no longer inheriting from NavigationThrottle. This really made the class itself a bit cleaner, but it looks weird in NavigationHandleImpl, since it didn't have any such custom observers before. Are you OK with that? Even after this change, it's still a platform s/throttle/observer/ that is tied to a single navigation, so NavigationHandleImpl still seems like a reasonable place to put it to.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
clamy@chromium.org changed reviewers: + nasko@chromium.org
Sorry I did not have the time to review this today, and I'm going away for 2 weeks. @nasko: could you follow up with this? I asked for a change from a NavigationThrottle to a WebContentsObserver in the last patchset, so that they can monitor for DidFinishNavigation to get the right RenderFrameHost.
Some initial comments. However, I'd like to see a design document on how this header will be implemented, as the current structure in the CL doesn't seem correct to me. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_header_observer.cc (right): https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:58: ? new ClearSiteDataHeaderObserver(handle->GetWebContents()) This looks wrong to me. You only need one observer to observe a specific WebContents. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:90: std::string output; What is output used for? https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:91: if (message.url == last_seen_url) { last_seen_url is never set to any meaningful URL. What is its purpose? https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:103: ClearSiteDataHeaderObserver::ClearSiteDataHeaderObserver( nit: I'd put the constructor next to the destructor, even though they are not declared in that order in the header file. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:156: bool* clear_cookies, bool* clear_storage, bool* clear_cache, style: Each param is on a new line. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:159: base::JSONReader::Read(header); Since this is untrusted data from the network, you should use components/safe_json. However, that means it is async and cannot be done in sync fasion. If you want to keep this parser, you should at least reject non-ASCII inputs. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:161: if (!parsed_header) { I'll stop reviewing this method for now, as I think we need to figure out what the high level design is. Do you have a design document describing how this feature is to be implemented? https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_header_observer.h (right): https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:19: class Profile; This is unused. What is it for? https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:38: CreateFor(NavigationHandle* handle); Can you document what this method does? Since this is WebContentsObserver, it should only be created and correspond 1:1 with a WebContents object. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:52: // of WillProcessResponse() and WillRedirectRequest(). Those two methods no longer exist here. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:55: // Parses the value of the 'Clear-Site-Data' header and outputs whether Can you point to the spec for this header? It might be useful to also have it at the class level comment, as for now this class is used for that functionality only. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:67: friend class ClearSiteDataHeaderObserverTest; friend declarations go right after the private: label. https://codereview.chromium.org/2025683003/diff/580001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2025683003/diff/580001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:313: std::unique_ptr<ClearSiteDataHeaderObserver> clear_site_data_header_observer_; This doesn't seem correct. WebContentsObservers should be 1:1 with WebContents and WebContents is 1:many with NavigationHandle. https://codereview.chromium.org/2025683003/diff/580001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2025683003/diff/580001/content/public/browser... content/public/browser/content_browser_client.h:556: // for the entire eTLD+1 of |origin|. Why would cookies and other types of data differ? https://codereview.chromium.org/2025683003/diff/580001/content/public/browser... content/public/browser/content_browser_client.h:558: content::BrowserContext* browser_context, const url::Origin& origin, style: Each parameter goes on a new line. Ideally, just run "git cl format" to have automated tools take care of the formatting.
Patchset #21 (id:600001) 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #21 (id:620001) has been deleted
Hi Nasko, I addressed most of your comments, but not the ones about WebContentsObserver and safe_json. This would be the third time the structure of this class significantly changes in this CL, so I want to make sure everyone agrees first. Please see the longest of my comments in this CL for more background and please advise which approach do you think works best. Thanks, Martin https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_header_observer.cc (right): https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:58: ? new ClearSiteDataHeaderObserver(handle->GetWebContents()) On 2016/07/28 17:30:30, nasko wrote: > This looks wrong to me. You only need one observer to observe a specific > WebContents. Acknowledged. Please see the other comment with the explanation how we got here. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:90: std::string output; On 2016/07/28 17:30:30, nasko wrote: > What is output used for? Argh :( This piece of code was added as a last-minute "improvement" as I was hasting to send out this patchset in the afternoon before clamy@ left OOO. This is not actually used. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:91: if (message.url == last_seen_url) { On 2016/07/28 17:30:30, nasko wrote: > last_seen_url is never set to any meaningful URL. What is its purpose? Ditto as above. Sorry for that. |last_seen_url| is supposed to cache the URL from the previous iteration, so that we only output the prefix "Clear-Site-Data header on <URL>" once per a sequence of messages from the same URL. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:103: ClearSiteDataHeaderObserver::ClearSiteDataHeaderObserver( On 2016/07/28 17:30:30, nasko wrote: > nit: I'd put the constructor next to the destructor, even though they are not > declared in that order in the header file. Done. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:156: bool* clear_cookies, bool* clear_storage, bool* clear_cache, On 2016/07/28 17:30:30, nasko wrote: > style: Each param is on a new line. Done. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:159: base::JSONReader::Read(header); On 2016/07/28 17:30:30, nasko wrote: > Since this is untrusted data from the network, you should use > components/safe_json. However, that means it is async and cannot be done in sync > fasion. If you want to keep this parser, you should at least reject non-ASCII > inputs. I added the ASCII test for now. I'm happy to use safe_json for better security, but before I start, I want to make sure that we all agree on whether this should be a NavigationThrottle, a WebContentsObserver, or something else. The design of this class might change significantly depending on that, because if this class only lives for the duration of one navigation, I assume that the JSON parser might easily take longer and then we'll have to process everything in static functions. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:161: if (!parsed_header) { On 2016/07/28 17:30:30, nasko wrote: > I'll stop reviewing this method for now, as I think we need to figure out what > the high level design is. Do you have a design document describing how this > feature is to be implemented? I don't have a design doc at hand; I'm working with the spec at https://www.w3.org/TR/clear-site-data/. The spec defines that the value should be JSON of the form { "types": [ <string>, ... ] }. The spec doesn't prescribe how the console error messages should look like, so I'm just outputting one error message for each step where the parsing could fail. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_header_observer.h (right): https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:19: class Profile; On 2016/07/28 17:30:30, nasko wrote: > This is unused. What is it for? Removed, thanks for catching. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:38: CreateFor(NavigationHandle* handle); On 2016/07/28 17:30:30, nasko wrote: > Can you document what this method does? Since this is WebContentsObserver, it > should only be created and correspond 1:1 with a WebContents object. See the other comment - let's discuss whether this should be a WebContentsObserver in the first place. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:52: // of WillProcessResponse() and WillRedirectRequest(). On 2016/07/28 17:30:31, nasko wrote: > Those two methods no longer exist here. Renamed to WebContentObserver ~equivalents. https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:55: // Parses the value of the 'Clear-Site-Data' header and outputs whether On 2016/07/28 17:30:31, nasko wrote: > Can you point to the spec for this header? It might be useful to also have it at > the class level comment, as for now this class is used for that functionality > only. Done. (Referenced it in the class comment) https://codereview.chromium.org/2025683003/diff/580001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.h:67: friend class ClearSiteDataHeaderObserverTest; On 2016/07/28 17:30:30, nasko wrote: > friend declarations go right after the private: label. Done. https://codereview.chromium.org/2025683003/diff/580001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2025683003/diff/580001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:313: std::unique_ptr<ClearSiteDataHeaderObserver> clear_site_data_header_observer_; On 2016/07/28 17:30:31, nasko wrote: > This doesn't seem correct. WebContentsObservers should be 1:1 with WebContents > and WebContents is 1:many with NavigationHandle. I agree that this is weird here. Let me summarize the history of this CL first. The idea was that we might want the navigation to be blocked while deleting the data, that's why we used NavigationThrottle. But we weren't sure yet (it's hard to say without testing this out in the wild), and we think that the blocking approach will require some UI to tell the user what's going on. That's why we started with a NavigationThrottle. Now, Camille suggested to use a WebContentsObserver instead since a) we don't need throttle if we're not actually blocking the navigation yet, and b) we need to output console messages, but NavigationThrottle::WillProcessResponse might not have the correct RFH yet. So I changed NavigationThrottle to WebContentsObserver. But this is no longer a good place to hook it up. Do you have any suggestion where to? This observer is supposed to exist on every single WebContents, since it's the platform that should recognize the Clear-Site-Data header, not a browser feature. Alternatively, we can return to NavigationThrottle if we find a different way to output console messages. And actually, I should probably write all of this down to a doc. https://codereview.chromium.org/2025683003/diff/580001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2025683003/diff/580001/content/public/browser... content/public/browser/content_browser_client.h:556: // for the entire eTLD+1 of |origin|. On 2016/07/28 17:30:31, nasko wrote: > Why would cookies and other types of data differ? The proposal is to delete data for an origin as a security boundary. However, when we started building the infrastructure to this change, it was pointed out that "it will break the web". Websites generally use cookies from different origins (e.g. mail.google.com from accounts.google.com); such partial deletion would lead to unexpected behavior (inconsistent state), and it would also not achieve the goal of clearing all places where the website could possibly store data. It's possible that we'll eventually decide to delete everything per origin anyway, or that we'll switch to eTLD+1 for consistency. But for V1, we just want to use what we have so that we can start experimenting in the real world. https://codereview.chromium.org/2025683003/diff/580001/content/public/browser... content/public/browser/content_browser_client.h:558: content::BrowserContext* browser_context, const url::Origin& origin, On 2016/07/28 17:30:31, nasko wrote: > style: Each parameter goes on a new line. Ideally, just run "git cl format" to > have automated tools take care of the formatting. Done, also ran git cl format. (Some parts of this CL are definitely better formatted now, but sometimes git cl format produces less readable results).
...and I rebased, since the last patchset had a dependency that was in the meantime removed (https://chromium.googlesource.com/chromium/src/+/1671ed3071b42be24a36b1dccb5b...).
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2025683003/diff/580001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2025683003/diff/580001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:313: std::unique_ptr<ClearSiteDataHeaderObserver> clear_site_data_header_observer_; On 2016/08/01 16:03:19, msramek wrote: > On 2016/07/28 17:30:31, nasko wrote: > > This doesn't seem correct. WebContentsObservers should be 1:1 with WebContents > > and WebContents is 1:many with NavigationHandle. > > I agree that this is weird here. Let me summarize the history of this CL first. > > The idea was that we might want the navigation to be blocked while deleting the > data, that's why we used NavigationThrottle. But we weren't sure yet (it's hard > to say without testing this out in the wild), and we think that the blocking > approach will require some UI to tell the user what's going on. Hmm, why do we need UI for this? It is part of the loading of a page that the page requests the browser to do the data deletion. Exposing UI for this will only lead to confusion, so IMHO it is ok to just proceed with this. Furthermore, I think it will be a lot more consistent to defer the navigation until the data is deleted and only allow it to commit once the data deletion is complete. However, this is more of a design/spec question, so let's discuss separately. > That's why we > started with a NavigationThrottle. Now, Camille suggested to use a > WebContentsObserver instead since a) we don't need throttle if we're not > actually blocking the navigation yet, and b) we need to output console messages, > but NavigationThrottle::WillProcessResponse might not have the correct RFH yet. Sounds like a good suggestion :). If you don't need to defer/pause navigations, WebContentsObserver is what you would want to have. > So I changed NavigationThrottle to WebContentsObserver. But this is no longer a > good place to hook it up. Do you have any suggestion where to? This observer is > supposed to exist on every single WebContents, since it's the platform that > should recognize the Clear-Site-Data header, not a browser feature. Why not instantiate it in the WebContentsImpl constructor? ;) There are already observers as members in WebContentsImpl itself and you can add the new one directly there. > Alternatively, we can return to NavigationThrottle if we find a different way to > output console messages. Let's not do a NavigationThrottle, unless we really need to pause navigations to do deletion. > And actually, I should probably write all of this down to a doc. Thanks for the doc. https://codereview.chromium.org/2025683003/diff/580001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2025683003/diff/580001/content/public/browser... content/public/browser/content_browser_client.h:558: content::BrowserContext* browser_context, const url::Origin& origin, On 2016/08/01 16:03:19, msramek wrote: > On 2016/07/28 17:30:31, nasko wrote: > > style: Each parameter goes on a new line. Ideally, just run "git cl format" to > > have automated tools take care of the formatting. > > Done, also ran git cl format. (Some parts of this CL are definitely better > formatted now, but sometimes git cl format produces less readable results). The main goal of git cl format is to remove any discussions on what looks more or less readable, so please run it on all CLs. You remove a layer of cognitive load from reviewers and lets them spend more mental energy on the things that matter. https://codereview.chromium.org/2025683003/diff/640001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_header_observer.cc (right): https://codereview.chromium.org/2025683003/diff/640001/content/browser/browsi... content/browser/browsing_data/clear_site_data_header_observer.cc:164: if (base::IsStringASCII(header)) It is better to invert the check and return early. It is the more common pattern in content/ and easier to read.
https://codereview.chromium.org/2025683003/diff/580001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2025683003/diff/580001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:313: std::unique_ptr<ClearSiteDataHeaderObserver> clear_site_data_header_observer_; On 2016/08/03 20:53:13, nasko (Out until 8-8) wrote: > On 2016/08/01 16:03:19, msramek wrote: > > On 2016/07/28 17:30:31, nasko wrote: > > > This doesn't seem correct. WebContentsObservers should be 1:1 with > WebContents > > > and WebContents is 1:many with NavigationHandle. > > > > I agree that this is weird here. Let me summarize the history of this CL > first. > > > > The idea was that we might want the navigation to be blocked while deleting > the > > data, that's why we used NavigationThrottle. But we weren't sure yet (it's > hard > > to say without testing this out in the wild), and we think that the blocking > > approach will require some UI to tell the user what's going on. > > Hmm, why do we need UI for this? It is part of the loading of a page that the > page requests the browser to do the data deletion. Exposing UI for this will > only lead to confusion, so IMHO it is ok to just proceed with this. > > Furthermore, I think it will be a lot more consistent to defer the navigation > until the data is deleted and only allow it to commit once the data deletion is > complete. However, this is more of a design/spec question, so let's discuss > separately. > > > That's why we > > started with a NavigationThrottle. Now, Camille suggested to use a > > WebContentsObserver instead since a) we don't need throttle if we're not > > actually blocking the navigation yet, and b) we need to output console > messages, > > but NavigationThrottle::WillProcessResponse might not have the correct RFH > yet. > > Sounds like a good suggestion :). If you don't need to defer/pause navigations, > WebContentsObserver is what you would want to have. > > > So I changed NavigationThrottle to WebContentsObserver. But this is no longer > a > > good place to hook it up. Do you have any suggestion where to? This observer > is > > supposed to exist on every single WebContents, since it's the platform that > > should recognize the Clear-Site-Data header, not a browser feature. > > Why not instantiate it in the WebContentsImpl constructor? ;) There are already > observers as members in WebContentsImpl itself and you can add the new one > directly there. > > > Alternatively, we can return to NavigationThrottle if we find a different way > to > > output console messages. > > Let's not do a NavigationThrottle, unless we really need to pause navigations to > do deletion. > > > And actually, I should probably write all of this down to a doc. > > Thanks for the doc. Thanks a lot for your feedback in the doc, Nasko! I didn't have a chance to get back to the CL today, but the way forward seems to be clear. It seems that by now there's more support for synchronous deletion, so I'll talk to Mike again to make a more binding decision. If we proceed with the synchronous one, I'll return to using a NavigationThrottle and see if I can get RFH in the destructor. If we proceed with the asynchronous, we'll stay with WebContentsObserver that will be instantiated directly in WebContentsImpl.
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_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...
Patchset #24 (id:700001) 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...
Nasko, please have another look! I rebased (Patchset 23) and made a number of changes (Patchset 24) that were again often coupled in such ways that it wasn't really possible to split this into several patchsets that would individually make sense and compile :( But if I understand correctly, you didn't review the details of the CL as we haven't sorted out the architecture yet at that time, so I hope it's OK. Changes: 1. Rebased. Since the signature of BrowsingDataRemover::Remove[...] methods changed to accept scoped_ptr, which is non-copyable, it is no longer possible to use GMOCK to test it. Wrote a custom mock class in chrome_content_browser_client_unittest. 2. I talked to mkwst@ and we decided to go with *blocking* deletion first. So I changed back to NavigationThrottle which now DEFERs until the deletion is complete. This also means that ChromeContentBrowserClient::ClearSiteData() signature had to be changed to include a callback. We are now outputting the console messages in the throttle destructor. This one is called when NavigationHandle is destroyed, which is immediately after WebContentsObserver::DidFinishNavigation calls are processed, so timing is basically the same as we had before. 3. We are now checking if the input is ASCII. Nasko, when you told me to use safe_json, I knew there was a reason why I didn't use it originally, but I had forgotten (this CL is quite old now). It's because safe_json can't be used in //content - it's a circular dependency. I heard that security-wise this is not such a problem in //content/renderer, but we're in //content/browser. So, is the ASCII test sufficient? Perhaps we should change the spec, because a JSON format here might be an overkill anyway. 4. Small change: I stopped SPrintf-ing the header to the console outputs. It actually makes them quite long and unreadable. We generally only expect one header in the response, so it's unnecessary to always repeat the value of the header that caused the error. And even if there was more than one, the valid ones have a console output as well so it's not difficult to determine which console output belongs to which header.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping! :)
I didn't review chrome/browser/chrome_content_browser_client_unittest.cc, but covered the rest. https://codereview.chromium.org/2025683003/diff/720001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2025683003/diff/720001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2539: if (remove_mask) { At this point, checking remove_mask is equivalent to checking remove_cookies. Why not check directly that? Also, on line 2547 the remove_mask is passed in, but it can only be the OR of the two flags. Why not directly pass them there? https://codereview.chromium.org/2025683003/diff/720001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2555: remove_mask = 0; Since remove_mask is reset here, its usage above (for cookies) is not needed. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:41: void ConsoleLog(std::vector<ClearSiteDataThrottle::ConsoleMessage>* messages, nit: Seems an overkill to have a full on method for a single line. Makes code less readable. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:73: // of messages belonging to the same URL with |kConsoleMessagePrefix|. |messages| is a vector and there is no sorting performed here. How do we guarantee that the same URLs will be combined? Do we care? https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:121: The spec calls out that Clear-Site-Data is only valid when the response URL is not "insecure". I don't see such a check in this implementation. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:126: while (handle->GetResponseHeaders()->EnumerateHeaderLines(&iter, &header_name, Do we want to allow multiple instances of the header? I'd suggest the spec clarify that only one instance is expected and whether we honor the first(my preference)/last when duplicates are encountered. This will allow for using HasHeaderValue and avoiding lots of string copies, which the current implementation will make. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:152: url::Origin origin(current_url_); Do we honor unique origins for deletion too? The spec doesn't call it out. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:154: ++active_tasks_; Honoring only single instance of the header will also avoid counting of tasks, since we will only have one. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:168: if (base::IsStringASCII(header)) nit: Invert the check and early return. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:173: "Not a valid JSON or non-ASCII characters present.", Wouldn't we want to be more precise as to what went wrong? As a developer I'd appreciate if I knew which one of those two conditions I've hit with my header. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:68: // Signalizes that a parsing and deletion task was finished. nit: Signals? https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:73: class ClearSiteDataThrottleBrowsertest : public ContentBrowserTest { Capitalize: BrowserTest. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:142: std::map<GURL, std::string> headers_; Unused? https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:143: std::map<GURL, GURL> redirects_; Unused? https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:158: for (int mask = 0; mask < (1 << 3); ++mask) { Why use "(1 << 3)" instead of the actual constant? You even mention that there are 8 configs to tests in the comment :). https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:64: {"{ \"types\": [\"storage\"], \"other_params\": {} }", false, true, Shouldn't we be failing the parsing in this case? https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:69: {"{ \"types\": [\"cache\", \"foo\"] }", false, false, true}, Typos are easy to occur, so I think it is best for us to either fail fully the parsing (probably not) or at least emit an error message that unrecognized data was seen. This way if I put "cookie" and my eyes glance over thinking the "s" is there, I wouldn't be surprised when I still see cookies around. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:91: TEST_F(ClearSiteDataThrottleTest, InvalidHeader) { I would also suggest looking at libFuzzer and writing a simple fuzzer for the parsing code. It should be fairly easy to do so. Doing it in a separate CL will be great :). https://security.googleblog.com/2016/08/guided-in-process-fuzzing-of-chrome.html https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:565: throttles_to_register.push_back(clear_site_data_throttle.release()); nit: std::move(clear_site_data_throttle) https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:12: #include <memory> There are no changes to actual code in this header file. Why is this include needed here? https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:30: class ClearSiteDataHeaderObserver; Why is this forward declare needed? https://codereview.chromium.org/2025683003/diff/720001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2025683003/diff/720001/content/public/browser... content/public/browser/content_browser_client.h:556: // |remove_storage|, and |remove_cache|. Note that cookies will be removed nit: s/will be removed/should be removed/, as this is an API for embedders and we need to tell them what content/ expects to happen. Since content/ isn't doing the deletion, it can't say what "will" happen :). https://codereview.chromium.org/2025683003/diff/720001/content/public/browser... content/public/browser/content_browser_client.h:557: // for the entire eTLD+1 of |origin|. Calls |callback| when finished. nit: Same here - must call |callback|.
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_...)
Patchset #25 (id:740001) 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_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #26 (id:780001) 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...
Thanks a lot for the thorough review, Nasko! Please have another look :) I had to rebase again because of a merge conflict, but I did so after addressing your comments, so the diff is well visible this time. https://codereview.chromium.org/2025683003/diff/720001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2025683003/diff/720001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2539: if (remove_mask) { On 2016/08/11 20:07:21, nasko (slow) wrote: > At this point, checking remove_mask is equivalent to checking remove_cookies. > Why not check directly that? Also, on line 2547 the remove_mask is passed in, > but it can only be the OR of the two flags. Why not directly pass them there? Done. I was probably mostly trying to make the two blocks (domain- and origin-scoped deletions) look the same. Changed it to test |remove_cookies| directly, and also put the initialization of the filter inside that if() block. https://codereview.chromium.org/2025683003/diff/720001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2555: remove_mask = 0; On 2016/08/11 20:07:21, nasko (slow) wrote: > Since remove_mask is reset here, its usage above (for cookies) is not needed. Done. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:41: void ConsoleLog(std::vector<ClearSiteDataThrottle::ConsoleMessage>* messages, On 2016/08/11 20:07:22, nasko (slow) wrote: > nit: Seems an overkill to have a full on method for a single line. Makes code > less readable. Hmm, here I would argue that ConsoleLog(where, what) is semantically clearer than where->push_back(what), especially with the {} struct initializer. Do you feel strongly about that? https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:73: // of messages belonging to the same URL with |kConsoleMessagePrefix|. On 2016/08/11 20:07:21, nasko (slow) wrote: > |messages| is a vector and there is no sorting performed here. How do we > guarantee that the same URLs will be combined? Do we care? If we're going through a redirect url1->url2, then all messages for url1 will be necessarily pushed to the vector before url2, since we're now blocking the navigation to url2 before url1 is processed. Even if we processed them asynchronously, they're on the same thread, so the ordering would be retained. If what you have in mind is a redirect chain url1->url2->url1, then it's going to output "url1 ... url2 ... url1 ...", which I think makes sense. I'm basically just trying to compact "Clear-Site-Data header on https://www.example.com/index.html: <msg1>" "Clear-Site-Data header on https://www.example.com/index.html: <msg2>" "Clear-Site-Data header on https://www.example.com/index.html: <msg3>" into "Clear-Site-Data header on https://www.google.com/index.html: <msg1>" "<msg2>" "<msg3>" to make it less spammy. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:121: On 2016/08/11 20:07:22, nasko (slow) wrote: > The spec calls out that Clear-Site-Data is only valid when the response URL is > not "insecure". I don't see such a check in this implementation. You're correct to expect the call to IsOriginSecure() exactly at this line. It was here, but I moved it a few lines below into the while() loop. The reason was that I wanted to output an error message if the origin is insecure AND the header is present. Since you suggested in the next comment to remove the while() loop, this is no longer necessary. Note that there is also a test in ClearSiteDataThrottleBrowsertest to verify that we don't run this on HTTP. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:126: while (handle->GetResponseHeaders()->EnumerateHeaderLines(&iter, &header_name, On 2016/08/11 20:07:21, nasko (slow) wrote: > Do we want to allow multiple instances of the header? I'd suggest the spec > clarify that only one instance is expected and whether we honor the first(my > preference)/last when duplicates are encountered. This will allow for using > HasHeaderValue and avoiding lots of string copies, which the current > implementation will make. Done. It's true that currently there's no reason to have more than one instance, since for example { 'types': ['cookies'] } { 'types': ['storage'] } can be merged to { 'types': ['cookies', 'storage'] } and for example { 'types': ['cookies'] } { 'types': ['cookies'] } is idempotent. Note that the original version of the spec didn't have this property, and that might change again in the future, but for v1, let's simplify it. It seems that EnumerateHeader(nullptr, ...) is what we want to use, but that also requires us to list the header in HttpUtil::IsNonCoalescingHeader() otherwise the JSON will be broken on commas. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:152: url::Origin origin(current_url_); On 2016/08/11 20:07:21, nasko (slow) wrote: > Do we honor unique origins for deletion too? The spec doesn't call it out. That's a good question. The current implementation of url::Origin says that unique origins are not equal to anything, not even to themselves. That means that comparing the origin of data storage backend to the origin specified in the header will always return false for unique origin. The deletion cannot be performed. headers->GetResponseHeaders() already excludes "file:" and "data:" scheme navigations, and I would expect that an unique origin cannot be IsOriginSecure(), but let me put a check here just to be sure. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:154: ++active_tasks_; On 2016/08/11 20:07:21, nasko (slow) wrote: > Honoring only single instance of the header will also avoid counting of tasks, > since we will only have one. Done. But we'll still need at least a boolean. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:168: if (base::IsStringASCII(header)) On 2016/08/11 20:07:22, nasko (slow) wrote: > nit: Invert the check and early return. Done. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:173: "Not a valid JSON or non-ASCII characters present.", On 2016/08/11 20:07:21, nasko (slow) wrote: > Wouldn't we want to be more precise as to what went wrong? As a developer I'd > appreciate if I knew which one of those two conditions I've hit with my header. Done. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.h (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.h:68: // Signalizes that a parsing and deletion task was finished. On 2016/08/11 20:07:22, nasko (slow) wrote: > nit: Signals? Done. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_browsertest.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:73: class ClearSiteDataThrottleBrowsertest : public ContentBrowserTest { On 2016/08/11 20:07:22, nasko (slow) wrote: > Capitalize: BrowserTest. Done. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:142: std::map<GURL, std::string> headers_; On 2016/08/11 20:07:22, nasko (slow) wrote: > Unused? Done. Remnants of old design, thanks for catching! https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:143: std::map<GURL, GURL> redirects_; On 2016/08/11 20:07:22, nasko (slow) wrote: > Unused? Done. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_browsertest.cc:158: for (int mask = 0; mask < (1 << 3); ++mask) { On 2016/08/11 20:07:22, nasko (slow) wrote: > Why use "(1 << 3)" instead of the actual constant? You even mention that there > are 8 configs to tests in the comment :). I think this is a bit better semantically. As in, I'm interested in bit vectors 000...111 and the fact that they happen to be the binary representations of 0...7 is not important. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:64: {"{ \"types\": [\"storage\"], \"other_params\": {} }", false, true, On 2016/08/11 20:07:22, nasko (slow) wrote: > Shouldn't we be failing the parsing in this case? See the comment about compatibility below. If the header specification changes and new entries are added, it's probably better to at least delete what we know we can than to bail out. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:69: {"{ \"types\": [\"cache\", \"foo\"] }", false, false, true}, On 2016/08/11 20:07:22, nasko (slow) wrote: > Typos are easy to occur, so I think it is best for us to either fail fully the > parsing (probably not) or at least emit an error message that unrecognized data > was seen. This way if I put "cookie" and my eyes glance over thinking the "s" is > there, I wouldn't be surprised when I still see cookies around. We do handle that :) See the "Invalid type:" error message in clear_site_data_throttle.cc, and the "passwords" example in the InvalidHeader test below. But we do not interpret this as a failure. If there is at least one valid type specified, we go forward. Note that this is good for compatibility. If we add a new datatype, old versions of Chrome will not be able to delete it, but will still delete at least the ones they know. And this is not hypothetical - the spec mentions "executionContexts" which we don't have in this first version but we will have it in the future. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:91: TEST_F(ClearSiteDataThrottleTest, InvalidHeader) { On 2016/08/11 20:07:22, nasko (slow) wrote: > I would also suggest looking at libFuzzer and writing a simple fuzzer for the > parsing code. It should be fairly easy to do so. Doing it in a separate CL will > be great :). > > https://security.googleblog.com/2016/08/guided-in-process-fuzzing-of-chrome.html Yep, that sounds like something we might want :) Added a TODO. https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:565: throttles_to_register.push_back(clear_site_data_throttle.release()); On 2016/08/11 20:07:22, nasko (slow) wrote: > nit: std::move(clear_site_data_throttle) Done, also above from where I copy-pasted this. https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:12: #include <memory> On 2016/08/11 20:07:22, nasko (slow) wrote: > There are no changes to actual code in this header file. Why is this include > needed here? Removed. Sorry for that, another accidental change from the back-and-forth between a handle and an observer. https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:30: class ClearSiteDataHeaderObserver; On 2016/08/11 20:07:22, nasko (slow) wrote: > Why is this forward declare needed? Removed. Ditto as above. https://codereview.chromium.org/2025683003/diff/720001/content/public/browser... File content/public/browser/content_browser_client.h (right): https://codereview.chromium.org/2025683003/diff/720001/content/public/browser... content/public/browser/content_browser_client.h:556: // |remove_storage|, and |remove_cache|. Note that cookies will be removed On 2016/08/11 20:07:22, nasko (slow) wrote: > nit: s/will be removed/should be removed/, as this is an API for embedders and > we need to tell them what content/ expects to happen. Since content/ isn't doing > the deletion, it can't say what "will" happen :). Done. Makes sense. https://codereview.chromium.org/2025683003/diff/720001/content/public/browser... content/public/browser/content_browser_client.h:557: // for the entire eTLD+1 of |origin|. Calls |callback| when finished. On 2016/08/11 20:07:22, nasko (slow) wrote: > nit: Same here - must call |callback|. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Looks good! Just one question. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:41: void ConsoleLog(std::vector<ClearSiteDataThrottle::ConsoleMessage>* messages, On 2016/08/12 15:06:27, msramek wrote: > On 2016/08/11 20:07:22, nasko (slow) wrote: > > nit: Seems an overkill to have a full on method for a single line. Makes code > > less readable. > > Hmm, here I would argue that ConsoleLog(where, what) is semantically clearer > than where->push_back(what), especially with the {} struct initializer. Do you > feel strongly about that? I'm fine as it is. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:73: // of messages belonging to the same URL with |kConsoleMessagePrefix|. On 2016/08/12 15:06:27, msramek wrote: > On 2016/08/11 20:07:21, nasko (slow) wrote: > > |messages| is a vector and there is no sorting performed here. How do we > > guarantee that the same URLs will be combined? Do we care? > > If we're going through a redirect url1->url2, then all messages for url1 will be > necessarily pushed to the vector before url2, since we're now blocking the > navigation to url2 before url1 is processed. Even if we processed them > asynchronously, they're on the same thread, so the ordering would be retained. > > If what you have in mind is a redirect chain url1->url2->url1, then it's going > to output "url1 ... url2 ... url1 ...", which I think makes sense. > > I'm basically just trying to compact > > "Clear-Site-Data header on https://www.example.com/index.html: <msg1>" > "Clear-Site-Data header on https://www.example.com/index.html: <msg2>" > "Clear-Site-Data header on https://www.example.com/index.html: <msg3>" > > into > > "Clear-Site-Data header on https://www.google.com/index.html: <msg1>" > "<msg2>" > "<msg3>" > > to make it less spammy. Yes, I had the url1->url2->url1 pattern in mind. I'm fine with what is there, since we can tweak it per dev feedback later on. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:121: On 2016/08/12 15:06:27, msramek wrote: > On 2016/08/11 20:07:22, nasko (slow) wrote: > > The spec calls out that Clear-Site-Data is only valid when the response URL is > > not "insecure". I don't see such a check in this implementation. > > You're correct to expect the call to IsOriginSecure() exactly at this line. It > was here, but I moved it a few lines below into the while() loop. The reason was > that I wanted to output an error message if the origin is insecure AND the > header is present. Since you suggested in the next comment to remove the while() > loop, this is no longer necessary. > > Note that there is also a test in ClearSiteDataThrottleBrowsertest to verify > that we don't run this on HTTP. Hmm, I wrote the comment and then discarded it. I guess Rietveld thought it needs to keep it around : (. I did see the check later on, since I grepped for a different pattern when I wrote the comment. Yes, the test is very welcome too! https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:126: while (handle->GetResponseHeaders()->EnumerateHeaderLines(&iter, &header_name, On 2016/08/12 15:06:27, msramek wrote: > On 2016/08/11 20:07:21, nasko (slow) wrote: > > Do we want to allow multiple instances of the header? I'd suggest the spec > > clarify that only one instance is expected and whether we honor the first(my > > preference)/last when duplicates are encountered. This will allow for using > > HasHeaderValue and avoiding lots of string copies, which the current > > implementation will make. > > Done. > > It's true that currently there's no reason to have more than one instance, since > for example > > { 'types': ['cookies'] } > { 'types': ['storage'] } > > can be merged to > > { 'types': ['cookies', 'storage'] } > > and for example > > { 'types': ['cookies'] } > { 'types': ['cookies'] } > > is idempotent. > > Note that the original version of the spec didn't have this property, and that > might change again in the future, but for v1, let's simplify it. > > It seems that EnumerateHeader(nullptr, ...) is what we want to use, but that > also requires us to list the header in HttpUtil::IsNonCoalescingHeader() > otherwise the JSON will be broken on commas. Doesn't GetNormalizedHeader work? It will likely also have the side effect of not parsing correctly if there are multiple instances of it. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:152: url::Origin origin(current_url_); On 2016/08/12 15:06:27, msramek wrote: > On 2016/08/11 20:07:21, nasko (slow) wrote: > > Do we honor unique origins for deletion too? The spec doesn't call it out. > > That's a good question. > > The current implementation of url::Origin says that unique origins are not equal > to anything, not even to themselves. That means that comparing the origin of > data storage backend to the origin specified in the header will always return > false for unique origin. The deletion cannot be performed. > > headers->GetResponseHeaders() already excludes "file:" and "data:" scheme > navigations, and I would expect that an unique origin cannot be > IsOriginSecure(), but let me put a check here just to be sure. Maybe not right now with the current state of affairs, but in general you can have a document with valid http/https URL and unique origin - for example a sandboxed iframe hosting web content. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:154: ++active_tasks_; On 2016/08/12 15:06:27, msramek wrote: > On 2016/08/11 20:07:21, nasko (slow) wrote: > > Honoring only single instance of the header will also avoid counting of tasks, > > since we will only have one. > > Done. But we'll still need at least a boolean. Acknowledged. https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle_unittest.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle_unittest.cc:69: {"{ \"types\": [\"cache\", \"foo\"] }", false, false, true}, On 2016/08/12 15:06:28, msramek wrote: > On 2016/08/11 20:07:22, nasko (slow) wrote: > > Typos are easy to occur, so I think it is best for us to either fail fully the > > parsing (probably not) or at least emit an error message that unrecognized > data > > was seen. This way if I put "cookie" and my eyes glance over thinking the "s" > is > > there, I wouldn't be surprised when I still see cookies around. > > We do handle that :) See the "Invalid type:" error message in > clear_site_data_throttle.cc, and the "passwords" example in the InvalidHeader > test below. > > But we do not interpret this as a failure. If there is at least one valid type > specified, we go forward. > > Note that this is good for compatibility. If we add a new datatype, old versions > of Chrome will not be able to delete it, but will still delete at least the ones > they know. And this is not hypothetical - the spec mentions "executionContexts" > which we don't have in this first version but we will have it in the future. Acknowledged. https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:565: throttles_to_register.push_back(clear_site_data_throttle.release()); On 2016/08/12 15:06:28, msramek wrote: > On 2016/08/11 20:07:22, nasko (slow) wrote: > > nit: std::move(clear_site_data_throttle) > > Done, also above from where I copy-pasted this. Thanks! https://codereview.chromium.org/2025683003/diff/760001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/760001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:257: clearing_in_progress_ = false; nit: DCHECK that it is true?
https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/720001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:126: while (handle->GetResponseHeaders()->EnumerateHeaderLines(&iter, &header_name, On 2016/08/12 21:44:53, nasko (slow) wrote: > On 2016/08/12 15:06:27, msramek wrote: > > On 2016/08/11 20:07:21, nasko (slow) wrote: > > > Do we want to allow multiple instances of the header? I'd suggest the spec > > > clarify that only one instance is expected and whether we honor the first(my > > > preference)/last when duplicates are encountered. This will allow for using > > > HasHeaderValue and avoiding lots of string copies, which the current > > > implementation will make. > > > > Done. > > > > It's true that currently there's no reason to have more than one instance, > since > > for example > > > > { 'types': ['cookies'] } > > { 'types': ['storage'] } > > > > can be merged to > > > > { 'types': ['cookies', 'storage'] } > > > > and for example > > > > { 'types': ['cookies'] } > > { 'types': ['cookies'] } > > > > is idempotent. > > > > Note that the original version of the spec didn't have this property, and that > > might change again in the future, but for v1, let's simplify it. > > > > It seems that EnumerateHeader(nullptr, ...) is what we want to use, but that > > also requires us to list the header in HttpUtil::IsNonCoalescingHeader() > > otherwise the JSON will be broken on commas. > > Doesn't GetNormalizedHeader work? It will likely also have the side effect of > not parsing correctly if there are multiple instances of it. Done. Ok, let's do it that way. We'll probably update the spec soon to formalize that only one instance is expected in which case GetNormalizedHeader() indeed works slightly better by failing to parse instead of silently ignoring other instances. Or we'll support multiple headers in v2 in which case I'll come back with a CL for EnumerateHeaders(). But in any case, we'll end up not using EnumerateHeader(). https://codereview.chromium.org/2025683003/diff/760001/content/browser/browsi... File content/browser/browsing_data/clear_site_data_throttle.cc (right): https://codereview.chromium.org/2025683003/diff/760001/content/browser/browsi... content/browser/browsing_data/clear_site_data_throttle.cc:257: clearing_in_progress_ = false; On 2016/08/12 21:44:54, nasko (slow) wrote: > nit: DCHECK that it is true? Done.
And Mike, you have already given your LGTM, but since I had to rewrite the tests in chrome_content_browser_client_unittest.cc as a result of https://codereview.chromium.org/2175703002, please have another look at that file! Also note that I'm not running trybots on this last patchset, because I managed to shoot myself in the foot again by landing the incompatible https://codereview.chromium.org/2221143003/ first. Will fix that ASAP.
Thanks for sticking through the review and being very responsive. LGTM!
\o/ Thanks Nasko! :)
Hey Mike - friendly ping :) Do you want to have another look at the changes in
chrome_content_browser_client_unittest.cc since you gave your original LGTM?
In the meantime, I have implemented the last remaining datatype on my checklist,
which is PLUGIN_DATA. I realized that Flash stores data per domain (not
necessarily eTLD+1). That means that origin scoping doesn't work here either. I
suggest putting it to the eTLD+1 bucket, so that we have consistency between
LSOs ("Flash cookies") and regular cookies. Does that make sense?
> Hey Mike - friendly ping :) Do you want to have another look at the changes in
chrome_content_browser_client_unittest.cc since you gave your original LGTM?
Still LGTM.
> In the meantime, I have implemented the last remaining datatype on my
checklist, which is PLUGIN_DATA. I realized that Flash stores data per domain
(not necessarily eTLD+1). That means that origin scoping doesn't work here
either. I suggest putting it to the eTLD+1 bucket, so that we have consistency
between LSOs ("Flash cookies") and regular cookies. Does that make sense?
Surprising, but not shocking. Treating plugin data like cookies SGTM.
Thanks Mike! In the last patchset, I moved REMOVE_PLUGIN_DATA to the eTLD+1 bucket. This CL is now ready to land - I'll do that as soon as I get plugins sorted out in BrowsingDataRemover.
Okay, the prerequisites should have been sorted out now. Let's try to land this :-)
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2025683003/#ps840001 (title: "REMOVE_PLUGIN_DATA for eTLD+1")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the navigation. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. Tests: - ClearSiteDataThrottleUnittest - parsing - error messages - ClearSiteDataThrottleBrowsertest - secure origins - redirects, - integration with the embedder - ChromeContentBrowserClientTest - removal parameters for BrowsingDataRemover Note that the effect of data being asynchronously deleted while possibly also written by the website in the same time has NOT yet been tested. BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the navigation. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. Tests: - ClearSiteDataThrottleUnittest - parsing - error messages - ClearSiteDataThrottleBrowsertest - secure origins - redirects, - integration with the embedder - ChromeContentBrowserClientTest - removal parameters for BrowsingDataRemover Note that the effect of data being asynchronously deleted while possibly also written by the website in the same time has NOT yet been tested. BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #28 (id:840001)
Message was sent while issue was closed.
Description was changed from ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the navigation. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. Tests: - ClearSiteDataThrottleUnittest - parsing - error messages - ClearSiteDataThrottleBrowsertest - secure origins - redirects, - integration with the embedder - ChromeContentBrowserClientTest - removal parameters for BrowsingDataRemover Note that the effect of data being asynchronously deleted while possibly also written by the website in the same time has NOT yet been tested. BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== First experimental implementation of the Clear-Site-Data header We add a ClearSiteDataThrottle to the list of ResourceThrottle-s on a main frame navigation. If the response contains a "Clear-Site-Data" header, ClearSiteDataThrottle parses it and calls BrowsingDataRemover to execute deletion with the specified parameters. The deletion is asynchronous and does not delay or block the navigation. This is an early proof-of-concept implementation of the working draft https://www.w3.org/TR/clear-site-data/ which does not yet comply with its full specification. Tests: - ClearSiteDataThrottleUnittest - parsing - error messages - ClearSiteDataThrottleBrowsertest - secure origins - redirects, - integration with the embedder - ChromeContentBrowserClientTest - removal parameters for BrowsingDataRemover Note that the effect of data being asynchronously deleted while possibly also written by the website in the same time has NOT yet been tested. BUG=607897 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/1f4746d285ff62f0747ef4ce00c72a74f91f92cb Cr-Commit-Position: refs/heads/master@{#413247} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/1f4746d285ff62f0747ef4ce00c72a74f91f92cb Cr-Commit-Position: refs/heads/master@{#413247} |
