[browser][wasm] Fix buffered symbol asset loading#127087
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes buffered symbols runtime asset loading in the browser WASM loader, and adds a regression scenario in WBT to exercise supplying runtime assets via asset.buffer.
Changes:
- Adjusted asset instantiation flow to correctly handle
symbolsassets when provided viabuffer(and enabled buffered-response.text()decoding). - Added a
BufferedAssetsTestscenario toWasmBasicTestAppthat assignsbufferfor wasm/assemblies/pdbs/symbols duringonConfigLoaded. - Added a matching
ModuleConfigTests.BufferedAssetsTestpublish-and-run integration test.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/mono/wasm/testassets/WasmBasicTestApp/App/wwwroot/main.js | Adds the BufferedAssetsTest scenario that supplies runtime assets via asset.buffer. |
| src/mono/wasm/Wasm.Build.Tests/ModuleConfigTests.cs | Adds an integration test that publishes and runs the new scenario (with symbol map emission enabled). |
| src/mono/browser/runtime/loader/assets.ts | Fixes loader handling for buffered symbols assets and implements .text() for buffered-response shim. |
|
It seems the fingerprint stuff doesn't support file names with hashes. Is it somehow possible to generate the symbols file without the hash? Or maybe I should also patch the fingerprint resolver to ignore the hash on symbols?
|
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
|
The thing is, runtime/src/mono/wasm/Wasm.Build.Tests/WasmSdkBasedProjectProvider.cs Lines 28 to 48 in 6f2ad2d |
|
I think the logic that asserts the files incorrectly finds where the fingerprint should be, see https://github.com/elringus/dotnet/blob/fix/symbols-buffer/src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs#L116 ( |
|
Would it be ok to add a special case for |
@maraf is that what you mean ? |
@elringus I'm sorry for late reply. Yes, we can add a special case for |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/mono/browser/runtime/loader/assets.ts:1
TextDecoderis constructed on everyjson()/text()call. While likely not hot, it’s avoidable overhead and easy to fix by reusing a singleTextDecoderinstance within the closure (or higher scope) and callingdecoder.decode(buffer)in both methods.
// Licensed to the .NET Foundation under one or more agreements.
| const originalFetch4 = globalThis.fetch.bind(globalThis); | ||
| dotnet.withModuleConfig({ | ||
| onConfigLoaded: (config) => { | ||
| const bufferedAssets = [ | ||
| ...config.resources.wasmNative, | ||
| ...config.resources.assembly, | ||
| ...config.resources.pdb, | ||
| ...config.resources.wasmSymbols, | ||
| ]; |
There was a problem hiding this comment.
It's controlled by the test fixtures — nothing can be missing here.
| for (const asset of bufferedAssets) { | ||
| const url = new URL(`./_framework/${asset.name}`, location.href); | ||
| asset.buffer = originalFetch4(url).then(r => r.arrayBuffer()); | ||
| } |
| dotnet.withApplicationArgumentsFromQuery(); | ||
| break; | ||
| case "BufferedAssetsTest": | ||
| const originalFetch4 = globalThis.fetch.bind(globalThis); |
There was a problem hiding this comment.
The name is following the established pattern in the file. Changing the pattern is out of scope of this PR.
|
I've renamed the conflicting fetch variable and added support for fingerprinting the symbols file. |
| const bufferedAssets = [ | ||
| ...config.resources.wasmNative, | ||
| ...config.resources.assembly, | ||
| ...config.resources.pdb, | ||
| ...config.resources.wasmSymbols, | ||
| ]; |
There was a problem hiding this comment.
Already commented above.
| const headersOnly = skipBufferByAssetTypes[asset.behavior]; | ||
|
|
||
| if (headersOnly) { | ||
| if (asset.behavior === "symbols") { | ||
| await runtimeHelpers.instantiate_symbols_asset(asset); | ||
| cleanupAsset(asset); | ||
| } | ||
| ++loaderHelpers.actual_downloaded_assets_count; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Out of scope for this PR.
| text: () => { | ||
| throw new Error("NotImplementedException"); | ||
| }, | ||
| text: () => new TextDecoder("utf-8").decode(buffer), |
There was a problem hiding this comment.
Out of scope for this PR.
This fixes
SymbolsAsset.bufferhandling in the browser runtime and adds a WBT regression test for buffered runtime assets:symbolsassets supplied viabufferare handled correctly, instead of effectively requiringpendingDownloadBufferedAssetsTesttoWasmBasicTestAppthat supplies these runtime assets viabuffer:dotnetwasmModuleConfigTests.BufferedAssetsTestintegration testThe issue was discussed with @pavelsavara on .NET's Discord server.
Note: I was not able to run the browser test end-to-end in the local environment due local WBT/workload packaging setup issues. If anything fails on CI, I’ll follow up and fix the remaining issues there.