Implement debouncing #9065
Implement debouncing #9065
Conversation
| request->trusted_params = network::ResourceRequest::TrustedParams(); | ||
| request->trusted_params->isolation_info = net::IsolationInfo::Create( | ||
| net::IsolationInfo::RequestType::kOther, | ||
| debounced_site_for_cookies_origin, debounced_site_for_cookies_origin, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
|
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. |
pilgrim-brave
force-pushed
the
mpilgrim_debounce
branch
from
5dd0ba8
to
6887166
Compare
pilgrim-brave
force-pushed
the
mpilgrim_debounce
branch
from
6887166
to
f6dd876
Compare
pilgrim-brave
force-pushed
the
mpilgrim_debounce
branch
2 times, most recently
from
ebe6e56
to
0f2c732
Compare
pilgrim-brave
force-pushed
the
mpilgrim_debounce
branch
from
0f2c732
to
4aa1a0b
Compare
pilgrim-brave
force-pushed
the
mpilgrim_debounce
branch
from
51e4fbb
to
eef8c2b
Compare
| weak_factory_.GetWeakPtr())); | ||
| } | ||
|
|
||
| void DebounceComponentInstaller::OnDATFileDataReady( |
There was a problem hiding this comment.
why are we doing this on UI thread? why not parse everything in a threadpool and return ready to use data?
| request->trusted_params = network::ResourceRequest::TrustedParams(); | ||
| request->trusted_params->isolation_info = net::IsolationInfo::Create( | ||
| net::IsolationInfo::RequestType::kOther, debounced_origin, | ||
| debounced_origin, net::SiteForCookies::FromOrigin(debounced_origin)); |
There was a problem hiding this comment.
won't this create an issue when we redirect some 3rd party frame and it will be allowed to use 1p cookies?
There was a problem hiding this comment.
@goodov we discussed this in a previous version of the code and trying to figure out what it should be here
pilgrim-brave
force-pushed
the
mpilgrim_debounce
branch
4 times, most recently
from
781fa8f
to
c790771
Compare
pilgrim-brave
force-pushed
the
mpilgrim_debounce
branch
6 times, most recently
from
39f67bb
to
a07e0cc
Compare
pilgrim-brave
force-pushed
the
mpilgrim_debounce
branch
9 times, most recently
from
134c382
to
d4c9fe0
Compare
pilgrim-brave
force-pushed
the
mpilgrim_debounce
branch
from
d4c9fe0
to
f447820
Compare
Resolves brave/brave-browser#15090
Security review at https://github.com/brave/security/issues/478.
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: