Implement debouncing

[pilgrim-brave]

Resolves brave/brave-browser#15090

Security review at https://github.com/brave/security/issues/478.

Submitter Checklist:

• I confirm that no security/privacy review is needed, or that I have requested one
• There is a ticket for my issue
• Used Github auto-closing keywords in the PR description above
• Wrote a good PR/commit description
• Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
• Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
• Ran git rebase master (if needed)

Reviewer Checklist:

• A security review is not needed, or a link to one is included in the PR description
• New files have MPL-2.0 license header
• Adequate test coverage exists to prevent regressions
• Major classes, functions and non-trivial code blocks are well-commented
• Changes in component dependencies are properly reflected in gn
• Code follows the style guide
• Test plan is specified in PR before merging

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on
• All relevant documentation has been updated, for instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protections
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-Compatibility-Exceptions-in-Brave
  • https://github.com/brave/brave-browser/wiki/QA-Guide
  • https://github.com/brave/brave-browser/wiki/P3A

Test Plan:

[bridiver]

I'm not sure this is what we want because SiteForCookies is normally the original site to prevent 3rd-party redirects from settings cookies as 1p. I think we want to carry through the original info here, but double checking with @iefremov

[iefremov]

I have the same feeling. Would be great to check some real world examples and code this scenario in tests. So if I understand correctly, we skipped the tracker in between and proceed to a legitimate destination. We probably should keep site for cookies etc as it was originally?

[pilgrim-brave]

As far as I can tell, this is the correct behavior. Any attempt to carry over the original info triggers a hard CHECK in url_loader.cc

[3586:12547:0712/135032.682268:FATAL:url_loader.cc(592)] Check failed: url_request_->isolation_info().site_for_cookies().IsEquivalent( request.site_for_cookies).

[fmarier]

This subsumes the query string filter, which is not related to debouncing, but it does bring a more flexible list format for that feature (and resolves brave/brave-browser#10188) which is great.

Unfortunately, an approach based on parsing the query string and writing it out again doesn't work for the general case as I noted in the original PR for the query filter (i.e. the format of the query string is not defined). Regular expressions are dirty and error-prone, but unfortunately there is no right way to parse a query string, other than one generated by a URL-encoded HTML form submission.

In particular, I wouldn't be surprised if these deleted tests cases no longer pass.

Using url::ExtractQueryKeyValue() should work fine for the other actions since those rules are specific to certain websites and therefore don't need to work for all possible websites (such as those that were written prior to the new WHATWG URL spec).

[fmarier]

It seems like there is another difference between the query string filtering done in this PR versus the current one: it no longer applies to non-top-level requests.

At the moment, the tracking parameters are removed from all URLs, regardless of the type of request (e.g. it includes sub-resource requests). This is a reduction in the scope of this filter, though it's hard to tell how much of an impact it would have.

[goodov]

why are we doing this on UI thread? why not parse everything in a threadpool and return ready to use data?

[goodov]

won't this create an issue when we redirect some 3rd party frame and it will be allowed to use 1p cookies?

[bridiver]

@goodov we discussed this in a previous version of the code and trying to figure out what it should be here

[pes10k]

I am happy to approve, based on Slack discussions about the implemented policy, but I do not think i have the C++ skills to give a close technical review