Added static list of pervasive resources along with an expiration time [chromium/src : main]
Patrick Meenan (Gerrit)
Patrick Meenan added 1 comment![]( "Open in Gerrit")
ricea@ could you PTAL?
dtapuska@ could you look at the file path change in content/browser/DEPS?
Thanks.
Related details
- Adam Rice
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 14 comments![]( "Open in Gerrit")
// pervasive script.Maybe change "script" to reflect that other types are supported now?
The lust or URLPatterns includes an expiration date for when the list is no longer considered "fresh" and should be ignored.```suggestion
The list of URLPatterns includes an expiration date for when the list is no longer considered "fresh" and should be ignored.
```
LoadPervasivePatterns(raw_patterns_, raw_patterns_expiration_);Maybe consider lazily loading the patterns the first time a script, style or dictionary is requested? Just to defer the cost until a time when we are slightly less busy.
std::stringstream ss(patterns);`std::stringstream` is famously inefficient. I'd prefer to use `base::SplitString()`.
if (pattern_as_url.is_valid()) {Now that the patterns are built-in,
```
CHECK(pattern_as_url.is_valid());
```
would be better.
if (pattern->is_valid()) {```
CHECK(pattern->is_valid());
```
if (now > patterns_expiration_) {Maybe just check this on load? People should be restarting their browsers every week or two to get security updates anyway.
// Only allow script, style and dictionary destinations.If you're going to do the expiry check every time, maybe put this check before the expiry check, to avoid the overhead of looking up the current time for documents and images?
// Copyright 2026 The Chromium AuthorsI suggest making this a header file that is only included by `shared_resource_checker.cc` instead of a `.cc` file. This has a few benefits:
1. The constants will no longer need to be member variables of SharedResourceChecker. You can just make them globals in the `network::internal` namespace.
2. You can make them `inline constexpr`, which may enable the compiler to perform additional optimisations.
3. The implementation of SharedResourceChecker will no longer be split over multiple `.cc` files.
// https://github.com/pmeenan/pervasiveMeta-nit: Should be on a Google-owned repository in case you get hit by a bus.
const char SharedResourceChecker::raw_patterns_[] =(Not for this CL) Once creation of the list is automated, consider compressing it with zstd to reduce binary size. See the call to `ZSTD_decompress()` in components/variations/seed_reader_writer.cc for example.
if (enabled_) {
scoped_feature_list_.InitAndEnableFeature(
features::kCacheSharingForPervasiveResources);
} else {
scoped_feature_list_.InitAndDisableFeature(
features::kCacheSharingForPervasiveResources);
}```suggestion
scoped_feature_list_.InitWithFeatureState(
features::kCacheSharingForPervasiveResources, enabled_);
```
// When enabled, fetches for "pervasive" scripts that match one of theMaybe change this comment to reflect that additional types are covered now?
"CacheSharingForPervasiveScripts": [Shouldn't you add an entry to enable CacheSharingForPervasiveResources at the same time?
Related details
- Dave Tapuska
- Patrick Meenan
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. |
Patrick Meenan (Gerrit)
Patrick Meenan added 15 comments![]( "Open in Gerrit")
Thanks Adam, great feedback - hopefully all addressed.
thakis@, could you PTAL at tools/pervasive_resources? It is pulling in the script that is used to automate the generation of the URL patterns for the pervasive resources from the HTTP Archive dataset.
Maybe change "script" to reflect that other types are supported now?
Done
The lust or URLPatterns includes an expiration date for when the list is no longer considered "fresh" and should be ignored.```suggestion
The list of URLPatterns includes an expiration date for when the list is no longer considered "fresh" and should be ignored.
```
Done
LoadPervasivePatterns(raw_patterns_, raw_patterns_expiration_);Maybe consider lazily loading the patterns the first time a script, style or dictionary is requested? Just to defer the cost until a time when we are slightly less busy.
Done, thanks. I shuffled the checks as well so that all of the trivial checks to exclude resources will be done so it only loads on the first script, style or dictionary that doesn't use query params and only if cookies are allowed.
`std::stringstream` is famously inefficient. I'd prefer to use `base::SplitString()`.
Done
Now that the patterns are built-in,
```
CHECK(pattern_as_url.is_valid());
```
would be better.
Done
if (pattern->is_valid()) {Patrick Meenan```
CHECK(pattern->is_valid());
```
Done
Maybe just check this on load? People should be restarting their browsers every week or two to get security updates anyway.
Done. I originally had it that way but was worried about kiosks and similar long-running cases. That said, the list is good for a year so if they haven't updated in that long, they have other problems (and the failure mode of an out-of-date list is still safe, just slightly more risk of history sniffing being possible).
If you're going to do the expiry check every time, maybe put this check before the expiry check, to avoid the overhead of looking up the current time for documents and images?
Moved the time check but I also rearranged all of the checks to make the really fast ones first.
I suggest making this a header file that is only included by `shared_resource_checker.cc` instead of a `.cc` file. This has a few benefits:
1. The constants will no longer need to be member variables of SharedResourceChecker. You can just make them globals in the `network::internal` namespace.
2. You can make them `inline constexpr`, which may enable the compiler to perform additional optimisations.
3. The implementation of SharedResourceChecker will no longer be split over multiple `.cc` files.
Done
Meta-nit: Should be on a Google-owned repository in case you get hit by a bus.
Rick and Nico recommended just adding it to the /tools directly in the Chrome codebase so I pulled it in.
(Not for this CL) Once creation of the list is automated, consider compressing it with zstd to reduce binary size. See the call to `ZSTD_decompress()` in components/variations/seed_reader_writer.cc for example.
This will be an interesting tradeoff with transparency with the list's contents (given the feature gets a fair bit of scrutiny). In that case it might work to store the plain-text version of the list in the repository next to the compressed version and make sure both are updated when it is changed.
if (enabled_) {
scoped_feature_list_.InitAndEnableFeature(
features::kCacheSharingForPervasiveResources);
} else {
scoped_feature_list_.InitAndDisableFeature(
features::kCacheSharingForPervasiveResources);
}```suggestion
scoped_feature_list_.InitWithFeatureState(
features::kCacheSharingForPervasiveResources, enabled_);```
Done
// When enabled, fetches for "pervasive" scripts that match one of theMaybe change this comment to reflect that additional types are covered now?
Done
"CacheSharingForPervasiveScripts": [Shouldn't you add an entry to enable CacheSharingForPervasiveResources at the same time?
- Adam Rice
- Dave Tapuska
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nico Weber
| Code-Review | +1 |
1 comment
- Adam Rice
- Dave Tapuska
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Adam Rice
- Patrick Meenan
Adam Rice (Gerrit)
Adam Rice voted and added 2 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
2 comments
lgtm, but I haven't actually reviewed the Python yet. I might not get to that until Monday. If you want to get this landed before then to avoid more rebase pain that is fine. I'm happy to review the Python after-the-fact since it is not in the critical path.
const char SharedResourceChecker::raw_patterns_[] =Patrick Meenan(Not for this CL) Once creation of the list is automated, consider compressing it with zstd to reduce binary size. See the call to `ZSTD_decompress()` in components/variations/seed_reader_writer.cc for example.
This will be an interesting tradeoff with transparency with the list's contents (given the feature gets a fair bit of scrutiny). In that case it might work to store the plain-text version of the list in the repository next to the compressed version and make sure both are updated when it is changed.
Ah, that is true. You could literally put the uncompressed list in a comment in this file.
Related details
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Meenan (Gerrit)
Patrick Meenan added 1 comment![]( "Open in Gerrit")
lgtm, but I haven't actually reviewed the Python yet. I might not get to that until Monday. If you want to get this landed before then to avoid more rebase pain that is fine. I'm happy to review the Python after-the-fact since it is not in the critical path.
No rush. Rebase is trivial (most of the changes are in dedicated files and those that aren't are in parts of files that don't change frequently).
I'll add the support for zstd-compressing the patterns so we have everything already done (and I don't expect it will be difficult - famous last words).
Feel free to start looking at the python logic or wait until I let you know that the zstd compression has been integrated - either works since the core logic won't be changing.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Meenan (Gerrit)
Patrick Meenan added 2 comments![]( "Open in Gerrit")
I took a run at zstd-compressing the strings but it blew out the binary size on Fuchsia and saved ~11k on Android APK size.
I could do platform-specific implementations and carve-out Fuchsia but I'm not sure the added complexity is worth the size difference on Android.
const char SharedResourceChecker::raw_patterns_[] =Patrick Meenan(Not for this CL) Once creation of the list is automated, consider compressing it with zstd to reduce binary size. See the call to `ZSTD_decompress()` in components/variations/seed_reader_writer.cc for example.
Adam RiceThis will be an interesting tradeoff with transparency with the list's contents (given the feature gets a fair bit of scrutiny). In that case it might work to store the plain-text version of the list in the repository next to the compressed version and make sure both are updated when it is changed.
Ah, that is true. You could literally put the uncompressed list in a comment in this file.
It looks like the network service didn't bundle zstd before and adding it increases the fuchsia binary size by 64kb (28kb uncompressed) and reduces the Android APK size change from 13k to 2k (saved about 11k).
At this point, I think the best path is to pull in the strings uncompressed and then at some future point to look at compressing a lot of strings (including these) when the aggregate savings is more than the size of zstd.
I could use raw strings on Fuchsia and compressed elsewhere (or disable the feature entirely on Fuchsia) but I'm not sure the added complexity is worth the 11k difference on Android.
Related details
- Adam Rice
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. |
Patrick Meenan (Gerrit)
Patrick Meenan added 2 comments![]( "Open in Gerrit")
Going ahead with the as-reviewed code and will follow-up with a CL for zstd-compressing the list (and working through platform-specific size/library issues) and any follow-up for the python utility.
const char SharedResourceChecker::raw_patterns_[] =Patrick Meenan(Not for this CL) Once creation of the list is automated, consider compressing it with zstd to reduce binary size. See the call to `ZSTD_decompress()` in components/variations/seed_reader_writer.cc for example.
Adam RiceThis will be an interesting tradeoff with transparency with the list's contents (given the feature gets a fair bit of scrutiny). In that case it might work to store the plain-text version of the list in the repository next to the compressed version and make sure both are updated when it is changed.
Patrick MeenanAh, that is true. You could literally put the uncompressed list in a comment in this file.
It looks like the network service didn't bundle zstd before and adding it increases the fuchsia binary size by 64kb (28kb uncompressed) and reduces the Android APK size change from 13k to 2k (saved about 11k).
At this point, I think the best path is to pull in the strings uncompressed and then at some future point to look at compressing a lot of strings (including these) when the aggregate savings is more than the size of zstd.
I could use raw strings on Fuchsia and compressed elsewhere (or disable the feature entirely on Fuchsia) but I'm not sure the added complexity is worth the 11k difference on Android.
Going with your original suggestion of separating the zstd-compression of the list into a separate CL.
Related details
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Meenan (Gerrit)
Patrick Meenan added 1 comment![]( "Open in Gerrit")
Adam, could you re-stamp the review bit on this? It's the same code as before the zstd-compressing experiment (patchset 9) but the experiment (patch set 10) introduced a new deps file for zstd that looks like it reset the code review votes (but not owners votes - bizarre) even though it was removed in patch set 11.
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. |
Patrick Meenan (Gerrit)
Patrick Meenan voted and added 1 comment![]( "Open in Gerrit")
Votes added by Patrick Meenan
| Commit-Queue | +1 |
1 comment
Sorry for the spam - hopefully this is it.
Adam, could you PTAL?
This adds the zstd compression of the pervasive patterns back in and disables the feature for Fuchsia (where the web_engine doesn't partition anyway). The net result is a smaller size increase on Android (2k), no increase to the Fuchsia size and no need to revisit later.
The pervasive.py script automatically generates the header with both the uncompressed and compressed versions of the list.
The uncompressed version of the list is in line comments because the patterns include */ in them and it was cleaner to prefix each line than to escape the pattern strings in a block comment.
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. |
Zijie He (Gerrit)
Zijie He voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zijie He
| Code-Review | +1 |
1 comment
Thank you Patrick to reach out.
64KB increment of fuchsia binary size is acceptable. You may use
```
Fuchsia-Binary-Size: An explanation
```
to bypass the fuchsia-binary-size error if excluding the feature from fuchsia would introduce a fuchsia-specific workflow / build rule, i.e. https://chromium.googlesource.com/chromium/src/+/HEAD/docs/speed/binary_size/fuchsia_binary_size_trybot.md
Related details
- Adam Rice
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 1 comment![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
1 comment
- Patrick Meenan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Meenan (Gerrit)
Patrick Meenan voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Added static list of pervasive resources along with an expiration time
The core change in this CL was to add the actual list of URLPatterns
for the matching of pervasive resources. The list itself is in a
stand-alone file that can be automatically generated
(shared_resource_checker_patterns.cc).
Other changes that go along with the adding of the list:
- The files were moved into a pervasive_resources folder.
- A README.md with details on the feature and list generation was added.
- Support for the pattern list to expire.
- The feature name was changed to CacheSharingForPervasiveResources.
- A test was added for testing list expiration.
- Style and Dictionary destinations were enabled (in addition to the existing Script destination)
Most of the changes in the CL are from moving the files.
DISABLE_SPELLCHECKER
- M android_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java
- M content/browser/DEPS
- M content/browser/loader/navigation_url_loader_impl_unittest.cc
- M services/network/BUILD.gn
- M services/network/network_context.cc
- M services/network/network_context.h
- A services/network/pervasive_resources/DEPS
- A services/network/pervasive_resources/OWNERS
- A services/network/pervasive_resources/README.md
- A services/network/pervasive_resources/shared_resource_checker.cc
- R services/network/pervasive_resources/shared_resource_checker.h
- A services/network/pervasive_resources/shared_resource_checker_patterns.h
- R services/network/pervasive_resources/shared_resource_checker_unittest.cc
- M services/network/public/cpp/features.cc
- M services/network/public/cpp/features.h
- D services/network/shared_resource_checker.cc
- M services/network/url_loader.cc
- M services/network/url_loader_unittest.cc
- M services/network/url_loader_util.cc
- M testing/variations/fieldtrial_testing_config.json
- A tools/pervasive_resources/OWNERS
- A tools/pervasive_resources/README.md
- A tools/pervasive_resources/pervasive.py
- A tools/pervasive_resources/shared_resource_checker_patterns.template
Code-Review: +1 by Zijie He, +1 by Adam Rice
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |