[source-phase-imports] Don't use ParkableStrings for Wasm sources [chromium/src : main]
Luis Pardo (Gerrit)
Luis Pardo added 3 comments![]( "Open in Gerrit")
@dan...@microsoft.com could you take a look? I left a couple of comments in-line.
class CORE_EXPORT ModuleScriptCreationParams {This is added just so that the linker doesn't complain about the `CopySource` method, we could probably inline it if we don't want to add this here.
*params));I **think** this should have always been `params_`. Seem like the correct think to pass the isolated copy to the other thread and not the original.
Related details
- Dan Clark
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 1 comment![]( "Open in Gerrit")
- Hiroshige Hayashizaki
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 8 comments![]( "Open in Gerrit")
// For Wasm modules the wire bytes are passed directly to the compiler.Could you make this comment a little more strict? e.g.
// For `ResolvedModuleType::kWasm`, the wire bytes are stored as `base::HeapArray<uint8_t>` and directly passed to the compiler.
// Otherwise, decoded text is stored as `ParkableString`.
const base::HeapArray<uint8_t>& WasmSource() const {nit: name like `GetSourceForWasm()` would be clearer (it's the wasm version of `GetSourceText()`).
ModuleScriptCreationParams(const ModuleScriptCreationParams& other)I'd like to avoid manual copy constructor because it's quite error-prone in general.
I found (pending trybot runs) ModuleScriptCreationParams can be move-only:
https://chromium-review.googlesource.com/c/chromium/src/+/6625840.
Could you check if this removes the copy constructor here?
ModuleScriptCreationParams::CopySource(Can we make this just `ModuleScriptCreationParams::CopySource()` and use `this` instead of `other`? (i.e. drop `other->`)?
I feel referencing both `this` and `other` seems confusing and not needed here.
ResolvedModuleType module_type,Could you also add a comment to describe the type invariant, i.e.
`base::HeapArray<uint8_t>` is stored when `module_type` is `ResolvedModuleType::kWasm`, or `ParkableString` otherwise.
Maybe it would be ideal to create a new class (in module_script_creation_params.h?) containing `ResolvedModuleType` and `source` to enforce the invariant and centralize the type contract, and use the class here and `ModuleScriptCreationParams`, but probably not necessarily in this CL.
// ScriptResource is a resource representing a JavaScript classicnit: isn't this virtually the same as before?
// This method should be called only once.I doubt this assmption holds:
what if multiple wasm modules points to the same `ScriptResource`?
e.g. when we import "a.wasm" and "a.wasm#foo" in the same page. MemoryCache ignores `#foo` part and thus I expect the same ScriptResource is used.
Could you check this, and add a corresponding wpt (or wpt_internal) test (if you find the current patch set crashes here)?
---
I suspect this might be crashing or incorrectly behaving also when:
- multiple same-origin pages import the same WASM modules
- the `ScriptResource` is used WASM module and classic script (which anyway cause script parse error or something, but shouldn't crash or cause other internal inconsistencies)
but these might be a little harder to test/reproduce, so just checking the condition above is sufficient.
*params));I **think** this should have always been `params_`. Seem like the correct think to pass the isolated copy to the other thread and not the original.
`CrossThreadBindOnce()` internally calls `IsolatedCopy`, so passing the original here is correct.
(I feel passing `*params_` is also equivalent though)
Or, did you encounter something that led this modification?
Related details
- Luis Pardo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 9 comments![]( "Open in Gerrit")
// For Wasm modules the wire bytes are passed directly to the compiler.Could you make this comment a little more strict? e.g.
// For `ResolvedModuleType::kWasm`, the wire bytes are stored as `base::HeapArray<uint8_t>` and directly passed to the compiler.
// Otherwise, decoded text is stored as `ParkableString`.
Done
const base::HeapArray<uint8_t>& WasmSource() const {nit: name like `GetSourceForWasm()` would be clearer (it's the wasm version of `GetSourceText()`).
Done
ModuleScriptCreationParams(const ModuleScriptCreationParams& other)I'd like to avoid manual copy constructor because it's quite error-prone in general.
I found (pending trybot runs) ModuleScriptCreationParams can be move-only:
https://chromium-review.googlesource.com/c/chromium/src/+/6625840.Could you check if this removes the copy constructor here?
Yes, it works, I'll rebase after you land that patch.
This is added just so that the linker doesn't complain about the `CopySource` method, we could probably inline it if we don't want to add this here.
Acknowledged
Can we make this just `ModuleScriptCreationParams::CopySource()` and use `this` instead of `other`? (i.e. drop `other->`)?
I feel referencing both `this` and `other` seems confusing and not needed here.
Done
Could you also add a comment to describe the type invariant, i.e.
`base::HeapArray<uint8_t>` is stored when `module_type` is `ResolvedModuleType::kWasm`, or `ParkableString` otherwise.
Maybe it would be ideal to create a new class (in module_script_creation_params.h?) containing `ResolvedModuleType` and `source` to enforce the invariant and centralize the type contract, and use the class here and `ModuleScriptCreationParams`, but probably not necessarily in this CL.
Added the comment and a TODO.
// ScriptResource is a resource representing a JavaScript classicnit: isn't this virtually the same as before?
Expanded it more to clarify that this is also used for JSON, CSS, and Wasm module scripts.
// This method should be called only once.I doubt this assmption holds:
what if multiple wasm modules points to the same `ScriptResource`?
e.g. when we import "a.wasm" and "a.wasm#foo" in the same page. MemoryCache ignores `#foo` part and thus I expect the same ScriptResource is used.Could you check this, and add a corresponding wpt (or wpt_internal) test (if you find the current patch set crashes here)?
---
I suspect this might be crashing or incorrectly behaving also when:
- multiple same-origin pages import the same WASM modules
- the `ScriptResource` is used WASM module and classic script (which anyway cause script parse error or something, but shouldn't crash or cause other internal inconsistencies)
but these might be a little harder to test/reproduce, so just checking the condition above is sufficient.
You are right, we do need to keep the data around. For now, I added a new `source_bytes_` field in this class to keep it in memory, but I'm not sure if this is the right long-term approach.
If the resource is going to stay in memory for a long time it would be better to have the source in a "parkable" object, but using a `ParkableString` for a non-string buffer seems hacky. WDYT?
*params));Hiroshige HayashizakiI **think** this should have always been `params_`. Seem like the correct think to pass the isolated copy to the other thread and not the original.
`CrossThreadBindOnce()` internally calls `IsolatedCopy`, so passing the original here is correct.
(I feel passing `*params_` is also equivalent though)Or, did you encounter something that led this modification?
Or, did you encounter something that led this modification?
No, I don't know why I thought this was calling the copy constructor instead of IsolatedCopy.
Related details
- Hiroshige Hayashizaki
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 1 comment![]( "Open in Gerrit")
ModuleScriptCreationParams(const ModuleScriptCreationParams& other)Luis PardoI'd like to avoid manual copy constructor because it's quite error-prone in general.
I found (pending trybot runs) ModuleScriptCreationParams can be move-only:
https://chromium-review.googlesource.com/c/chromium/src/+/6625840.Could you check if this removes the copy constructor here?
Yes, it works, I'll rebase after you land that patch.
Done
Related details
- Hiroshige Hayashizaki
Code-Coverage
Code-Review
No-Unresolved-Comments
| 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")
corrupting the Wasm source.Do you have tests? (i.e. to confirm the wasm source is not decoded as UTF-8)
// This method should be called only once.Luis PardoI doubt this assmption holds:
what if multiple wasm modules points to the same `ScriptResource`?
e.g. when we import "a.wasm" and "a.wasm#foo" in the same page. MemoryCache ignores `#foo` part and thus I expect the same ScriptResource is used.Could you check this, and add a corresponding wpt (or wpt_internal) test (if you find the current patch set crashes here)?
---
I suspect this might be crashing or incorrectly behaving also when:
- multiple same-origin pages import the same WASM modules
- the `ScriptResource` is used WASM module and classic script (which anyway cause script parse error or something, but shouldn't crash or cause other internal inconsistencies)
but these might be a little harder to test/reproduce, so just checking the condition above is sufficient.
You are right, we do need to keep the data around. For now, I added a new `source_bytes_` field in this class to keep it in memory, but I'm not sure if this is the right long-term approach.
If the resource is going to stay in memory for a long time it would be better to have the source in a "parkable" object, but using a `ParkableString` for a non-string buffer seems hacky. WDYT?
At least we shouldn't call `ClearData();` because it would invalidate the data that would be otherwise
Related details
- Luis Pardo
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Support for Wasm modules was already added, but the sources areI've figured out an underlying issue for this: when a `ScriptResource` is used both as a text JavaScript and WASM, users can observe unexpected empty bytes (or probably other inconsistent states).
Background (root cause):
`ScriptResource` (when used for ordinal text classic/module scripts) usually clears its underlying bytes as soon as possible.
Problem:
Reproducing case (to be placed under `virtual/javascript-source-phase-imports/external/wpt/wasm/webapi/esm-integration/`):
```
<!doctype html>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
setup({ single_test: true, allow_uncaught_exception : true });
</script>
<script crossorigin src="./resources/exported-names.wasm"></script>
<script type=module>
import source exportedNamesSource from "./resources/exported-names.wasm";
done();
</script>
```
1. A `ScriptResource` is used as a classic script. This grabs the loaded body (which is WASM) and causes `CONSOLE ERROR: Uncaught SyntaxError: Invalid or unexpected token` as expected (not JavaScript at all, of course). `crossorigin` attribute is used to make the `ScriptResource` shared between classic script here and module WASM below.
2. The same `ScriptResource` is used as WASM (via Blink MemoryCache). The body (`ScriptResource::Data()`) is already gone at this time, and causes `CONSOLE ERROR: Uncaught CompileError: WasmModuleObject::Compile(): expected 4 bytes, fell off end @+0`,
i.e. unexpected empty body is observed.
Anyway almost always at most one of them can be successful and there are no valid use cases for this scenario, but something wrong shouldn't be observed / shouldn't be used as attack vector.
Solutions...?
So ... what we should do? I'm not confident at all, brainstorming...
Given that we switch JavaScript/WASM compilation sorely based on Content-Type response header, anyway we need to use `ScriptResource` that can potentially shared with text module scripts.
So ... for example, we might want to keep raw bytes when Content-Type is `application/wasm`, but this anyway needs more careful design.
source_bytes_ = std::move(data_array);Do we need `source_bytes_` here?
Can we always get bytes directly from `Data()` without `ClearData()`?
Luis Pardo (Gerrit)
Luis Pardo voted and added 4 comments![]( "Open in Gerrit")
Votes added by Luis Pardo
| Commit-Queue | +1 |
4 comments
Support for Wasm modules was already added, but the sources areI've figured out an underlying issue for this: when a `ScriptResource` is used both as a text JavaScript and WASM, users can observe unexpected empty bytes (or probably other inconsistent states).
Background (root cause):
`ScriptResource` (when used for ordinal text classic/module scripts) usually clears its underlying bytes as soon as possible.
Problem:
Reproducing case (to be placed under `virtual/javascript-source-phase-imports/external/wpt/wasm/webapi/esm-integration/`):
```
<!doctype html>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script>
setup({ single_test: true, allow_uncaught_exception : true });
</script>
<script crossorigin src="./resources/exported-names.wasm"></script>
<script type=module>
import source exportedNamesSource from "./resources/exported-names.wasm";
done();
</script>
```1. A `ScriptResource` is used as a classic script. This grabs the loaded body (which is WASM) and causes `CONSOLE ERROR: Uncaught SyntaxError: Invalid or unexpected token` as expected (not JavaScript at all, of course). `crossorigin` attribute is used to make the `ScriptResource` shared between classic script here and module WASM below.
2. The same `ScriptResource` is used as WASM (via Blink MemoryCache). The body (`ScriptResource::Data()`) is already gone at this time, and causes `CONSOLE ERROR: Uncaught CompileError: WasmModuleObject::Compile(): expected 4 bytes, fell off end @+0`,
i.e. unexpected empty body is observed.Anyway almost always at most one of them can be successful and there are no valid use cases for this scenario, but something wrong shouldn't be observed / shouldn't be used as attack vector.
Solutions...?
So ... what we should do? I'm not confident at all, brainstorming...
Given that we switch JavaScript/WASM compilation sorely based on Content-Type response header, anyway we need to use `ScriptResource` that can potentially shared with text module scripts.
So ... for example, we might want to keep raw bytes when Content-Type is `application/wasm`, but this anyway needs more careful design.
I think think it should be enough to do this check before clearing the raw bytes. I'll follow up in another CL, opened another issue (blocking the feature) to track this: https://issues.chromium.org/issues/425682456.
Do you have tests? (i.e. to confirm the wasm source is not decoded as UTF-8)
Added the test.
// This method should be called only once.Luis PardoI doubt this assmption holds:
what if multiple wasm modules points to the same `ScriptResource`?
e.g. when we import "a.wasm" and "a.wasm#foo" in the same page. MemoryCache ignores `#foo` part and thus I expect the same ScriptResource is used.Could you check this, and add a corresponding wpt (or wpt_internal) test (if you find the current patch set crashes here)?
---
I suspect this might be crashing or incorrectly behaving also when:
- multiple same-origin pages import the same WASM modules
- the `ScriptResource` is used WASM module and classic script (which anyway cause script parse error or something, but shouldn't crash or cause other internal inconsistencies)
but these might be a little harder to test/reproduce, so just checking the condition above is sufficient.
Hiroshige HayashizakiYou are right, we do need to keep the data around. For now, I added a new `source_bytes_` field in this class to keep it in memory, but I'm not sure if this is the right long-term approach.
If the resource is going to stay in memory for a long time it would be better to have the source in a "parkable" object, but using a `ParkableString` for a non-string buffer seems hacky. WDYT?
At least we shouldn't call `ClearData();` because it would invalidate the data that would be otherwise
Done
Do we need `source_bytes_` here?
Can we always get bytes directly from `Data()` without `ClearData()`?
- Hiroshige Hayashizaki
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 14 comments![]( "Open in Gerrit")
Leaning toward landing this CL with a separate follow-up for crbug.com/425682456.
I added comments to make the contract/semantics more strict, as a preparation for the follow-up.
Support for Wasm modules was already added, but the sources areThe code isn't executed in the wild (unless developers explicitly enable the feature), right?
i.e. The feature kJavaScriptSourcePhaseImports is disabled and not enabled by any experiments etc.
If so, it's fine to discuss on the follow-up CL.
I basically expect selectively keeping raw bytes would work, while we should be careful.
(In other words, the fix should be applied before the code can run in the wild, because the `FlatData()` check could crash the Chromium in the wild at non-negligible rate)
// Cannot be const to support the move constructor.Is this still true? I feel this can be still const just like other const fields.
TestFetchURL(ModuleScriptCustomFetchType::kNone, client,Can this non-ASCII test be a WPT?
Not blocking this CL, but for the long term adding as WPT is better.
(very nit: maybe this is more like non-UTF8 rather than non-ASCII?)
// `ResolvedModuleType::kWasm`, and `ParkableString` otherwise.nit: we can also add the same TODO as `module_script_creation_params.h`:
```
// TODO(https://crbug.com/42204365): Wrap this and the module type in a
// new class.
```
base::HeapArray<uint8_t> FlatData();Could you rename this to `GetWasmSource()`?
Also add "for Wasm sources" in the comment.
- To align with ModuleScriptCreationParams
- Because I don't expect we can support non-WASM use cases due to https://issues.chromium.org/425682456.
// The buffer is not stored within this class."The returned buffer is not stored within this class." is clearer.
// Returns a flattened representation of the Data and clears the buffer." and clears the buffer" part is obsolete.
ResolvedModuleType module_type);Could you rename this to `GetSourceTextOrWasmSource()`?
(Assuming FlatData() is renamed to GetWasmSource())
So that the name directly corresponds to underlying GetWasmSource() and SourceText().
std::variant<ParkableString, base::HeapArray<uint8_t>> GetBytesOrTextSource(Could you add comments:
About return type:
```
For module purposes.
Returns the wire bytes for Wasm modules, or otherwise, the decoded text.
See `ModuleScriptCreationParams::source_`.
```
About module_type:
```
`module_type` should be from `ModuleScriptFetcher::WasModuleLoadSuccessful(this,...)`.
Particularly, if `module_type` is `kWasm`, then the Content Type of `this` should be a WASM MIME type (See the corresponding `CHECK()` in `FlatData()`).
```
// If needed, decode the source buffer into a ParkableString.This is a little confusing, because even without calling `SourceText`, the source buffer can be decoded.
So maybe this should be:
```
// Returns the decoded source text as a ParkableString.
```
// Data is not cleared for Wasm resources.Could you add a TODO? like
```
TODO(https://crbug.com/425682456): Currently this assumption doesn't hold.
```
CHECK(Data());Could you add
```
CHECK(IsLoaded());
CHECK(base::FeatureList::IsEnabled(
blink::features::kJavaScriptSourcePhaseImports));
CHECK(MIMETypeRegistry::IsWasmMIMEType(GetResponse().HttpContentType()));
```
// Wasm resources are not decoded, so we return an empty string.nit: Creating a separate paragraph + adding a word "also" might be easier to read.
```
// ... or we either haven't started loading and haven't received data yet, or
// we finished loading with an error/cancellation, and thus don't have data.
// In both cases, we can treat the resource as empty.
// Also Wasm resources are not decoded, so we return an empty string.
// TODO(https://crbug.com/42204365): Investigate if we need inspector support.
```
Related details
- Luis Pardo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
Bug: 42204365Could you also add `425682456`? (Because this CL introduces the behavior of crbug.com/425682456)
Related details
- Luis Pardo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 14 comments![]( "Open in Gerrit")
It isn't executed in the wild. Modules with `application/wasm` mime type are only allowed when the JavaScriptSourcePhaseImports feature is enabled, see check in [ModuleScriptFetcher::WasModuleLoadSuccesful](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/loader/modulescript/module_script_fetcher.cc;bpv=1;bpt=1;l=37?q=WasModuleLoadSuccessful&gsn=WasModuleLoadSuccessful&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dthird_party%2Fblink%2Frenderer%2Fcore%2Floader%2Fmodulescript%2Fmodule_script_fetcher.cc%23yzMJZNap6a8jc1RQY0zVXRnmJ5xPf82N-EN7N_8g5Es)
Could you also add `425682456`? (Because this CL introduces the behavior of crbug.com/425682456)
Done
// Cannot be const to support the move constructor.Is this still true? I feel this can be still const just like other const fields.
It's still true. The move constructor defaults to the copy constructor for the const members, all the other members are trivial or have a copy constructor (like KURL), but the new variant doesn't because `HeapArray` is move only, and even if it had, we wouldn't like to be copying this memory instead of moving it.
TestFetchURL(ModuleScriptCustomFetchType::kNone, client,Can this non-ASCII test be a WPT?
Not blocking this CL, but for the long term adding as WPT is better.(very nit: maybe this is more like non-UTF8 rather than non-ASCII?)
Can this non-ASCII test be a WPT?
I'll add it in a follow-up CL. Would you prefer it as a WPT internal or external test? I'm trying to understand if this would be a Chomium-specific bug or is general enough to be relevant to other browsers.
(very nit: maybe this is more like non-UTF8 rather than non-ASCII?)
Updated it
// `ResolvedModuleType::kWasm`, and `ParkableString` otherwise.nit: we can also add the same TODO as `module_script_creation_params.h`:
```
// TODO(https://crbug.com/42204365): Wrap this and the module type in a
// new class.
```
Done
Could you rename this to `GetWasmSource()`?
Also add "for Wasm sources" in the comment.
- To align with ModuleScriptCreationParams
- Because I don't expect we can support non-WASM use cases due to https://issues.chromium.org/425682456.
Done
// The buffer is not stored within this class."The returned buffer is not stored within this class." is clearer.
Done
// Returns a flattened representation of the Data and clears the buffer." and clears the buffer" part is obsolete.
Done
Could you rename this to `GetSourceTextOrWasmSource()`?
(Assuming FlatData() is renamed to GetWasmSource())So that the name directly corresponds to underlying GetWasmSource() and SourceText().
Done
std::variant<ParkableString, base::HeapArray<uint8_t>> GetBytesOrTextSource(Could you add comments:
About return type:
```
For module purposes.
Returns the wire bytes for Wasm modules, or otherwise, the decoded text.
See `ModuleScriptCreationParams::source_`.
```About module_type:
```
`module_type` should be from `ModuleScriptFetcher::WasModuleLoadSuccessful(this,...)`.
Particularly, if `module_type` is `kWasm`, then the Content Type of `this` should be a WASM MIME type (See the corresponding `CHECK()` in `FlatData()`).
```
Done
// If needed, decode the source buffer into a ParkableString.This is a little confusing, because even without calling `SourceText`, the source buffer can be decoded.
So maybe this should be:
```
// Returns the decoded source text as a ParkableString.
```
Done
Could you add a TODO? like
```
TODO(https://crbug.com/425682456): Currently this assumption doesn't hold.
```
Done
Could you add
```
CHECK(IsLoaded());
CHECK(base::FeatureList::IsEnabled(
blink::features::kJavaScriptSourcePhaseImports));
CHECK(MIMETypeRegistry::IsWasmMIMEType(GetResponse().HttpContentType()));
```
Done
// Wasm resources are not decoded, so we return an empty string.nit: Creating a separate paragraph + adding a word "also" might be easier to read.
```
// ... or we either haven't started loading and haven't received data yet, or
// we finished loading with an error/cancellation, and thus don't have data.
// In both cases, we can treat the resource as empty.// Also Wasm resources are not decoded, so we return an empty string.
// TODO(https://crbug.com/42204365): Investigate if we need inspector support.
```
- Hiroshige Hayashizaki
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki voted and added 4 comments![]( "Open in Gerrit")
Votes added by Hiroshige Hayashizaki
| Code-Review | +1 |
4 comments
Support for Wasm modules was already added, but the sources aresg. I wanted to just confirm the `JavaScriptSourcePhaseImports` feature isn't enabled by any experiments.
// Cannot be const to support the move constructor.Luis PardoIs this still true? I feel this can be still const just like other const fields.
It's still true. The move constructor defaults to the copy constructor for the const members, all the other members are trivial or have a copy constructor (like KURL), but the new variant doesn't because `HeapArray` is move only, and even if it had, we wouldn't like to be copying this memory instead of moving it.
Er, I see, we have to move out. Thank you for clarification.
TestFetchURL(ModuleScriptCustomFetchType::kNone, client,Luis PardoCan this non-ASCII test be a WPT?
Not blocking this CL, but for the long term adding as WPT is better.(very nit: maybe this is more like non-UTF8 rather than non-ASCII?)
Can this non-ASCII test be a WPT?
I'll add it in a follow-up CL. Would you prefer it as a WPT internal or external test? I'm trying to understand if this would be a Chomium-specific bug or is general enough to be relevant to other browsers.
(very nit: maybe this is more like non-UTF8 rather than non-ASCII?)
Updated it
sg.
Would you prefer it as a WPT internal or external test?
I basically prefer external WPT, to make internal tests really internal-only (those with Chromium-specific behavior expectations or Chromium-specific test infra).
The test might be specific to Chromium bug, but anyway other browsers should still pass the test without any bugs (and it's also theoretically possible to catch bugs in other browsers e.g. Safari due to the common code between Blink/WebKit).
Related details
- Luis Pardo
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 1 comment![]( "Open in Gerrit")
TestFetchURL(ModuleScriptCustomFetchType::kNone, client,Luis PardoCan this non-ASCII test be a WPT?
Not blocking this CL, but for the long term adding as WPT is better.(very nit: maybe this is more like non-UTF8 rather than non-ASCII?)
Hiroshige HayashizakiCan this non-ASCII test be a WPT?
I'll add it in a follow-up CL. Would you prefer it as a WPT internal or external test? I'm trying to understand if this would be a Chomium-specific bug or is general enough to be relevant to other browsers.
(very nit: maybe this is more like non-UTF8 rather than non-ASCII?)
Updated it
sg.
Would you prefer it as a WPT internal or external test?
I basically prefer external WPT, to make internal tests really internal-only (those with Chromium-specific behavior expectations or Chromium-specific test infra).
The test might be specific to Chromium bug, but anyway other browsers should still pass the test without any bugs (and it's also theoretically possible to catch bugs in other browsers e.g. Safari due to the common code between Blink/WebKit).
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo added 1 comment![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +1 |
1 comment
- Luis Pardo
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Pardo (Gerrit)
Luis Pardo voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[source-phase-imports] Don't use ParkableStrings for Wasm sources
Support for Wasm modules was already added, but the sources are
currently stored in ParkableStrings and going through the same decoding
as text sources. This is wrong because the Wasm compiler works directly
with the wire bytes; and it's buggy because the presence of non-UTF8
"characters" in the sources will lift the string to 16-bit storage,
corrupting the Wasm source.
Main changes:
- Replace the source_text_ ParkableString class member in
ModuleScriptCreationParams with a variant holding either a HeapArray
(for wasm) or a ParkableString (otherwise).
- Add a GetBytesOrTextSource method to ScriptResource to retrieve the
variant with the source.
- Add a wpt internal to test that the same resource can be requested
twice in the same module.
Drive by:
- WasmModuleScript: spanify the empty wasm byte sequence constant.
- Update outdated comments in ScriptResource.
- M third_party/blink/renderer/core/loader/modulescript/document_module_script_fetcher.cc
- M third_party/blink/renderer/core/loader/modulescript/module_script_creation_params.cc
- M third_party/blink/renderer/core/loader/modulescript/module_script_creation_params.h
- M third_party/blink/renderer/core/loader/modulescript/module_script_loader_test.cc
- M third_party/blink/renderer/core/loader/modulescript/worker_module_script_fetcher.cc
- M third_party/blink/renderer/core/loader/modulescript/worker_module_script_fetcher.h
- M third_party/blink/renderer/core/loader/modulescript/worklet_module_script_fetcher.cc
- M third_party/blink/renderer/core/loader/resource/script_resource.cc
- M third_party/blink/renderer/core/loader/resource/script_resource.h
- M third_party/blink/renderer/core/script/module_map_test.cc
- M third_party/blink/renderer/core/script/wasm_module_script.cc
- M third_party/blink/renderer/core/script/wasm_module_script.h
- M third_party/blink/renderer/core/testing/data/core_test_bundle_data.filelist
- A third_party/blink/renderer/core/testing/data/non_UTF8.wasm
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/virtual/javascript-source-phase-imports/wpt_internal/js/source-phase-imports/import-same-wasm-resource-twice-expected.txt
- A third_party/blink/web_tests/wpt_internal/js/source-phase-imports/import-same-wasm-resource-twice-expected.txt
- A third_party/blink/web_tests/wpt_internal/js/source-phase-imports/import-same-wasm-resource-twice.html
- A third_party/blink/web_tests/wpt_internal/js/source-phase-imports/resources/exported-names.wasm
Code-Review: +1 by Dominic Farolino, +1 by Hiroshige Hayashizaki
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |