[persistent_cache] Support source-keyed code cache on browser process [chromium/src : main]
Takashi Nakayama (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Christian Dullweber
- Chromium IPC Reviews
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- 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. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: toyo...@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): toyo...@chromium.org
Reviewer source(s):
toyo...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Christian Dullweber
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 4 comments![]( "Open in Gerrit")
mojo_base::BigBuffer data) override {}Can you comment why this does nothing?
mojo_base::BigBuffer data) override {Can you comment why this is not permitted (and why disallowed calls could happen)?
CHECK_EQ(source_hash.size(), 32u);Can you have a comment that says this CHECK is safe as this vector size is ensured in the mojo binding, as this is declared as array<uint8, 32>.
Let's explain what's this magic is, e.g. why this is needed, and why this magic name is fine.
Also, I think it's robust to modify the original GeneratedCodeCache::GetContextKey() so that we can have a single key generator for the ContextKey. Otherwise, this kind of wrapper causes confusion on creating a consistent key for all users in the chromium.
For instance, if we follow the key name rules in the original method, you may use kSeparator (" \n") to append your specific new prefix, or suffix might be recommended?
Related details
- Christian Dullweber
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Nakayama
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 4 comments![]( "Open in Gerrit")
mojo_base::BigBuffer data) override {}Can you comment why this does nothing?
I think we don't have to add any comments here to leave it blank because this is a part of `NoopCodeCacheHost`.
mojo_base::BigBuffer data) override {Can you comment why this is not permitted (and why disallowed calls could happen)?
Done
CHECK_EQ(source_hash.size(), 32u);Can you have a comment that says this CHECK is safe as this vector size is ensured in the mojo binding, as this is declared as array<uint8, 32>.
Done
Let's explain what's this magic is, e.g. why this is needed, and why this magic name is fine.
Also, I think it's robust to modify the original GeneratedCodeCache::GetContextKey() so that we can have a single key generator for the ContextKey. Otherwise, this kind of wrapper causes confusion on creating a consistent key for all users in the chromium.
For instance, if we follow the key name rules in the original method, you may use kSeparator (" \n") to append your specific new prefix, or suffix might be recommended?
@g...@chromium.org Do you have any insights on the format of `ContextKey`? I'm not completely confident on this topic.
Related details
- Christian Dullweber
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 10 comments![]( "Open in Gerrit")
* Prepend an `_sk_` prefix to source-keyed cache IDs within
`CodeCacheWithPersistentCacheHost` to ensure proper isolation and prevent collisions.
* Adds `blink::ComposeSourceKeyedCacheKey()` to `code_cache_util.h` to consistently compute cache keys. This method will also be used by the renderer-side implementation of InlineScriptCache.maybe reformat is required? (for <80)
https://docs.google.com/document/d/1MeUZFfk5ykGrXZoPrFEPLQt6J9XyJE3wSQOPjuiMBTM/edit?tab=t.0#heading=h.66s3tkt0hlyCan we have a public version that contains the essential design part?
mojo_base::BigBuffer data) override {}Takashi NakayamaCan you comment why this does nothing?
I think we don't have to add any comments here to leave it blank because this is a part of `NoopCodeCacheHost`.
Ah, I see. I overlooked it.
DCHECK_EQ(source_hash.size(), 32u);Prefer CHECK over DCHECK if you want to have this check here.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md
// source-keyed cache do not support WebUI.But you may still need to check if the caller renderer is locked to a network origin?
std::string resource_key = blink::ComposeSourceKeyedCacheKey(source_hash);nit: const std::string
std::string resource_key = blink::ComposeSourceKeyedCacheKey(source_hash);nit: const
std::array<uint8_t, 32> source_hash = {
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32};can we use an actual sha256(data_str) here rather than a crafted magic numbers?
std::array<uint8_t, 32> source_hash = {
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32};ditto
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32};ditto
Related details
- Christian Dullweber
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Nakayama
Greg Thompson (Gerrit)
Greg Thompson added 6 comments![]( "Open in Gerrit")
* Prepend an `_sk_` prefix to source-keyed cache IDs within
`CodeCacheWithPersistentCacheHost` to ensure proper isolation and prevent collisions.
* Adds `blink::ComposeSourceKeyedCacheKey()` to `code_cache_util.h` to consistently compute cache keys. This method will also be used by the renderer-side implementation of InlineScriptCache.maybe reformat is required? (for <80)
DCHECK_EQ(source_hash.size(), 32u);Prefer CHECK over DCHECK if you want to have this check here.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md
I think it's fair to say that if `source_hash` isn't 32, the renderer must be misbehaving. In this case, I think `mojo::ReportBadMessage` is the right way to handle it rather than crashing the browser process.
// source-keyed cache do not support WebUI.But you may still need to check if the caller renderer is locked to a network origin?
Please check with Chrome Security on this. Maybe we only want to cache inline scripts for renderers that are locked to http(s) sites. Do we use inline scripts in WebUI?
std::string resource_key = blink::ComposeSourceKeyedCacheKey(source_hash);nit: const std::string
As per my other comment: keys are opaque binary blobs now, so this could be `base::HeapArray<uint8_t>` to avoid needing to stringify the hash digest.
Takashi NakayamaLet's explain what's this magic is, e.g. why this is needed, and why this magic name is fine.
Also, I think it's robust to modify the original GeneratedCodeCache::GetContextKey() so that we can have a single key generator for the ContextKey. Otherwise, this kind of wrapper causes confusion on creating a consistent key for all users in the chromium.
For instance, if we follow the key name rules in the original method, you may use kSeparator (" \n") to append your specific new prefix, or suffix might be recommended?
@g...@chromium.org Do you have any insights on the format of `ContextKey`? I'm not completely confident on this topic.
`GetCacheId` ensures a distinct backend/database for each type of data. Can we not use a single backend for bytecode from both normal JS and inline JS? `blink::UrlToCodeCacheKey` adds a fixed prefix to URLs to generate a key in the cache given a URL. A corresponding `blink::SourceHashToCodeCacheKey` that added a distinct fixed prefix to source hashes should be sufficient to ensure that it's not possible to manufacture a collision between these two types in the same database.
If we can use the same backend, we don't need to change `GetPendingBackend`.
return base::Base64Encode(source_hash);the `key` for PersistentCache is an opaque blob of bytes as of https://crrev.com/1590928, so there's no longer a need to stringify the hash.
Related details
- Christian Dullweber
Takashi Nakayama (Gerrit)
Takashi Nakayama added 4 comments![]( "Open in Gerrit")
* Prepend an `_sk_` prefix to source-keyed cache IDs within
`CodeCacheWithPersistentCacheHost` to ensure proper isolation and prevent collisions.
* Adds `blink::ComposeSourceKeyedCacheKey()` to `code_cache_util.h` to consistently compute cache keys. This method will also be used by the renderer-side implementation of InlineScriptCache.Greg Thompsonmaybe reformat is required? (for <80)
72 cols as per https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#uploading-a-change-for-review :-)
Can we have a public version that contains the essential design part?
I will create the public version and get back to you soon.
DCHECK_EQ(source_hash.size(), 32u);Greg ThompsonPrefer CHECK over DCHECK if you want to have this check here.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md
I think it's fair to say that if `source_hash` isn't 32, the renderer must be misbehaving. In this case, I think `mojo::ReportBadMessage` is the right way to handle it rather than crashing the browser process.
I want to confirm if mojo IPC does `mojo::ReportBadMessage` if the length doesn't match the mojo definition of `array<uint8, 32> script_hash`. If yes, I will keep this as `CHECK`; otherwise, I will change this line to `mojo::ReportBadMessage()`.
Takashi NakayamaLet's explain what's this magic is, e.g. why this is needed, and why this magic name is fine.
Also, I think it's robust to modify the original GeneratedCodeCache::GetContextKey() so that we can have a single key generator for the ContextKey. Otherwise, this kind of wrapper causes confusion on creating a consistent key for all users in the chromium.
For instance, if we follow the key name rules in the original method, you may use kSeparator (" \n") to append your specific new prefix, or suffix might be recommended?
Greg Thompson@g...@chromium.org Do you have any insights on the format of `ContextKey`? I'm not completely confident on this topic.
`GetCacheId` ensures a distinct backend/database for each type of data. Can we not use a single backend for bytecode from both normal JS and inline JS? `blink::UrlToCodeCacheKey` adds a fixed prefix to URLs to generate a key in the cache given a URL. A corresponding `blink::SourceHashToCodeCacheKey` that added a distinct fixed prefix to source hashes should be sufficient to ensure that it's not possible to manufacture a collision between these two types in the same database.
If we can use the same backend, we don't need to change `GetPendingBackend`.
I feel like splitting backend for JS resources and inline JS is better because the inline script cache can have a different requirement for cache size, or a different LRU-history, from caches for JS resources.
Related details
- Christian Dullweber
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Toyoshima
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. |
Christian Dullweber (Gerrit)
Christian Dullweber added 1 comment![]( "Open in Gerrit")
components/content_settings/renderer/content_settings_agent_impl_browsertest.cc lgtm
Related details
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Nakayama
- Takashi Toyoshima
Christian Dullweber (Gerrit)
Christian Dullweber voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Nakayama
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Greg Thompson (Gerrit)
Greg Thompson added 2 comments![]( "Open in Gerrit")
DCHECK_EQ(source_hash.size(), 32u);Greg ThompsonPrefer CHECK over DCHECK if you want to have this check here.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md
Takashi NakayamaI think it's fair to say that if `source_hash` isn't 32, the renderer must be misbehaving. In this case, I think `mojo::ReportBadMessage` is the right way to handle it rather than crashing the browser process.
I want to confirm if mojo IPC does `mojo::ReportBadMessage` if the length doesn't match the mojo definition of `array<uint8, 32> script_hash`. If yes, I will keep this as `CHECK`; otherwise, I will change this line to `mojo::ReportBadMessage()`.
Ah! https://chromium.googlesource.com/chromium/src/+/main/mojo/public/tools/bindings/README.md#primitive-types and https://chromium.googlesource.com/chromium/src/+/main/mojo/public/cpp/bindings/README.md#structtraits-reference tell me that mojo deserialization will take care of it. Is it possible to use `std::array<uint8_t, 32>` here instead of `std::vector<uint8_t>`? I see that there is an `ArrayTraits` specialization for it in mojo/public/cpp/bindings/array_traits.h.
Takashi NakayamaLet's explain what's this magic is, e.g. why this is needed, and why this magic name is fine.
Also, I think it's robust to modify the original GeneratedCodeCache::GetContextKey() so that we can have a single key generator for the ContextKey. Otherwise, this kind of wrapper causes confusion on creating a consistent key for all users in the chromium.
For instance, if we follow the key name rules in the original method, you may use kSeparator (" \n") to append your specific new prefix, or suffix might be recommended?
Greg Thompson@g...@chromium.org Do you have any insights on the format of `ContextKey`? I'm not completely confident on this topic.
Takashi Nakayama`GetCacheId` ensures a distinct backend/database for each type of data. Can we not use a single backend for bytecode from both normal JS and inline JS? `blink::UrlToCodeCacheKey` adds a fixed prefix to URLs to generate a key in the cache given a URL. A corresponding `blink::SourceHashToCodeCacheKey` that added a distinct fixed prefix to source hashes should be sufficient to ensure that it's not possible to manufacture a collision between these two types in the same database.
If we can use the same backend, we don't need to change `GetPendingBackend`.
I feel like splitting backend for JS resources and inline JS is better because the inline script cache can have a different requirement for cache size, or a different LRU-history, from caches for JS resources.
The reason I bring this up is that `PersistentCacheCollection` keeps at most 100 (`kDefaultLruCacheCapacity`) backends open. If each renderer starts using twice as many backends, `PCC` may start closing backends sooner, leading to more abandonment and more cache misses when renderers need to re-fetch the params for a backend.
Can you be more specific about how the caches would be treated differently?
Related details
Takashi Nakayama (Gerrit)
Takashi Nakayama added 6 comments![]( "Open in Gerrit")
DCHECK_EQ(source_hash.size(), 32u);Greg ThompsonPrefer CHECK over DCHECK if you want to have this check here.
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md
Takashi NakayamaI think it's fair to say that if `source_hash` isn't 32, the renderer must be misbehaving. In this case, I think `mojo::ReportBadMessage` is the right way to handle it rather than crashing the browser process.
Greg ThompsonI want to confirm if mojo IPC does `mojo::ReportBadMessage` if the length doesn't match the mojo definition of `array<uint8, 32> script_hash`. If yes, I will keep this as `CHECK`; otherwise, I will change this line to `mojo::ReportBadMessage()`.
Ah! https://chromium.googlesource.com/chromium/src/+/main/mojo/public/tools/bindings/README.md#primitive-types and https://chromium.googlesource.com/chromium/src/+/main/mojo/public/cpp/bindings/README.md#structtraits-reference tell me that mojo deserialization will take care of it. Is it possible to use `std::array<uint8_t, 32>` here instead of `std::vector<uint8_t>`? I see that there is an `ArrayTraits` specialization for it in mojo/public/cpp/bindings/array_traits.h.
Let's keep this as `CHECK` and use `std::vector` instead, because making this an `std::array` would require not only an additional type definition in the .mojom file, but also an additional type converter that would be nearly useless.
std::string resource_key = blink::ComposeSourceKeyedCacheKey(source_hash);Greg Thompsonnit: const std::string
As per my other comment: keys are opaque binary blobs now, so this could be `base::HeapArray<uint8_t>` to avoid needing to stringify the hash digest.
Marked as resolved because I deleted `blink::ComposeSourceKeyedCacheKey`
std::string resource_key = blink::ComposeSourceKeyedCacheKey(source_hash);Takashi Nakayamanit: const
ditto
Takashi NakayamaLet's explain what's this magic is, e.g. why this is needed, and why this magic name is fine.
Also, I think it's robust to modify the original GeneratedCodeCache::GetContextKey() so that we can have a single key generator for the ContextKey. Otherwise, this kind of wrapper causes confusion on creating a consistent key for all users in the chromium.
For instance, if we follow the key name rules in the original method, you may use kSeparator (" \n") to append your specific new prefix, or suffix might be recommended?
Greg Thompson@g...@chromium.org Do you have any insights on the format of `ContextKey`? I'm not completely confident on this topic.
Takashi Nakayama`GetCacheId` ensures a distinct backend/database for each type of data. Can we not use a single backend for bytecode from both normal JS and inline JS? `blink::UrlToCodeCacheKey` adds a fixed prefix to URLs to generate a key in the cache given a URL. A corresponding `blink::SourceHashToCodeCacheKey` that added a distinct fixed prefix to source hashes should be sufficient to ensure that it's not possible to manufacture a collision between these two types in the same database.
If we can use the same backend, we don't need to change `GetPendingBackend`.
Greg ThompsonI feel like splitting backend for JS resources and inline JS is better because the inline script cache can have a different requirement for cache size, or a different LRU-history, from caches for JS resources.
The reason I bring this up is that `PersistentCacheCollection` keeps at most 100 (`kDefaultLruCacheCapacity`) backends open. If each renderer starts using twice as many backends, `PCC` may start closing backends sooner, leading to more abandonment and more cache misses when renderers need to re-fetch the params for a backend.
Can you be more specific about how the caches would be treated differently?
I don't have a very strong opinion on whether we should split backends or not, but let me list up two perspectives:
1. Cache key: Resource script cache and inline script cache have different keying systems. The former uses `_key{$URL}` and the latter plans to use direct BLOB of SHA256 byte arrays. I worry that this can cause cache entry confusion if inline script cache shares its backend with resource script cache.
2. Caching lifetime: Inline script cache entries can have a different lifetime from resource script. Typically, one page has a larger number of shorter scripts for inline scripts than for resource scripts. So inline scripts can affect the LRU strategy of resource script cache if we unify the backends.
WDYT?
std::array<uint8_t, 32> source_hash = {
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32};can we use an actual sha256(data_str) here rather than a crafted magic numbers?
I couldn't find any functions in base library to calculate SHA256. Can I leave this as-is?
the `key` for PersistentCache is an opaque blob of bytes as of https://crrev.com/1590928, so there's no longer a need to stringify the hash.
- Christian Dullweber
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Toyoshima
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. |
Greg Thompson (Gerrit)
Greg Thompson added 2 comments![]( "Open in Gerrit")
Takashi NakayamaLet's explain what's this magic is, e.g. why this is needed, and why this magic name is fine.
Also, I think it's robust to modify the original GeneratedCodeCache::GetContextKey() so that we can have a single key generator for the ContextKey. Otherwise, this kind of wrapper causes confusion on creating a consistent key for all users in the chromium.
For instance, if we follow the key name rules in the original method, you may use kSeparator (" \n") to append your specific new prefix, or suffix might be recommended?
Greg Thompson@g...@chromium.org Do you have any insights on the format of `ContextKey`? I'm not completely confident on this topic.
Takashi Nakayama`GetCacheId` ensures a distinct backend/database for each type of data. Can we not use a single backend for bytecode from both normal JS and inline JS? `blink::UrlToCodeCacheKey` adds a fixed prefix to URLs to generate a key in the cache given a URL. A corresponding `blink::SourceHashToCodeCacheKey` that added a distinct fixed prefix to source hashes should be sufficient to ensure that it's not possible to manufacture a collision between these two types in the same database.
If we can use the same backend, we don't need to change `GetPendingBackend`.
Greg ThompsonI feel like splitting backend for JS resources and inline JS is better because the inline script cache can have a different requirement for cache size, or a different LRU-history, from caches for JS resources.
Takashi NakayamaThe reason I bring this up is that `PersistentCacheCollection` keeps at most 100 (`kDefaultLruCacheCapacity`) backends open. If each renderer starts using twice as many backends, `PCC` may start closing backends sooner, leading to more abandonment and more cache misses when renderers need to re-fetch the params for a backend.
Can you be more specific about how the caches would be treated differently?
I don't have a very strong opinion on whether we should split backends or not, but let me list up two perspectives:
1. Cache key: Resource script cache and inline script cache have different keying systems. The former uses `_key{$URL}` and the latter plans to use direct BLOB of SHA256 byte arrays. I worry that this can cause cache entry confusion if inline script cache shares its backend with resource script cache.
2. Caching lifetime: Inline script cache entries can have a different lifetime from resource script. Typically, one page has a larger number of shorter scripts for inline scripts than for resource scripts. So inline scripts can affect the LRU strategy of resource script cache if we unify the backends.
WDYT?
I don't have a very strong opinion on whether we should split backends or not, but let me list up two perspectives:
1. Cache key: Resource script cache and inline script cache have different keying systems. The former uses `_key{$URL}` and the latter plans to use direct BLOB of SHA256 byte arrays. I worry that this can cause cache entry confusion if inline script cache shares its backend with resource script cache.
If the inline script cache were to prepend a fixed prefix to the hash digest (e.g., "_digest{BLOB}"), then there is no risk of collision. If collisions are ruled out, what concern(s) remain?
2. Caching lifetime: Inline script cache entries can have a different lifetime from resource script. Typically, one page has a larger number of shorter scripts for inline scripts than for resource scripts. So inline scripts can affect the LRU strategy of resource script cache if we unify the backends.
PersistentCache-backed code cache doesn't track time-of-last-use of individual cache entries, and therefore can't purge individual entries based on that. When the size of the cache needs to be reduced, it's done based on the modification times of the database files themselves. We could purge based on time-of-insert, which we do track in the entry metadata. Since we don't have a separate index for resource vs. inline, I can see how it would be more complicated to have a distinct rule for removing old entries of each type in this case. It remains to be seen if it's necessary to go that route, however.
Just throwing this idea out there: we could use a distinct `PersistentCacheCollection` instance for the inline script cache. This would allow it to have its own target disk footprint and cache capacity (max number of active backends).
std::array<uint8_t, 32> source_hash = {
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32};Takashi Nakayamacan we use an actual sha256(data_str) here rather than a crafted magic numbers?
I couldn't find any functions in base library to calculate SHA256. Can I leave this as-is?
there's one in `//crypto/sha2.h`. `content_unittests` already has a dependency on `//crypto`, so you should be clear to use it in this context.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Nakayama
- Takashi Toyoshima
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. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
std::array<uint8_t, 32> source_hash = {
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32};Takashi Nakayamacan we use an actual sha256(data_str) here rather than a crafted magic numbers?
Greg ThompsonI couldn't find any functions in base library to calculate SHA256. Can I leave this as-is?
there's one in `//crypto/sha2.h`. `content_unittests` already has a dependency on `//crypto`, so you should be clear to use it in this context.
correction: `//crypto/hash.h`.
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
// source-keyed cache do not support WebUI.Greg ThompsonBut you may still need to check if the caller renderer is locked to a network origin?
Please check with Chrome Security on this. Maybe we only want to cache inline scripts for renderers that are locked to http(s) sites. Do we use inline scripts in WebUI?
Yeah, WebUI's script is under control in the chromium, and I believe this optimization is for the public web sites that may not take the best approach for performance.
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Nakayama
Takashi Nakayama (Gerrit)
Takashi Nakayama added 4 comments![]( "Open in Gerrit")
PersistentCache-backed code cache doesn't track time-of-last-use of individual cache entries
That is good to know! Then, I would like to adopt your idea of sharing the backend with resource script cache and adding `_digest` to cache keys, at least, on the initial stage. I made some code change to implement this. PTAL.
Splitting `PersistentCacheCollection` sounds good, but I would like to collect data of inline script cache first to minimize uncertainty. Thanks!
std::array<uint8_t, 32> source_hash = {
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32};Takashi Nakayamacan we use an actual sha256(data_str) here rather than a crafted magic numbers?
Greg ThompsonI couldn't find any functions in base library to calculate SHA256. Can I leave this as-is?
Greg Thompsonthere's one in `//crypto/sha2.h`. `content_unittests` already has a dependency on `//crypto`, so you should be clear to use it in this context.
correction: `//crypto/hash.h`.
Updated code to use `crypto::hash::Sha256()`. Thanks for pointing it out!
std::array<uint8_t, 32> source_hash = {
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32};Takashi Nakayamaditto
Done
1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16,
17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32};- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Toyoshima
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. |
Greg Thompson (Gerrit)
Greg Thompson added 16 comments![]( "Open in Gerrit")
* Introduces the `CodeCacheKeyType` enum to differentiate betweenupdate description
* Updates `GeneratedCodeCacheContext` to accept generic cache keyscan remove this item now
* Prepend an `_sk_` prefix to source-keyed cache IDs withinremove this item? i think the point of this is covered by the item below
#include "third_party/blink/public/mojom/loader/code_cache.mojom-shared.h"omit -- this header is exported by code_cache.mojom.h, so it doesn't need to be included explicitly like this.
// context uses a seperate database file. The second key is the resource
// url or the serialized script hash used on that cache.nit: maybe "prefixed resource URL or hash digest of a serialized inline script" so that readers don't think it's the raw URL/digest?
// isolation context from the collection. This insures that each isolationwhile you're editing this, would you change this to "ensures" :-)
(English.... amiright?)
// Returns the entry for `cache_key` in the cache identified by `context_key`,here, too
// Inserts `content` and `metadata` for `cache_key` in the cache identified byis there any reason not to keep this as "resource_key"?
base::span<const uint8_t> source_hash,(pedant warning!) "message digest" is the name of the thing that pops out of a hash function, so wdyt of "source_digest" here and elsewhere?
CHECK_IS_TEST();since this method has "ForTesting" in its name, do we need this line?
should we report a bad message if `cache_type` != `blink::mojom::CodeCacheType::kJavascript`?
CHECK_IS_TEST(); // Fetch is handled directly in the client in blink.since this method has "ForTesting" in its name, do we need this line? we have this check in `FetchCachedCode` because that's a non-test method that we want to use only in tests for the sake of convenience.
#include <cstdint>please remove -- `stdint.h` is the right header since Chromium uses `uint8_t` rather than `std::uint8_t`
std::vector<uint8_t> ComposeSourceKeyedCacheKey(can we use `base::HeapArray<uint8_t>` (or maybe `const uint8_t`?) instead of `vector`? we don't expect this to need to be resized and such by its holders after creation.
std::vector<uint8_t> key;
key.reserve(std::size(kSourceKeyedCodeCacheKeyPrefix) + source_hash.size());
key.insert_range(key.end(),
base::as_byte_span(kSourceKeyedCodeCacheKeyPrefix));
key.insert_range(key.end(), source_hash);this boils down to a malloc and two memcpys:
```
auto prefix = base::byte_span_from_cstring(kSourceKeyedCodeCacheKeyPrefix);
auto key =
base::HeapArray<uint8_t>::Uninit(prefix.size() + source_hash.size());
key.first(prefix.size()).copy_from_nonoverlapping(prefix);
key.last(source_hash.size()).copy_from_nonoverlapping(source_hash);
```
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Nakayama
- Takashi Toyoshima
Takashi Nakayama (Gerrit)
Takashi Nakayama added 15 comments![]( "Open in Gerrit")
* Introduces the `CodeCacheKeyType` enum to differentiate betweenTakashi Nakayamaupdate description
Done
* Updates `GeneratedCodeCacheContext` to accept generic cache keyscan remove this item now
Done
* Prepend an `_sk_` prefix to source-keyed cache IDs withinremove this item? i think the point of this is covered by the item below
Done
#include "third_party/blink/public/mojom/loader/code_cache.mojom-shared.h"omit -- this header is exported by code_cache.mojom.h, so it doesn't need to be included explicitly like this.
Done
// context uses a seperate database file. The second key is the resource
// url or the serialized script hash used on that cache.nit: maybe "prefixed resource URL or hash digest of a serialized inline script" so that readers don't think it's the raw URL/digest?
Done
// isolation context from the collection. This insures that each isolationwhile you're editing this, would you change this to "ensures" :-)
(English.... amiright?)
Done
// Returns the entry for `cache_key` in the cache identified by `context_key`,Takashi Nakayamahere, too
Acknowledged
// Inserts `content` and `metadata` for `cache_key` in the cache identified byis there any reason not to keep this as "resource_key"?
In blink, the word "resource" sometimes refers to a file with URL (e.g., `blink::Resource`). Renaming is to clarify that these functions can be also store inline script cache.
since this method has "ForTesting" in its name, do we need this line?
Let me keep this as-is because the "ForTesting" suffix is just a warning for developers and does not enforce any restrictions.
should we report a bad message if `cache_type` != `blink::mojom::CodeCacheType::kJavascript`?
I don't think we need that check because the current browser-side implementation (this CL) does not explicitly ban source-keyed cache for wasm. If we ban wasm, the check can be done on the renderer side.
CHECK_IS_TEST(); // Fetch is handled directly in the client in blink.since this method has "ForTesting" in its name, do we need this line? we have this check in `FetchCachedCode` because that's a non-test method that we want to use only in tests for the sake of convenience.
ditto.
Let me resolve this thread.
please remove -- `stdint.h` is the right header since Chromium uses `uint8_t` rather than `std::uint8_t`
Done
can we use `base::HeapArray<uint8_t>` (or maybe `const uint8_t`?) instead of `vector`? we don't expect this to need to be resized and such by its holders after creation.
Done
std::vector<uint8_t> key;
key.reserve(std::size(kSourceKeyedCodeCacheKeyPrefix) + source_hash.size());
key.insert_range(key.end(),
base::as_byte_span(kSourceKeyedCodeCacheKeyPrefix));
key.insert_range(key.end(), source_hash);this boils down to a malloc and two memcpys:
```
auto prefix = base::byte_span_from_cstring(kSourceKeyedCodeCacheKeyPrefix);
auto key =
base::HeapArray<uint8_t>::Uninit(prefix.size() + source_hash.size());
key.first(prefix.size()).copy_from_nonoverlapping(prefix);
key.last(source_hash.size()).copy_from_nonoverlapping(source_hash);
```
Thank you for the great suggestion! Reflected to the code.
Related details
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Toyoshima
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. |
Takashi Nakayama (Gerrit)
Takashi Nakayama added 2 comments![]( "Open in Gerrit")
// context uses a seperate database file. The second key is the prefixedWill handle:
Please fix this WARNING reported by Spellchecker: "seperate" is a possible misspelling of "separate".
To bypass Spellchecker, add...
"seperate" is a possible misspelling of "separate".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
(pedant warning!) "message digest" is the name of the thing that pops out of a hash function, so wdyt of "source_digest" here and elsewhere?
I understand your warning, but let me keep `source_hash` because it is more difficult to see the meaning of `digest` when it appears with the non-crypto (or non-hash related) contexts 😞
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
reset my attention flag until existing comments are resolved
Related details
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Rakina Zata Amni
- Takashi Nakayama
Greg Thompson (Gerrit)
Greg Thompson added 3 comments![]( "Open in Gerrit")
// Inserts `content` and `metadata` for `cache_key` in the cache identified byTakashi Nakayamais there any reason not to keep this as "resource_key"?
In blink, the word "resource" sometimes refers to a file with URL (e.g., `blink::Resource`). Renaming is to clarify that these functions can be also store inline script cache.
I'm a little concerned about `context` vs. `cache` in the names. `context_key` determines which cache will be accessed. To me, `cache_key` sounds awfully close to "the key that identifies the cache", and I think using `cache` in the name is redundant since this whole method is about inserting something into a cache.
I had picked "resource" as a generic word for "thing inserted into the cache". Maybe we can just call this `key` and forget about a prefix. But then it collides a bit with `context_key`. Maybe that should really be `cache_id`? Naming is hard.
I'll resolve this since I don't think it's really a big deal, but please chime in if you have other suggestions. Thanks.
CHECK_IS_TEST();Takashi Nakayamasince this method has "ForTesting" in its name, do we need this line?
Let me keep this as-is because the "ForTesting" suffix is just a warning for developers and does not enforce any restrictions.
Please remove as per https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#tests-and-test_only-code. The presubmit rule for use of `...ForTesting` is the Chromium way to deal with this. We should not have `CHECK_IS_TEST()` in such functions/method.
Takashi Nakayamashould we report a bad message if `cache_type` != `blink::mojom::CodeCacheType::kJavascript`?
I don't think we need that check because the current browser-side implementation (this CL) does not explicitly ban source-keyed cache for wasm. If we ban wasm, the check can be done on the renderer side.
We know that no renderer should try to insert into the WASM cache today. If one does, it's either due to a logic bug in the renderer or a compromised renderer. Reporting this as a bad message ensures that the renderer is promptly terminated. This is a security protection.
Related details
Takashi Nakayama (Gerrit)
Takashi Nakayama added 4 comments![]( "Open in Gerrit")
// context uses a seperate database file. The second key is the prefixedWill handle:
Please fix this WARNING reported by Spellchecker: "seperate" is a possible misspelling of "separate".
To bypass Spellchecker, add...
"seperate" is a possible misspelling of "separate".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
CHECK_IS_TEST();Takashi Nakayamasince this method has "ForTesting" in its name, do we need this line?
Greg ThompsonLet me keep this as-is because the "ForTesting" suffix is just a warning for developers and does not enforce any restrictions.
Please remove as per https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#tests-and-test_only-code. The presubmit rule for use of `...ForTesting` is the Chromium way to deal with this. We should not have `CHECK_IS_TEST()` in such functions/method.
I misunderstood the style guide. Thanks for pointing out. Fixed.
FWIW: The presubmit check for `ForTesting` seems to have some false-positives per https://crbug.com/40176497.
Takashi Nakayamashould we report a bad message if `cache_type` != `blink::mojom::CodeCacheType::kJavascript`?
Greg ThompsonI don't think we need that check because the current browser-side implementation (this CL) does not explicitly ban source-keyed cache for wasm. If we ban wasm, the check can be done on the renderer side.
We know that no renderer should try to insert into the WASM cache today. If one does, it's either due to a logic bug in the renderer or a compromised renderer. Reporting this as a bad message ensures that the renderer is promptly terminated. This is a security protection.
Acknowledged. I removed `cache_type` from the arguments of `DidGenerateSourceKeyedCacheableMetadata`.
// source-keyed cache do not support WebUI.Greg ThompsonBut you may still need to check if the caller renderer is locked to a network origin?
Takashi ToyoshimaPlease check with Chrome Security on this. Maybe we only want to cache inline scripts for renderers that are locked to http(s) sites. Do we use inline scripts in WebUI?
Yeah, WebUI's script is under control in the chromium, and I believe this optimization is for the public web sites that may not take the best approach for performance.
Added `CheckSecurityForAccessingSourceKeyedCodeCacheData()` and a call for it here to make sure WebUI and extentions are not cached. PTAL
Related details
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
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. |
Kouhei Ueno (Gerrit)
Kouhei Ueno voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kouhei Ueno
| Code-Review | +1 |
2 comments
Can we have a public version that contains the essential design part?
+1
}Would you note a FIXME comment that we shouldn't be using render_process_id to determine the NIK but CodeCacheHost should be made DocumentService and we should consult the RFH's origin here (and in other places)?
cc: @g...@chromium.org
Related details
- Greg Thompson
- Hiroshige Hayashizaki
- Takashi Nakayama
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. |