Only decode streamed scripts in blink [chromium/src : main]
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois added 1 comment![]( "Open in Gerrit")
Related details
- Marja Hölttä
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä added 7 comments![]( "Open in Gerrit")
generally LG; it would make sense for somebody from the TOK team to review as they have the up-to-date understanding of the resource loading code. A bunch of things have changed here since I last touched the code. horo@, would you be a suitable reviewer?
I also ran the AI review on this and I posted its findings (I didn't investigate them very closely but they sounded plausible). I marked those comments with "AI review". (If they turn out to be valid, please also add tests for the corresponding corner cases.)
initial_data_ = base::HeapArray<uint8_t>();AI review:
This line is redundant as `initial_data_` was already moved out in the previous line.
DecodeAndReturnFlexibleBuffer(data.as_span(), src, encoding);AI review:
The `initial_data_` (raw bytes already received) should be appended to `raw_data_` here to ensure that `raw_data_` represents the complete script. While `BackgroundJSStreamManager` currently always starts with empty `initial_data_`, `ResourceScriptStreamer` can have prefix data, and leaving `raw_data_` incomplete is a bug in the `SourceStream` implementation.
std::make_unique<SecureStringDigest>(*digest_)));AI review:
If `total_length_` is 0 (which can happen if the script only contained a BOM or was empty), `FlattenChunks()` returns a null string and does not initialize `digest_`. This line will then crash when dereferencing the null `digest_` pointer.
Consider checking if `digest_` is non-null before creating a copy, or ensure `FlattenChunks` always computes a digest (even for empty strings).
*encoding = v8::ScriptCompiler::StreamedSource::FLEXIBLE_UTF16;Slightly surprised by this - this used to be the list of encodins V8 supports, but does Blink have a similar restriction somewhere?
TextEncoding(script_resource_->Encoding())));AI review:
This uses the original encoding of the resource. However, the BOM-checking logic at lines 960-967 might have detected a different encoding. You should use `decoder->Encoding()` here to ensure the streaming decoder uses the correct charset, similar to how it's handled in `BackgroundJSStreamManager` at line 2113.
script_decoder_(AI review:
When `kDecodeScriptsInBlink` is enabled, this constructor still creates a `ScriptDecoderWithClient`, which typically involves spawning a background decoding thread. This is inefficient since this decoder is discarded and replaced by the Blink-side decoder at line 1066.
Consider checking the feature flag here and skipping the creation of `script_decoder_` if the new path is being used.
Related details
- Anthony Vallee-Dubois
- Tsuyoshi Horo
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. |
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois added 7 comments![]( "Open in Gerrit")
Thanks! The AI reviewer comments were valid, I've added test cases to exercise them and fixed the behavior.
AI review:
This line is redundant as `initial_data_` was already moved out in the previous line.
Done
DecodeAndReturnFlexibleBuffer(data.as_span(), src, encoding);AI review:
The `initial_data_` (raw bytes already received) should be appended to `raw_data_` here to ensure that `raw_data_` represents the complete script. While `BackgroundJSStreamManager` currently always starts with empty `initial_data_`, `ResourceScriptStreamer` can have prefix data, and leaving `raw_data_` incomplete is a bug in the `SourceStream` implementation.
Done
AI review:
If `total_length_` is 0 (which can happen if the script only contained a BOM or was empty), `FlattenChunks()` returns a null string and does not initialize `digest_`. This line will then crash when dereferencing the null `digest_` pointer.
Consider checking if `digest_` is non-null before creating a copy, or ensure `FlattenChunks` always computes a digest (even for empty strings).
Good catch, added a unit test and fixed the issue.
*encoding = v8::ScriptCompiler::StreamedSource::FLEXIBLE_UTF16;Slightly surprised by this - this used to be the list of encodins V8 supports, but does Blink have a similar restriction somewhere?
They are the only encodings that blink supports *for streaming* (I'm guessing that's because they were the only ones supported in v8).
Now that v8 expects decoded UTF16, and that blink can decode more stuff, I imagine it'd be pretty easy to support more encodings in streaming. Not sure how useful that would be: do we have metrics related to script encodings used in the wild?
AI review:
This uses the original encoding of the resource. However, the BOM-checking logic at lines 960-967 might have detected a different encoding. You should use `decoder->Encoding()` here to ensure the streaming decoder uses the correct charset, similar to how it's handled in `BackgroundJSStreamManager` at line 2113.
Done
AI review:
When `kDecodeScriptsInBlink` is enabled, this constructor still creates a `ScriptDecoderWithClient`, which typically involves spawning a background decoding thread. This is inefficient since this decoder is discarded and replaced by the Blink-side decoder at line 1066.
Consider checking the feature flag here and skipping the creation of `script_decoder_` if the new path is being used.
- Marja Hölttä
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 18 comments![]( "Open in Gerrit")
Overall looks good.
It might feel a bit redundant, but I suggest adding as many feature `CHECK()`s as possible to make the eventual post-launch cleanup much cleaner and easier.
DCHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));Please use `CHECK()` in new code. Could you also apply this to other new codes introduced in this CL?
v8::ScriptCompiler::FlexibleExternalSourceStream::Chunk chunk =nit: `auto`
v8::ScriptCompiler::FlexibleExternalSourceStream::Chunk chunk =nit: `auto`
continue;Similar to `GetMoreData()`, should we also check if the stream has been cancelled after waking up from `mojo::Wait()` in `GetNextChunk()`?
Checking it here would prevent us from running `BeginReadData()` and performing unnecessary decoding/processing after `SourceStream::Cancel()` has been called.
```cpp
// We were blocked, so check for cancelation again.
if (cancelled_.IsSet()) {
SetFinished(ResourceScriptStreamer::LoadingState::kCancelled);
return EmptyChunk();
}
[](ResponseBodyLoaderClient* client, const String& decoded_data,
std::unique_ptr<SecureStringDigest> digest) {
if (client) {
client->DidReceiveDecodedData(decoded_data,
std::move(digest));
}
},I think this should be `&ResponseBodyLoaderClient::DidReceiveDecodedData`.
} else {
PostCrossThreadTask(
*loading_task_runner_, FROM_HERE,
CrossThreadBindOnce(&ResourceScriptStreamer::StreamingComplete,
WrapCrossThreadPersistent(this), state));
}Could you please explain why we need this?
```
} else {
PostCrossThreadTask(
*loading_task_runner_, FROM_HERE,
CrossThreadBindOnce(&ResourceScriptStreamer::StreamingComplete,
WrapCrossThreadPersistent(this), state));
}
```
source_ = std::make_unique<v8::ScriptCompiler::StreamedSource>(`CHECK(base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));` ?
source_ = std::make_unique<v8::ScriptCompiler::StreamedSource>(`CHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));` ?
if (encoding_ == v8::ScriptCompiler::StreamedSource::FLEXIBLE_UTF16) {
source_ = std::make_unique<v8::ScriptCompiler::StreamedSource>(
std::move(stream_ptr));
} else {
source_ = std::make_unique<v8::ScriptCompiler::StreamedSource>(
std::move(stream_ptr), encoding_);
}
Could you add a brief comment explaining why this branching is necessary?
Just to make sure we are on the same page, is my understanding of the design correct?
1. The `StreamedSource` constructor that accepts an explicit `Encoding` parameter does not support `FLEXIBLE_UTF16` (due to the `DCHECK(encoding != FLEXIBLE_UTF16)` on the V8 side).
2. The constructor taking `std::unique_ptr<FlexibleExternalSourceStream>` is only intended for when `features::kDecodeScriptsInBlink` is enabled.
3. If the `kDecodeScriptsInBlink` experiment is successful, we plan to eventually deprecate the old path and only use this new constructor.
script_decoder_->DidReceiveData(Vector<char>(chars),
/*send_to_client=*/false);
}Could we add a `CHECK` inside this block, and also add a comment explaining the data flow when `kDecodeScriptsInBlink` is enabled?
For example:
```
if (script_decoder_) {
// When `kDecodeScriptsInBlink` is enabled, the initial bytes read here are
// forwarded to `ScriptResource` via `DidReceiveData()` above. They are then
// retrieved from the `ScriptResource` as `initial_data_` in
// `SourceStream::TakeDataAndPipeOnMainThread()`, and eventually decoded
// on the background thread inside `SourceStream::GetNextChunk()`.
CHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));
script_decoder_->DidReceiveData(Vector<char>(chars),
/*send_to_client=*/false);
}
```
script_decoder_->FinishDecode(CrossThreadBindOnce(`CHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));`
SendClientLoadFinishedCallback();`CHECK(base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));`
streamed_source = std::make_unique<v8::ScriptCompiler::StreamedSource>(`CHECK(base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));` ?
streamed_source = std::make_unique<v8::ScriptCompiler::StreamedSource>(`CHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));` ?
INSTANTIATE_TEST_SUITE_P(All, ScriptStreamingTest, testing::Bool());Could please move this just below the class definition of ScriptStreamingTest?
Also
```
INSTANTIATE_TEST_SUITE_P(All,
ScriptStreamingTest,
testing::Bool(),
&ScriptStreamingTest::DescribeParams);
```
And add DescribeParams method in the ScriptStreamingTest class:
```
static std::string DescribeParams(
const testing::TestParamInfo<ParamType>& info) {
return info.param ? "DecodeScriptsInBlinkEnabled" : "DecodeScriptsInBlinkDisabled";
}
```
INSTANTIATE_TEST_SUITE_P(All,
BackgroundResourceScriptStreamerTest,
testing::Bool());Could please move this just below the class definition of BackgroundResourceScriptStreamerTest, and add DescribeParams method?
INSTANTIATE_TEST_SUITE_P(
All,
BackgroundResourceScriptStreamerCodeCacheDecodeStartTest,
testing::Bool());
Could please move this just below the class definition of BackgroundResourceScriptStreamerTest, and add DescribeParams method?
Related details
- Anthony Vallee-Dubois
- Marja Hölttä
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. |
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois added 18 comments![]( "Open in Gerrit")
Thanks for the review! Comments addressed, PTAnL.
DCHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));Please use `CHECK()` in new code. Could you also apply this to other new codes introduced in this CL?
Done
v8::ScriptCompiler::FlexibleExternalSourceStream::Chunk chunk =Anthony Vallee-Duboisnit: `auto`
Done
v8::ScriptCompiler::FlexibleExternalSourceStream::Chunk chunk =Anthony Vallee-Duboisnit: `auto`
Done
Similar to `GetMoreData()`, should we also check if the stream has been cancelled after waking up from `mojo::Wait()` in `GetNextChunk()`?
Checking it here would prevent us from running `BeginReadData()` and performing unnecessary decoding/processing after `SourceStream::Cancel()` has been called.
```cpp
// We were blocked, so check for cancelation again.
if (cancelled_.IsSet()) {
SetFinished(ResourceScriptStreamer::LoadingState::kCancelled);
return EmptyChunk();
}
Good call, done!
[](ResponseBodyLoaderClient* client, const String& decoded_data,
std::unique_ptr<SecureStringDigest> digest) {
if (client) {
client->DidReceiveDecodedData(decoded_data,
std::move(digest));
}
},I think this should be `&ResponseBodyLoaderClient::DidReceiveDecodedData`.
Done, although now that this prevents checking for nullptr `client`, I've replaced client CrossThreadWeakHandles with CrossThreadHandles.
} else {
PostCrossThreadTask(
*loading_task_runner_, FROM_HERE,
CrossThreadBindOnce(&ResourceScriptStreamer::StreamingComplete,
WrapCrossThreadPersistent(this), state));
}Could you please explain why we need this?
```
} else {
PostCrossThreadTask(
*loading_task_runner_, FROM_HERE,
CrossThreadBindOnce(&ResourceScriptStreamer::StreamingComplete,
WrapCrossThreadPersistent(this), state));
}
```
Not sure how that ended up there, I think I was confused when adding the new if block and wanted all of the branches to be covered.
It's not necessary as per the old behavior script_decoder_ is always non-null here when kDecodeScriptsInBlink is disabled. Removed, and replaced the `else if` with a CHECK
source_ = std::make_unique<v8::ScriptCompiler::StreamedSource>(`CHECK(base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));` ?
Done
source_ = std::make_unique<v8::ScriptCompiler::StreamedSource>(`CHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));` ?
Done
if (encoding_ == v8::ScriptCompiler::StreamedSource::FLEXIBLE_UTF16) {
source_ = std::make_unique<v8::ScriptCompiler::StreamedSource>(
std::move(stream_ptr));
} else {
source_ = std::make_unique<v8::ScriptCompiler::StreamedSource>(
std::move(stream_ptr), encoding_);
}
Could you add a brief comment explaining why this branching is necessary?
Just to make sure we are on the same page, is my understanding of the design correct?
1. The `StreamedSource` constructor that accepts an explicit `Encoding` parameter does not support `FLEXIBLE_UTF16` (due to the `DCHECK(encoding != FLEXIBLE_UTF16)` on the V8 side).
2. The constructor taking `std::unique_ptr<FlexibleExternalSourceStream>` is only intended for when `features::kDecodeScriptsInBlink` is enabled.
3. If the `kDecodeScriptsInBlink` experiment is successful, we plan to eventually deprecate the old path and only use this new constructor.
That's exactly correct. Added a comment to that effect.
script_decoder_->DidReceiveData(Vector<char>(chars),
/*send_to_client=*/false);
}Could we add a `CHECK` inside this block, and also add a comment explaining the data flow when `kDecodeScriptsInBlink` is enabled?
For example:
```
if (script_decoder_) {
// When `kDecodeScriptsInBlink` is enabled, the initial bytes read here are
// forwarded to `ScriptResource` via `DidReceiveData()` above. They are then
// retrieved from the `ScriptResource` as `initial_data_` in
// `SourceStream::TakeDataAndPipeOnMainThread()`, and eventually decoded
// on the background thread inside `SourceStream::GetNextChunk()`.
CHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));
script_decoder_->DidReceiveData(Vector<char>(chars),
/*send_to_client=*/false);
}
```
Good idea, and I like the way you phrased that comment so using it directly 😊
script_decoder_->FinishDecode(CrossThreadBindOnce(Anthony Vallee-Dubois`CHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));`
Done
SendClientLoadFinishedCallback();Anthony Vallee-Dubois`CHECK(base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));`
Done
streamed_source = std::make_unique<v8::ScriptCompiler::StreamedSource>(`CHECK(base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));` ?
Done
streamed_source = std::make_unique<v8::ScriptCompiler::StreamedSource>(`CHECK(!base::FeatureList::IsEnabled(features::kDecodeScriptsInBlink));` ?
Done
INSTANTIATE_TEST_SUITE_P(All, ScriptStreamingTest, testing::Bool());Could please move this just below the class definition of ScriptStreamingTest?
Also
```
INSTANTIATE_TEST_SUITE_P(All,
ScriptStreamingTest,
testing::Bool(),
&ScriptStreamingTest::DescribeParams);
```And add DescribeParams method in the ScriptStreamingTest class:
```
static std::string DescribeParams(
const testing::TestParamInfo<ParamType>& info) {
return info.param ? "DecodeScriptsInBlinkEnabled" : "DecodeScriptsInBlinkDisabled";
}
```
Done
INSTANTIATE_TEST_SUITE_P(All,
BackgroundResourceScriptStreamerTest,
testing::Bool());Could please move this just below the class definition of BackgroundResourceScriptStreamerTest, and add DescribeParams method?
Done
INSTANTIATE_TEST_SUITE_P(
All,
BackgroundResourceScriptStreamerCodeCacheDecodeStartTest,
testing::Bool());
Could please move this just below the class definition of BackgroundResourceScriptStreamerTest, and add DescribeParams method?
- Marja Hölttä
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo added 1 comment![]( "Open in Gerrit")
BackgroundResourceScriptStreamerTests are failing.
Could you please take a look?
Related details
- Anthony Vallee-Dubois
- Marja Hölttä
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois added 1 comment![]( "Open in Gerrit")
BackgroundResourceScriptStreamerTests are failing.
Could you please take a look?
Yep! Sorry I started the dry run and left for the day. Looks like my local build didn't have DCHECKs enabled, and the issue was that we were hitting one from dereferencing a WeakPtr on the wrong sequence. Fixed!
Related details
- Marja Hölttä
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois added 1 comment![]( "Open in Gerrit")
Seems like ScriptStreamingTest.CancellingStreaming tests are flaky with and without the new feature. I will debug this and let you know when I've found the cause.
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois added 1 comment![]( "Open in Gerrit")
Fixed. CancellingStreaming wasn't properly resetting its producer_handle_ which caused a time out in the parametrized version of the test where full tear down doesn't necessarily happen between each test case.
Related details
- Marja Hölttä
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tsuyoshi Horo
| Code-Review | +1 |
1 comment
- Anthony Vallee-Dubois
- Marja Hölttä
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Marja Hölttä (Gerrit)
Marja Hölttä voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Anthony Vallee-Dubois
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anthony Vallee-Dubois (Gerrit)
Anthony Vallee-Dubois added 1 comment![]( "Open in Gerrit")
+kouhei@ as script_decoder_test and blink/common/features OWNERs. PTAL!
Related details
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job android-pixel10-perf/loadline2_phone.crossbench failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1784ace2290000
Related details
- Anthony Vallee-Dubois
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job android-pixel10-perf/loadline2_phone.crossbench failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/145a4464290000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job android-pixel10-perf/loadline2_phone.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/13a5ab94290000
Related details
- Anthony Vallee-Dubois
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job android-pixel10-perf/loadline2_phone.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1432674a290000