[DOM Storage] Replace string source with StorageAreaSource struct [chromium/src : main]
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 1 comment![]( "Open in Gerrit")
Hi folks, please take a look when you have time. Thanks in advance for reviewing!
Related details
- Chromium IPC Reviews
- Evan Stade
- Steve Becker
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
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 2 comments![]( "Open in Gerrit")
StorageAreaSource source)probably ideal for all of these to be optional, i.e. `StorageAreaSource?`, since there are cases where we're passing an empty value.
String id = String::Number(base::RandUint64());so while we're at it can we make this a `base::Token`? (In some cases one might want to use an `UnguessableToken`, but here we explicitly do not need use this for security, instead relying on the mojo pipes between renderer and browser to correctly direct information, so it's better not to imply the unguessable nature of the token is a security measure.)
Related details
- Steve Becker
- Takashi Toyoshima
- Xiaohan Zhao
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. |
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
and thanks for doing this!
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 2 comments![]( "Open in Gerrit")
probably ideal for all of these to be optional, i.e. `StorageAreaSource?`, since there are cases where we're passing an empty value.
Done
so while we're at it can we make this a `base::Token`? (In some cases one might want to use an `UnguessableToken`, but here we explicitly do not need use this for security, instead relying on the mojo pipes between renderer and browser to correctly direct information, so it's better not to imply the unguessable nature of the token is a security measure.)
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
String id = String::Number(base::RandUint64());Xiaohan Zhaoso while we're at it can we make this a `base::Token`? (In some cases one might want to use an `UnguessableToken`, but here we explicitly do not need use this for security, instead relying on the mojo pipes between renderer and browser to correctly direct information, so it's better not to imply the unguessable nature of the token is a security measure.)
Done
Sorry, I meant to use `Token` throughout code, not just to generate a string. `Token` has native mojom support, `mojo_base.mojom.Token`.
Related details
- Steve Becker
- Takashi Toyoshima
- Xiaohan Zhao
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 3 comments![]( "Open in Gerrit")
// null, the key was newly added and had no previous value stored. `source`
// is null when the mutation was not initiated by a specific page (e.g.
// browser-initiated or internal reconciliation).As this seems relevant to other methods below, we may have this part in the interface comment?
// Identifies the source of a storage mutation for observer notifications.This can compile, but it may be nicer to declare before the first use above.
// store.Let's add an explanation on the nullability of the `source` parameter.
Related details
- Steve Becker
- Xiaohan Zhao
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 1 comment![]( "Open in Gerrit")
String id = String(base::Token::CreateRandom().ToString());We may clarify this change in the CL description?
Another question is shall we use base::UnguessableToken that can be mapped to mojom_base.UnguessableToken directly rather than using string in Mojo.
Xiaohan Zhao (Gerrit)
Xiaohan Zhao added 6 comments![]( "Open in Gerrit")
// null, the key was newly added and had no previous value stored. `source`
// is null when the mutation was not initiated by a specific page (e.g.
// browser-initiated or internal reconciliation).As this seems relevant to other methods below, we may have this part in the interface comment?
Done
// Identifies the source of a storage mutation for observer notifications.This can compile, but it may be nicer to declare before the first use above.
Done
Let's add an explanation on the nullability of the `source` parameter.
Done
String id = String::Number(base::RandUint64());Xiaohan Zhaoso while we're at it can we make this a `base::Token`? (In some cases one might want to use an `UnguessableToken`, but here we explicitly do not need use this for security, instead relying on the mojo pipes between renderer and browser to correctly direct information, so it's better not to imply the unguessable nature of the token is a security measure.)
Evan StadeDone
Sorry, I meant to use `Token` throughout code, not just to generate a string. `Token` has native mojom support, `mojo_base.mojom.Token`.
Sorry I misunderstood it. Updated. Thank you!
String id = String(base::Token::CreateRandom().ToString());We may clarify this change in the CL description?
Another question is shall we use base::UnguessableToken that can be mapped to mojom_base.UnguessableToken directly rather than using string in Mojo.
I've updated String with `base::Token` since this ID is not a security token and the mojo pipe already provides the security boundary, so the token doesn't need to be unguessable. But thanks for the suggestion!
Related details
- Evan Stade
- Steve Becker
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Steve Becker (Gerrit)
Steve Becker added 9 comments![]( "Open in Gerrit")
Thanks for this cleanup. I'm still reviewing the tests, but here are some initial comments.
// StorageAreaObserver events. This map is keyed by storage_area_id (a uniquenit: this does not map to any variable or code in this file.
const base::Token& storage_area_id);nit: I think the new name loses some meaning without source. Including source in the name makes it very clear that it is the source of the mutation.
Same applies to `pending_mutations_by_source_` member.
// An in-process implementation of LocalStorage using a LevelDB Mojo service.Thanks for cleaning up this comment. Can we also remove this reference to LevelDB?
// base::Token() is all-zero, matching the zero-initialized empty slot.
static constexpr bool kEmptyValueIsZero = true;
// Use the all-ones token as the deleted-value sentinel.
static void ConstructDeletedValue(base::Token& slot) {
slot = base::Token(std::numeric_limits<uint64_t>::max(),
std::numeric_limits<uint64_t>::max());
}
static bool IsDeletedValue(const base::Token& token) {
return token.high() == std::numeric_limits<uint64_t>::max() &&
token.low() == std::numeric_limits<uint64_t>::max();
}nit: Can we implement DeletedValue() instead of ConstructDeletedValue() and IsDeletedValue()?
See the comment above DeletedValue() here:
return HashInts(static_cast<unsigned>(token.high()),This cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
auto mojo_source =nit: mojo does not describe this variable since mojo is used everywhere to communicate between processes and threads. Maybe we should use something like `storage_area_source` or `source_url_and_id` instead?
Same applies below.
const base::Token& storage_area_id) {Can/should we check assert that storage_area_id is not 0?
// A zero token means no source (browser-initiated mutation), so there can't
// be a matching pending local mutation. Also, base::Token() is the
// empty-bucket sentinel in the HashMap, so we must not look it up.
if (storage_area_id.is_zero()) {
return nullptr;
}Are these lines necessary? Should searching for a zero token return the end iterator below instead?
Related details
- Evan Stade
- Takashi Toyoshima
- Xiaohan Zhao
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. |
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
return HashInts(static_cast<unsigned>(token.high()),This cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
Bit unfortunate we are not allowed to just use absl::flat_hash_map (for reasons that aren't that well explained in the documentation). I see a good deal of `base::flat_map` and `std::map` in Blink, even though they are supposed discouraged? I think it's worth seeking an exception here to avoid this complexity. Would also be good to understand what the tradeoffs actually are.
Failing that, I would still bail on this added complexity and use the stringified token for the key.
Related details
- Takashi Toyoshima
- Xiaohan Zhao
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Xiaohan Zhao
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
return HashInts(static_cast<unsigned>(token.high()),Evan StadeThis cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
Bit unfortunate we are not allowed to just use absl::flat_hash_map (for reasons that aren't that well explained in the documentation). I see a good deal of `base::flat_map` and `std::map` in Blink, even though they are supposed discouraged? I think it's worth seeking an exception here to avoid this complexity. Would also be good to understand what the tradeoffs actually are.
Failing that, I would still bail on this added complexity and use the stringified token for the key.
I asked this question here: https://chromium.slack.com/archives/CGJNSDHA7/p1772724774596959
Evan Stade (Gerrit)
Evan Stade added 1 comment![]( "Open in Gerrit")
return HashInts(static_cast<unsigned>(token.high()),Evan StadeThis cast will truncate the 64-bit token.high(), reducing it to 32-bit. To hash the entire token, we need to call HashInt() on the first 32-bits of the token. Afterwards, we need to call AddIntToHash() on the remaining three 32-bit parts of the token.
See:
[1] HashInt()
[2] AddIntToHash()
Evan StadeBit unfortunate we are not allowed to just use absl::flat_hash_map (for reasons that aren't that well explained in the documentation). I see a good deal of `base::flat_map` and `std::map` in Blink, even though they are supposed discouraged? I think it's worth seeking an exception here to avoid this complexity. Would also be good to understand what the tradeoffs actually are.
Failing that, I would still bail on this added complexity and use the stringified token for the key.
I asked this question here: https://chromium.slack.com/archives/CGJNSDHA7/p1772724774596959
Seems like folks thinks it's ok to use some non-Blink container such as `absl::flat_hash_map` since the map's value is not a GCed type, although we'll still need to update `audit_non_blink_usage.py` (and also use `ALLOW_DISCOURAGED_TYPE`?) which triggers a final approval.