Refactor multi-network CCT to rely on net::UrlRequest's target_network [chromium/src : main]
Adam Rice (Gerrit)
Adam Rice voted and added 4 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Commit-Queue | +1 |
4 comments
//services/network lgtm. I haven't reviewed the rest yet.
size_t count = 0;
for (const auto& factory : url_loader_factories_) {
if (factory->GetBoundNetworkForTesting() == target_network) {
count++;
}
}
return count;```suggestion
return std::ranges::count_if(url_local_factories_,
[](const auto& factory) {
return factory->GetBoundNetworkForTesting() == target_network;
});
```
// pipe to make sure we run through ConnectTerminal. This is necessary toI find this comment a little hard to understand.
#include "base/logging.h"Should have `"base/check_op.h"` as well.
Pedantically, `#include <ostream>` is also required, although somehow it never seems to cause a problem to omit it.
Related details
- Adam Rice
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
Intent intent =Gemini says this line needs clang-formatting.
Related details
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefano Duo (Gerrit)
Stefano Duo voted and added 5 comments![]( "Open in Gerrit")
Votes added by Stefano Duo
| Commit-Queue | +1 |
5 comments
Gemini says this line needs clang-formatting.
git cl format disagrees with Gemini :(
Too many blank lines left.
for (const auto& factory : url_loader_factories_) {
if (factory->GetBoundNetworkForTesting() == target_network) {
count++;
}
}
return count;```suggestion
return std::ranges::count_if(url_local_factories_,
[](const auto& factory) {
return factory->GetBoundNetworkForTesting() == target_network;
});
```
Done
// pipe to make sure we run through ConnectTerminal. This is necessary toI find this comment a little hard to understand.
I've reworded it and added more context. It should be easier to read.
Should have `"base/check_op.h"` as well.
Pedantically, `#include <ostream>` is also required, although somehow it never seems to cause a problem to omit it.
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefano Duo (Gerrit)
Stefano Duo added 1 comment![]( "Open in Gerrit")
creis@ for //content
peconn@ for //android_webview and //chrome/**/android
Related details
- Adam Rice
- Charlie Reis
- Chromium IPC Reviews
- Peter Conn
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: kin...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): kin...@chromium.org
Reviewer source(s):
kin...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Adam Rice
- Charlie Reis
- Kinuko Yasuda
- Peter Conn
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefano Duo (Gerrit)
Stefano Duo added 1 comment![]( "Open in Gerrit")
I'm shuffling reviewers after a quick offline chat with the current ones.
rakina@ for //content (I'm adding a new for-testing setter to further test multi-network CCT, see the original changes in https://crrev.com/c/5557142 and http://shortn/_oRFHXuNwjv)
meacer@ for services/network/public/mojom/network_context.mojom
Related details
- Adam Rice
- Mustafa Emre Acer
- Peter Conn
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Conn (Gerrit)
Peter Conn voted and added 1 comment![]( "Open in Gerrit")
Votes added by Peter Conn
| Code-Review | +1 |
1 comment
- Adam Rice
- Mustafa Emre Acer
- Rakina Zata Amni
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Code-Review | +1 |
1 comment
- Adam Rice
- Mustafa Emre Acer
- Stefano Duo
Adam Rice (Gerrit)
Adam Rice voted and added 9 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
9 comments
class TestChromeContentBrowserClient : public ChromeContentBrowserClient {To avoid violating the one-definition rule when linking in the same binary as chrome/browser/chrome_content_browser_client_unittest.cc, I suggest putting this inside an anonymous namespace.
for (net::handles::NetworkHandle h : it->second) {
if (h == network) {
return true;
}
}Nit: std::ranges::contains is nicer for readability.
```suggestion
return std::ranges::contains(it->second, network);
```
for (bool val : it->second) {
if (val == expected) {
return true;
}
}Readability nit:
```suggestion
return std::ranges::contains(it->second, true);
```
ChromeContentBrowserClient* client() { return client_; }Not used (which means `client_` is also not used, which means the definition of `MultiNetworkBrowserTest` can be replaced with
```
using MultiNetworkBrowserTest = PlatformBrowserTest;
```
std::ignore = content::EvalJs(web_contents.get(), "fetch('/title1.html')");It's probably good to require the fetch to succeed:
```suggestion
EXPECT_TRUE(content::EvalJs(web_contents.get(), "fetch('/title1.html')").is_ok());
```
std::string script = base::StringPrintf(
"fetch('%s', {mode: 'no-cors'}).then(() => 'success').catch(err => "
"'fail')",
fetch_url.spec().c_str());`JSReplace()` is a safer choice when interpolating into JavaScript.
```suggestion
const std::string script = JSReplace(
"fetch($1, {mode: 'no-cors'}).then(() => 'success').catch(err => "
"'fail')",
fetch_url.spec());
```
testing::IsNull(), testing::IsNull(), true));Nit: consider adding `/*is_for_network_service=*/` here and below.
testing::IsNull(), testing::IsNull(), false));Maybe add `/*is_for_network_service=*/` ?
Related details
- Mustafa Emre Acer
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Code-Review | +0 |
1 comment
Hello, sorry I wasn't aware there are still ongoing discussions on an internal thread! It looks like lukasza@ pointed out some interesting cases to at least test / document (and maybe triggering some fixing/refactor) around subframes / popups / all kinds of navigation requests. So let's make sure those are addressed, either in this CL or a follow-up CL.
(Let me also add Lukasz to review since he's been leading the discussions)
Related details
- Mustafa Emre Acer
- Stefano Duo
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustafa Emre Acer (Gerrit)
Mustafa Emre Acer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mustafa Emre Acer
| Code-Review | +1 |
1 comment
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefano Duo (Gerrit)
Stefano Duo added 9 comments![]( "Open in Gerrit")
Hello, sorry I wasn't aware there are still ongoing discussions on an internal thread! It looks like lukasza@ pointed out some interesting cases to at least test / document (and maybe triggering some fixing/refactor) around subframes / popups / all kinds of navigation requests. So let's make sure those are addressed, either in this CL or a follow-up CL.
(Let me also add Lukasz to review since he's been leading the discussions)
No problem! Resolving this as we've now agreed that WebContentsImpl is still the right place to store target_network and chrome/browser/multi_network_browser_test.cc contains a test for all the scenarios we're interested about.
class TestChromeContentBrowserClient : public ChromeContentBrowserClient {To avoid violating the one-definition rule when linking in the same binary as chrome/browser/chrome_content_browser_client_unittest.cc, I suggest putting this inside an anonymous namespace.
Done
for (net::handles::NetworkHandle h : it->second) {
if (h == network) {
return true;
}
}Nit: std::ranges::contains is nicer for readability.
```suggestion
return std::ranges::contains(it->second, network);
```
Done
for (bool val : it->second) {
if (val == expected) {
return true;
}
}Readability nit:
```suggestion
return std::ranges::contains(it->second, true);
```
Done
Not used (which means `client_` is also not used, which means the definition of `MultiNetworkBrowserTest` can be replaced with
```
using MultiNetworkBrowserTest = PlatformBrowserTest;
```
Done
std::ignore = content::EvalJs(web_contents.get(), "fetch('/title1.html')");It's probably good to require the fetch to succeed:
```suggestion
EXPECT_TRUE(content::EvalJs(web_contents.get(), "fetch('/title1.html')").is_ok());
```
Done
std::string script = base::StringPrintf(
"fetch('%s', {mode: 'no-cors'}).then(() => 'success').catch(err => "
"'fail')",
fetch_url.spec().c_str());`JSReplace()` is a safer choice when interpolating into JavaScript.
```suggestion
const std::string script = JSReplace(
"fetch($1, {mode: 'no-cors'}).then(() => 'success').catch(err => "
"'fail')",
fetch_url.spec());
```
Done
Nit: consider adding `/*is_for_network_service=*/` here and below.
Done
testing::IsNull(), testing::IsNull(), false));Stefano DuoMaybe add `/*is_for_network_service=*/` ?
Done
Related details
- Adam Rice
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz added 6 comments![]( "Open in Gerrit")
for (auto type : kAllURLLoaderFactoryTypes) {Minor nit / suggestion to make it easier to see from the test logs which `type` caused test assertion failures:
```suggestion
for (auto type : kAllURLLoaderFactoryTypes) {
SCOPED_TRACE(testing::Message()
<< std::endl
<< "type = " << type << std::endl;
```
VerifyFactoryCounts(test_client, network,
{.navigation = 1, .document_subresource = 2});It seems unexpected that lines 258-259 in `MultiNetworkBrowserTest.NavigationSetsTargetNetwork` have the exact same expectations as lines 295-296 here in `MultiNetworkBrowserTest.SubresourceSetsTargetNetwork`. Is this because network cache kicks-in? If so, should the test work around that by fetching a different subresource?
Also:
1. Maybe it would be a bit clearer what is covered by a test if the `test_client` could reset its request counts. Resetting just before `fetch` would make it clean that we only count network requests stemming from `fetch` (and ignoring earlier ones).
2. This question also applies to other tests below (e.g. `PrefetchSetsTargetNetwork` also has similar test expectations at the end)
}Can you please also add a separate new test that covers the `window.open` API? (IIRC it goes through a separate code path from `OpenURL`. And I think we've said that we want `window.open` to fall back to the default / un-bound network - hypothesizing that this may help SSO scenarios)
ServiceWorkerSetsTargetNetwork) {nit:
```suggestion
ServiceWorkerIgnoresTargetNetwork) {
```
IN_PROC_BROWSER_TEST_F(MultiNetworkBrowserTest, WebSocketSetsTargetNetwork) {```suggestion
IN_PROC_BROWSER_TEST_F(MultiNetworkBrowserTest, WebSocketIgnoresTargetNetwork) {
```
WebRTCPeerConnectionSetsTargetNetwork) {```suggestion
WebRTCPeerConnectionIgnoresTargetNetwork) {
```
Should this say "Ignores"? OTOH I am not sure about this suggestion because I don't quite understand why many of the new tests use the same `VerifyFactoryCounts` expectations.
Related details
- Adam Rice
- Rakina Zata Amni
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
Removing myself from the attention set. Add me back when you're ready.
Related details
Stefano Duo (Gerrit)
Stefano Duo added 6 comments![]( "Open in Gerrit")
Minor nit / suggestion to make it easier to see from the test logs which `type` caused test assertion failures:
```suggestion
for (auto type : kAllURLLoaderFactoryTypes) {
SCOPED_TRACE(testing::Message()
<< std::endl
<< "type = " << type << std::endl;
```
Done
VerifyFactoryCounts(test_client, network,
{.navigation = 1, .document_subresource = 2});It seems unexpected that lines 258-259 in `MultiNetworkBrowserTest.NavigationSetsTargetNetwork` have the exact same expectations as lines 295-296 here in `MultiNetworkBrowserTest.SubresourceSetsTargetNetwork`. Is this because network cache kicks-in? If so, should the test work around that by fetching a different subresource?
Also:
1. Maybe it would be a bit clearer what is covered by a test if the `test_client` could reset its request counts. Resetting just before `fetch` would make it clean that we only count network requests stemming from `fetch` (and ignoring earlier ones).
2. This question also applies to other tests below (e.g. `PrefetchSetsTargetNetwork` also has similar test expectations at the end)
Fetches for subresources re-use the single UrlLoaderFactory created for subresources during navigation commit time (I believe this is done for performance reasons, similarly to how there is a single SharedURLLoaderFactory for the browser process?).
With that in mind, we do expect to see the same number of for `document_subresource` across the various tests. Except for tests that triggers additional navigations, where we generally expect document_subresource = 2 * # of navigations.
Can you please also add a separate new test that covers the `window.open` API? (IIRC it goes through a separate code path from `OpenURL`. And I think we've said that we want `window.open` to fall back to the default / un-bound network - hypothesizing that this may help SSO scenarios)
Done. Though, we do want the target_network to be propagated (and this is what currently happens).
ServiceWorkerSetsTargetNetwork) {Stefano Duonit:
```suggestion
ServiceWorkerIgnoresTargetNetwork) {
```
Done
IN_PROC_BROWSER_TEST_F(MultiNetworkBrowserTest, WebSocketSetsTargetNetwork) {Stefano Duo```suggestion
IN_PROC_BROWSER_TEST_F(MultiNetworkBrowserTest, WebSocketIgnoresTargetNetwork) {
```
Done
```suggestion
WebRTCPeerConnectionIgnoresTargetNetwork) {
```Should this say "Ignores"? OTOH I am not sure about this suggestion because I don't quite understand why many of the new tests use the same `VerifyFactoryCounts` expectations.
- Rakina Zata Amni
- Łukasz Anforowicz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Łukasz Anforowicz (Gerrit)
Łukasz Anforowicz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Łukasz Anforowicz
| Code-Review | +1 |
1 comment
VerifyFactoryCounts(test_client, network,
{.navigation = 1, .document_subresource = 2});Stefano DuoIt seems unexpected that lines 258-259 in `MultiNetworkBrowserTest.NavigationSetsTargetNetwork` have the exact same expectations as lines 295-296 here in `MultiNetworkBrowserTest.SubresourceSetsTargetNetwork`. Is this because network cache kicks-in? If so, should the test work around that by fetching a different subresource?
Also:
1. Maybe it would be a bit clearer what is covered by a test if the `test_client` could reset its request counts. Resetting just before `fetch` would make it clean that we only count network requests stemming from `fetch` (and ignoring earlier ones).
2. This question also applies to other tests below (e.g. `PrefetchSetsTargetNetwork` also has similar test expectations at the end)
Fetches for subresources re-use the single UrlLoaderFactory created for subresources during navigation commit time (I believe this is done for performance reasons, similarly to how there is a single SharedURLLoaderFactory for the browser process?).
With that in mind, we do expect to see the same number of for `document_subresource` across the various tests. Except for tests that triggers additional navigations, where we generally expect document_subresource = 2 * # of navigations.
Fetches for subresources re-use the single UrlLoaderFactory created for subresources during navigation commit time (I believe this is done for performance reasons, similarly to how there is a single SharedURLLoaderFactory for the browser process?).
With that in mind, we do expect to see the same number of for `document_subresource` across the various tests. Except for tests that triggers additional navigations, where we generally expect document_subresource = 2 * # of navigations.
Thanks for explaining and expanding the comments. I was confused before and now I understand that the verified/expected numbers are tracking the number of URLLoaderFactories, rather than where/how individual requests are handled. This means that we're not quite verifying exactly what we need, but I don't know of a better way to do this, so maybe this is good enough.
Related details
- Rakina Zata Amni
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefano Duo (Gerrit)
Stefano Duo added 1 comment![]( "Open in Gerrit")
Removing myself from the attention set. Add me back when you're ready.
- Adam Rice
- Rakina Zata Amni
Rakina Zata Amni (Gerrit)
Rakina Zata Amni voted and added 2 comments![]( "Open in Gerrit")
Votes added by Rakina Zata Amni
| Code-Review | +1 |
LGTM again, thanks!
void set_target_network_for_testing(net::handles::NetworkHandle network) {
target_network_ = network;
}Let's move this somewhere else away from the RenderViewHostDelegate overrides, closer to other `for_testing` methods.
Related details
- Adam Rice
- Stefano Duo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefano Duo (Gerrit)
Stefano Duo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stefano Duo
| Commit-Queue | +1 |
1 comment
void set_target_network_for_testing(net::handles::NetworkHandle network) {
target_network_ = network;
}Let's move this somewhere else away from the RenderViewHostDelegate overrides, closer to other `for_testing` methods.
Done
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stefano Duo (Gerrit)
Stefano Duo added 1 comment![]( "Open in Gerrit")
I'm shuffling reviewers after a quick offline chat with the current ones.
rakina@ for //content (I'm adding a new for-testing setter to further test multi-network CCT, see the original changes in https://crrev.com/c/5557142 and http://shortn/_oRFHXuNwjv)
meacer@ for services/network/public/mojom/network_context.mojom
victorvianna@ for adding chrome/browser/multi_network_browser_test.cc and updating chrome/browser/OWNERS
Related details
- Adam Rice
- Victor Vianna
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |