[L] Change in dart/sdk[main]: [dart2wasm] Catch JS exceptions with the right tag, improve toString ...
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
return translator.mainModule.tags.import('WebAssembly', 'JSTag', tagType);There seems to be some code that messes with the import name here. @nate...@google.com do you know which code this may be? Example error: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8695131615160763905/+/u/test_results/new_test_failures__logs_
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 1 comment![]( "Open in Gerrit")
return translator.mainModule.tags.import('WebAssembly', 'JSTag', tagType);There seems to be some code that messes with the import name here. @nate...@google.com do you know which code this may be? Example error: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8695131615160763905/+/u/test_results/new_test_failures__logs_
`_importedTags` is a `WasmTagImporter` which will try to minify names (if minification is enabled):
https://github.com/dart-lang/sdk/blob/main/pkg/dart2wasm/lib/translator.dart#L3221
You actually probably don't want to use the Importer here. The Importer is meant to help import/export from other Dart modules (e.g. for deferred loading). It maps "defined" entities to their "imports" in other modules.
So I think it would make sense to just keep a separate `Map<w.ModuleBuilder, w.ImportedTag>` here that tracks the imported tag entity for each module:
```
late final w.FunctionType _exnTagType = translator.typesBuilder
.defineFunction(const [w.RefType.extern(nullable: true)], const []);
w.Tag getJsExceptionTag(w.ModuleBuilder module) => _jsTagsByModule[module] ??=
_defineJsExceptionTag(module);
w.ImportedTag _defineJsExceptionTag(w.ModuleBuilder module) {
return module.tags.import('WebAssembly', 'JSTag', _exnTagType);
}
```Related details
- Martin Kustermann
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 1 comment![]( "Open in Gerrit")
As this is, I think this will break Flutter apps.
The Flutter JS fallback logic isn't checking that these instructions are supported. So someone on an older browser might load a Flutter app and get served WASM that their browser can't execute.
Same thing for another framework like Jaspr, they all need to update their fallback logic to account for this.
Part of this is updating the `support.js` we emit. But I think Flutter might not be using that file so we should be careful there.
Related details
- Martin Kustermann
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs added 4 comments![]( "Open in Gerrit")
As this is, I think this will break Flutter apps.
The Flutter JS fallback logic isn't checking that these instructions are supported. So someone on an older browser might load a Flutter app and get served WASM that their browser can't execute.
Same thing for another framework like Jaspr, they all need to update their fallback logic to account for this.
Part of this is updating the `support.js` we emit. But I think Flutter might not be using that file so we should be careful there.
I guess there isn't any new instruction here per se but there is a new type. Same argument still applies I think.
b.ref_as_non_null();See below about making `javaScriptErrorStackTraceGetter` return non-nullable value. Should be able to delete this cast.
return translator.mainModule.tags.import('WebAssembly', 'JSTag', tagType);Nate BiggsThere seems to be some code that messes with the import name here. @nate...@google.com do you know which code this may be? Example error: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8695131615160763905/+/u/test_results/new_test_failures__logs_
`_importedTags` is a `WasmTagImporter` which will try to minify names (if minification is enabled):
https://github.com/dart-lang/sdk/blob/main/pkg/dart2wasm/lib/translator.dart#L3221You actually probably don't want to use the Importer here. The Importer is meant to help import/export from other Dart modules (e.g. for deferred loading). It maps "defined" entities to their "imports" in other modules.
So I think it would make sense to just keep a separate `Map<w.ModuleBuilder, w.ImportedTag>` here that tracks the imported tag entity for each module:
```
late final w.FunctionType _exnTagType = translator.typesBuilder
.defineFunction(const [w.RefType.extern(nullable: true)], const []);w.Tag getJsExceptionTag(w.ModuleBuilder module) => _jsTagsByModule[module] ??=
_defineJsExceptionTag(module);w.ImportedTag _defineJsExceptionTag(w.ModuleBuilder module) {
return module.tags.import('WebAssembly', 'JSTag', _exnTagType);
}
```
This also requires making sure all the other module instantiation points are passing in the WebAssembly object. I think most are via `baseImports` in `runtime_blob.dart`. But it seems like the dynamic module instantiation might not be (see line 243 in `runtime_blob.dart`).
StackTrace? get stackTrace =>Can we type this as non-nullable? It should be okay if an override has a narrower type (i.e. without null).
Related details
- Martin Kustermann
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 3 comments![]( "Open in Gerrit")
Nate BiggsAs this is, I think this will break Flutter apps.
The Flutter JS fallback logic isn't checking that these instructions are supported. So someone on an older browser might load a Flutter app and get served WASM that their browser can't execute.
Same thing for another framework like Jaspr, they all need to update their fallback logic to account for this.
Part of this is updating the `support.js` we emit. But I think Flutter might not be using that file so we should be careful there.
I guess there isn't any new instruction here per se but there is a new type. Same argument still applies I think.
There are no new stuff in this CL. The new instructions are in another CL.
The only new thing in terms of Wasm features in this CL is the new tag that we import. It's not a feature on its own and there's no easy way to find out when browsers added that tag, but given that it's a part of the legacy exception handling I think it should be safe to assume that most browsers should be supporting the new tag for a while now.
See below about making `javaScriptErrorStackTraceGetter` return non-nullable value. Should be able to delete this cast.
I updated its type but it's an override so its type doesn't change.
Can we type this as non-nullable? It should be okay if an override has a narrower type (i.e. without null).
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
return translator.mainModule.tags.import('WebAssembly', 'JSTag', tagType);Nate BiggsThere seems to be some code that messes with the import name here. @nate...@google.com do you know which code this may be? Example error: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8695131615160763905/+/u/test_results/new_test_failures__logs_
Nate Biggs`_importedTags` is a `WasmTagImporter` which will try to minify names (if minification is enabled):
https://github.com/dart-lang/sdk/blob/main/pkg/dart2wasm/lib/translator.dart#L3221You actually probably don't want to use the Importer here. The Importer is meant to help import/export from other Dart modules (e.g. for deferred loading). It maps "defined" entities to their "imports" in other modules.
So I think it would make sense to just keep a separate `Map<w.ModuleBuilder, w.ImportedTag>` here that tracks the imported tag entity for each module:
```
late final w.FunctionType _exnTagType = translator.typesBuilder
.defineFunction(const [w.RefType.extern(nullable: true)], const []);w.Tag getJsExceptionTag(w.ModuleBuilder module) => _jsTagsByModule[module] ??=
_defineJsExceptionTag(module);w.ImportedTag _defineJsExceptionTag(w.ModuleBuilder module) {
return module.tags.import('WebAssembly', 'JSTag', _exnTagType);
}
```
This also requires making sure all the other module instantiation points are passing in the WebAssembly object. I think most are via `baseImports` in `runtime_blob.dart`. But it seems like the dynamic module instantiation might not be (see line 243 in `runtime_blob.dart`).
We could use the importer and import from the main module, or import directly.
- The main module could export the imported `WebAssembly.JSTag` tag with a minified name and others could import.
- Or each module could import `WebAssembly.JSTag`.
I think the first option should generate smaller binaries. "WebAssembly" (11 bytes) and "JSTag" (5 bytes) are 16 bytes in total. It could reduce to two bytes when imported from the main module.
Main module will also have to export the tag so that'll add a few bytes, but it's one time cost for the whole package (unlike imports which will be in many or maybe even most of the modules).
Rest of the encoding should be the same in both cases (the function type needs to be defined in each of the modules).
I think this code already implements the first option, but there should be some bug when splitting modules because the tag is being imported in submodules even when they're never used in the submodules. I'll investigate.
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ömer Ağacan (Gerrit)
Ömer Ağacan added 1 comment![]( "Open in Gerrit")
return translator.mainModule.tags.import('WebAssembly', 'JSTag', tagType);Nate BiggsThere seems to be some code that messes with the import name here. @nate...@google.com do you know which code this may be? Example error: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket/8695131615160763905/+/u/test_results/new_test_failures__logs_
Nate Biggs`_importedTags` is a `WasmTagImporter` which will try to minify names (if minification is enabled):
https://github.com/dart-lang/sdk/blob/main/pkg/dart2wasm/lib/translator.dart#L3221You actually probably don't want to use the Importer here. The Importer is meant to help import/export from other Dart modules (e.g. for deferred loading). It maps "defined" entities to their "imports" in other modules.
So I think it would make sense to just keep a separate `Map<w.ModuleBuilder, w.ImportedTag>` here that tracks the imported tag entity for each module:
```
late final w.FunctionType _exnTagType = translator.typesBuilder
.defineFunction(const [w.RefType.extern(nullable: true)], const []);w.Tag getJsExceptionTag(w.ModuleBuilder module) => _jsTagsByModule[module] ??=
_defineJsExceptionTag(module);w.ImportedTag _defineJsExceptionTag(w.ModuleBuilder module) {
return module.tags.import('WebAssembly', 'JSTag', _exnTagType);
}
```
Ömer AğacanThis also requires making sure all the other module instantiation points are passing in the WebAssembly object. I think most are via `baseImports` in `runtime_blob.dart`. But it seems like the dynamic module instantiation might not be (see line 243 in `runtime_blob.dart`).
We could use the importer and import from the main module, or import directly.
- The main module could export the imported `WebAssembly.JSTag` tag with a minified name and others could import.
- Or each module could import `WebAssembly.JSTag`.
I think the first option should generate smaller binaries. "WebAssembly" (11 bytes) and "JSTag" (5 bytes) are 16 bytes in total. It could reduce to two bytes when imported from the main module.
Main module will also have to export the tag so that'll add a few bytes, but it's one time cost for the whole package (unlike imports which will be in many or maybe even most of the modules).
Rest of the encoding should be the same in both cases (the function type needs to be defined in each of the modules).
I think this code already implements the first option, but there should be some bug when splitting modules because the tag is being imported in submodules even when they're never used in the submodules. I'll investigate.
Done
Related details
- Martin Kustermann
- Nate Biggs
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Biggs (Gerrit)
Nate Biggs voted and added 3 comments![]( "Open in Gerrit")
Votes added by Nate Biggs
| Code-Review | +1 |
Nate BiggsAs this is, I think this will break Flutter apps.
The Flutter JS fallback logic isn't checking that these instructions are supported. So someone on an older browser might load a Flutter app and get served WASM that their browser can't execute.
Same thing for another framework like Jaspr, they all need to update their fallback logic to account for this.
Part of this is updating the `support.js` we emit. But I think Flutter might not be using that file so we should be careful there.
Ömer AğacanI guess there isn't any new instruction here per se but there is a new type. Same argument still applies I think.
There are no new stuff in this CL. The new instructions are in another CL.
The only new thing in terms of Wasm features in this CL is the new tag that we import. It's not a feature on its own and there's no easy way to find out when browsers added that tag, but given that it's a part of the legacy exception handling I think it should be safe to assume that most browsers should be supporting the new tag for a while now.
Ah my bad, I was conflating this CL with your other one.
b.ref_as_non_null();Ömer AğacanSee below about making `javaScriptErrorStackTraceGetter` return non-nullable value. Should be able to delete this cast.
I updated its type but it's an override so its type doesn't change.
I forgot that function types are invariant across the board so we can't give the override a better type.
.defineFunction([w.RefType.extern(nullable: true)], const []);nit: This can be const.
Related details
- Martin Kustermann
- Ömer Ağacan
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |