Implement CodeCacheHost for inline script cache on Blink [chromium/src : main]
Takashi Nakayama (Gerrit)
Takashi Nakayama voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 24 comments![]( "Open in Gerrit")
* Added a new method, `CodeCacheHost::FetchInlineScriptCacheSync()`, to comply with the HTML scripting specification. This function handles synchronous fetching of compiled code during the parsing phase. See crrev.com/c/7622052 for the usage.nit: wrap at 72 cols
// Fetch an inline script cache entry. `source_hash.size()` must equal tonit: "Fetches" as per https://google.github.io/styleguide/cppguide.html#Function_Comments
// to the `mojo::Remote` and the data it holds reliy on the object being validwhile you're here: "rely" :-)
DCHECK(inline_script_cache_fetch_state_);why this? the member is set in the ctor and never cleared. i don't find this check necessary. (`scoped_refptr<>::operator->()` will do its own `DCHECK`, too.)
if you do think it's necessary, please switch to `CHECK` as per https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c%2B%2B/c%2B%2B.md#check_dcheck_and-notreached
CHECK(features::IsInlineScriptCacheEnabled());is it necessary to have this here? it just means one more thing that needs to be touched if/when the feature is removed.
.WithArgs(cache_type, source_hash,if the timeout is reached before `FetchSourceKeyedCachedCode` finishes using `source_hash`, we'll have a UaF on this memory. i think you'll need something like `base::HeapArray::CopiedFrom(source_hash)` so that the callback owns a copy of the hash digest.
if (!source_keyed_cache_result) {
return {};
} else {
return std::move(source_keyed_cache_result.value());
}`return std::move(source_keyed_cache_result).value_or({});`
or, equivalently, do away with the local:
```
return inline_script_cache_fetch_state_
->WaitForFetchResultSync(features::kInlineScriptCacheTimeout.Get())
.value_or({});
```
maybe you'll need to spell out `mojo_base::BigBuffer()` in place of `{}`.
CHECK(features::IsInlineScriptCacheEnabled());remove this? (see comment above)
case mojom::blink::CodeCacheType::kWebAssembly:is there such a thing as inline wasm? if not, shall we simplify and make this js-only?
void HandleTransactionError(persistent_cache::TransactionError error) {`VALID_CONTEXT_REQUIRED(sequence_checker_)` up here in place of the DCHECK on line 441
// Start connection to the backend toincomplete comment
InitiateConnectionToBackend();Do you know what proportion of pages contain inline scripts? This will create a backend proactively, which will cause the browser to reach the PersistentCacheCollection's capacity sooner. This could lead to more cache misses for other renderers as their backends are abandoned.
base::ScopedClosureRunner cleanup(base::BindOnce(as far as i can tell, this `cleanup` will never run the callback. wdyt of removing this and instead putting `CHECK(!callback);` at the end of the function? That won't catch early returns, but it's better than nothing if you're concerned about someone missing running the callback in the future.
[](base::OnceCallback<void(mojo_base::BigBuffer)>* callback) {nit: pass by reference and use `std::ref(callback)` rather than `base::Unretained` below.
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);nit: move to the top of the function
if (!features::IsInlineScriptCacheEnabled()) {
std::move(callback).Run({});
return;
}Doesn't the `CHECK` on line 121 mean that this could never happen? I don't think we need to check this here.
void InitiateConnectionToBackend() {`VALID_CONTEXT_REQUIRED(...)` here, too
void HandleTransactionError(persistent_cache::TransactionError error) {`VALID_CONTEXT_REQUIRED(...)` here, too
// Advance the current nonce and prepare for the next cache lookup. Thisnit: "Advances... and prepares..." as per style guide
++current_nonce_;`CHECK_NE(++current_nonce_, 0ULL);`
++current_nonce_;consider: `result_ = mojo_base::BigBuffer();` to clear a value left behind by a previous fetch for which the requestor timed out.
}consider `result_ = mojo_base::BigBuffer();` as above
bool GUARDED_BY(lock_) is_destroyed_ = false;consider removing `is_destroyed_`:
- initialize `current_nonce_` to `1` below
- set `current_nonce_` to `0` in `OnMainThreadDestroyed`
- change `!is_destroyed_` checks to `current_nonce_ != 0`
bool GUARDED_BY(lock_) is_destroyed_ = false;very optional nit: i'm not sure that it matters, but i usually see this after the member name; see https://source.chromium.org/search?q=GUARDED_BY%20f:%5Ebase%20f:%5C.h
Related details
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- 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. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 2 comments![]( "Open in Gerrit")
DCHECK(inline_script_cache_fetch_state_);why this? the member is set in the ctor and never cleared. i don't find this check necessary. (`scoped_refptr<>::operator->()` will do its own `DCHECK`, too.)
if you do think it's necessary, please switch to `CHECK` as per https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c%2B%2B/c%2B%2B.md#check_dcheck_and-notreached
nit: making `inline_script_cache_fetch_state_` const (`const scoped_refptr<HostStateForInlineScriptCacheLookup>`) would clarify this.
CHECK(features::IsInlineScriptCacheEnabled());is it necessary to have this here? it just means one more thing that needs to be touched if/when the feature is removed.
I weakly prefer adding this kind of `CHECK()` to clarify which parts of code are behind the flag, but I defer to Greg and tnak@.
Related details
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
case mojom::blink::CodeCacheType::kWebAssembly:Hiroshige Hayashizakiis there such a thing as inline wasm? if not, shall we simplify and make this js-only?
+1
Takashi Nakayama (Gerrit)
Takashi Nakayama voted and added 23 comments![]( "Open in Gerrit")
Votes added by Takashi Nakayama
| Commit-Queue | +1 |
23 comments
* Added a new method, `CodeCacheHost::FetchInlineScriptCacheSync()`, to comply with the HTML scripting specification. This function handles synchronous fetching of compiled code during the parsing phase. See crrev.com/c/7622052 for the usage.nit: wrap at 72 cols
Done
// Fetch an inline script cache entry. `source_hash.size()` must equal tonit: "Fetches" as per https://google.github.io/styleguide/cppguide.html#Function_Comments
Done
// to the `mojo::Remote` and the data it holds reliy on the object being validwhile you're here: "rely" :-)
Done
Hiroshige Hayashizakiwhy this? the member is set in the ctor and never cleared. i don't find this check necessary. (`scoped_refptr<>::operator->()` will do its own `DCHECK`, too.)
if you do think it's necessary, please switch to `CHECK` as per https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c%2B%2B/c%2B%2B.md#check_dcheck_and-notreached
nit: making `inline_script_cache_fetch_state_` const (`const scoped_refptr<HostStateForInlineScriptCacheLookup>`) would clarify this.
Removed `DCHECK` and made `inline_script_cache_fetch_state_` const.
Hiroshige Hayashizakiis it necessary to have this here? it just means one more thing that needs to be touched if/when the feature is removed.
I weakly prefer adding this kind of `CHECK()` to clarify which parts of code are behind the flag, but I defer to Greg and tnak@.
Not necessary, but I would prefer these kind of `CHECK()` to prevent the feature is incorrectly used when the flag is disabled.
.WithArgs(cache_type, source_hash,if the timeout is reached before `FetchSourceKeyedCachedCode` finishes using `source_hash`, we'll have a UaF on this memory. i think you'll need something like `base::HeapArray::CopiedFrom(source_hash)` so that the callback owns a copy of the hash digest.
I hadn't noticed that. Thank you for pointing that out! Changed the second argument type of `AsyncCodeCacheHost::FetchSourceKeyedCachedCode` to `base::HeapArray<uint8_t>` and changed the caller code accordingly. PTAL.
if (!source_keyed_cache_result) {
return {};
} else {
return std::move(source_keyed_cache_result.value());
}`return std::move(source_keyed_cache_result).value_or({});`
or, equivalently, do away with the local:
```
return inline_script_cache_fetch_state_
->WaitForFetchResultSync(features::kInlineScriptCacheTimeout.Get())
.value_or({});
```
maybe you'll need to spell out `mojo_base::BigBuffer()` in place of `{}`.
Removed this code by changing the return type of `FetchInlineScriptCacheSync()` to `std::optional<...>`.
remove this? (see comment above)
ditto
case mojom::blink::CodeCacheType::kWebAssembly:Hiroshige Hayashizakiis there such a thing as inline wasm? if not, shall we simplify and make this js-only?
+1
Removed the support for WASM. PTAL
void HandleTransactionError(persistent_cache::TransactionError error) {`VALID_CONTEXT_REQUIRED(sequence_checker_)` up here in place of the DCHECK on line 441
Done
// Start connection to the backend toincomplete comment
Updated (and moved to `RemoteCache::RemoteCache()`). PTAL
as far as i can tell, this `cleanup` will never run the callback. wdyt of removing this and instead putting `CHECK(!callback);` at the end of the function? That won't catch early returns, but it's better than nothing if you're concerned about someone missing running the callback in the future.
Removed `cleanup` as you suggested. I added this to prevent unintentional stuck of the main thread before implementing the safety timeout on the main thread. It is safe to remove the code now. Thanks!
[](base::OnceCallback<void(mojo_base::BigBuffer)>* callback) {nit: pass by reference and use `std::ref(callback)` rather than `base::Unretained` below.
Acknowledged
nit: move to the top of the function
Done
if (!features::IsInlineScriptCacheEnabled()) {
std::move(callback).Run({});
return;
}Doesn't the `CHECK` on line 121 mean that this could never happen? I don't think we need to check this here.
ditto. (There is a slight difference. The `CHECK` in `FetchInlineScriptCacheSync()` runs in the main thread while this is `CHECK` running in the worker thread.)
void InitiateConnectionToBackend() {Takashi Nakayama`VALID_CONTEXT_REQUIRED(...)` here, too
Done
void HandleTransactionError(persistent_cache::TransactionError error) {Takashi Nakayama`VALID_CONTEXT_REQUIRED(...)` here, too
Done
// Advance the current nonce and prepare for the next cache lookup. Thisnit: "Advances... and prepares..." as per style guide
Done
++current_nonce_;Takashi Nakayama`CHECK_NE(++current_nonce_, 0ULL);`
As my comment in crrev.com/c/7621959/comment/4e757857_cadf9930/, I don't think this is necessary. Please feel free to reopen this thread if necessary.
consider: `result_ = mojo_base::BigBuffer();` to clear a value left behind by a previous fetch for which the requestor timed out.
Done
consider `result_ = mojo_base::BigBuffer();` as above
I understand your concern, but I want to keep this function as simple as possible because this is called in the destructor. WDYT?
very optional nit: i'm not sure that it matters, but i usually see this after the member name; see https://source.chromium.org/search?q=GUARDED_BY%20f:%5Ebase%20f:%5C.h
Done
bool GUARDED_BY(lock_) is_destroyed_ = false;consider removing `is_destroyed_`:
- initialize `current_nonce_` to `1` below
- set `current_nonce_` to `0` in `OnMainThreadDestroyed`
- change `!is_destroyed_` checks to `current_nonce_ != 0`
I don't think one variable having two meaning is a good way if there are no performance issues. WDYT?
Related details
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
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 8 comments![]( "Open in Gerrit")
CHECK_EQ(source_hash.size(), kSha256Bytes);`source_hash` is now a fixed-extent span, so this `CHECK` is no longer needed, no?
.WithArgs(cache_type, source_hash,Takashi Nakayamaif the timeout is reached before `FetchSourceKeyedCachedCode` finishes using `source_hash`, we'll have a UaF on this memory. i think you'll need something like `base::HeapArray::CopiedFrom(source_hash)` so that the callback owns a copy of the hash digest.
I hadn't noticed that. Thank you for pointing that out! Changed the second argument type of `AsyncCodeCacheHost::FetchSourceKeyedCachedCode` to `base::HeapArray<uint8_t>` and changed the caller code accordingly. PTAL.
great!
mojom::blink::CodeCacheType cache_type,can we remove this from the mojom interface since we only support JS?
case mojom::blink::CodeCacheType::kWebAssembly:Hiroshige Hayashizakiis there such a thing as inline wasm? if not, shall we simplify and make this js-only?
Takashi Nakayama+1
Removed the support for WASM. PTAL
ty!
// Start connection to the backend toTakashi Nakayamaincomplete comment
Updated (and moved to `RemoteCache::RemoteCache()`). PTAL
Acknowledged
void HandleTransactionError(persistent_cache::TransactionError error)nit: blank line before this
Takashi Nakayamaconsider `result_ = mojo_base::BigBuffer();` as above
I understand your concern, but I want to keep this function as simple as possible because this is called in the destructor. WDYT?
my observation is that it's called from `~CodeCacheWithPersistentCacheHostImpl`, but `HostStateForInlineScriptCacheLookup` itself is a refcounted object so it could live longer. if it happens that `OnMainThreadDestroyed` is called while `result_` holds a hunk of memory, we may as well release the memory now. i don't think it's a big deal either way...
bool GUARDED_BY(lock_) is_destroyed_ = false;consider removing `is_destroyed_`:
- initialize `current_nonce_` to `1` below
- set `current_nonce_` to `0` in `OnMainThreadDestroyed`
- change `!is_destroyed_` checks to `current_nonce_ != 0`
I don't think one variable having two meaning is a good way if there are no performance issues. WDYT?
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
Takashi Nakayama (Gerrit)
Takashi Nakayama added 4 comments![]( "Open in Gerrit")
`source_hash` is now a fixed-extent span, so this `CHECK` is no longer needed, no?
Done
can we remove this from the mojom interface since we only support JS?
Done
void HandleTransactionError(persistent_cache::TransactionError error)nit: blank line before this
Done
Takashi Nakayamaconsider `result_ = mojo_base::BigBuffer();` as above
Greg ThompsonI understand your concern, but I want to keep this function as simple as possible because this is called in the destructor. WDYT?
my observation is that it's called from `~CodeCacheWithPersistentCacheHostImpl`, but `HostStateForInlineScriptCacheLookup` itself is a refcounted object so it could live longer. if it happens that `OnMainThreadDestroyed` is called while `result_` holds a hunk of memory, we may as well release the memory now. i don't think it's a big deal either way...
Acknowledged. Thanks for sharing your thoughts. I added `result_ = {};`.
Related details
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
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 added 5 comments![]( "Open in Gerrit")
InitiateConnectionToBackend();Do you know what proportion of pages contain inline scripts? This will create a backend proactively, which will cause the browser to reach the PersistentCacheCollection's capacity sooner. This could lead to more cache misses for other renderers as their backends are abandoned.
I can imagine that to be pretty close to 100%? I believe almost every website would contain at least one inline script. Since the backend creation is gated by the feature flag, let's check the overhead via UMA/Finch.
// main thread.Would you add trace events to aid debugging?
if (!waiter_.TimedWait(timeout)) {Would you add histograms to measure how long we blocked the main thread?
if (!waiter_.TimedWait(timeout)) {Would you reference the param here if WaitForFetchResultSync is always called with the same param?
base::WaitableEvent waiter_;As discussed offline, we should be using ConditionVariable instead of WaitableEvent.
Related details
- Greg Thompson
- Hiroshige Hayashizaki
- Leszek Swirski
- Takashi Nakayama