Skip to content

[browser][coreCLR] Fix loadBootResourceCallback receiving undefined integrity and refactor config merge#127030

Merged
pavelsavara merged 5 commits intodotnet:mainfrom
pavelsavara:more_integrity
Apr 17, 2026
Merged

[browser][coreCLR] Fix loadBootResourceCallback receiving undefined integrity and refactor config merge#127030
pavelsavara merged 5 commits intodotnet:mainfrom
pavelsavara:more_integrity

Conversation

@pavelsavara
Copy link
Copy Markdown
Member

@pavelsavara pavelsavara commented Apr 16, 2026

Summary

This PR fixes a bug where the loadBootResourceCallback was always receiving undefined for the integrity parameter, and refactors the loader config merge logic for correctness and clarity.

Changes

Bug fix: integrity parameter in loadBootResourceCallback

The AssetEntryInternal.integrity field was never assigned anywhere in the codebase, so both call sites in assets.ts that passed assetInternal.integrity! to loadBootResourceCallback were always passing undefined. The fix replaces this with assetInternal.hash ?? "", using the hash field from the base AssetEntry type which is actually populated from the asset manifest. The dead integrity field is removed from AssetEntryInternal.

Config merge refactor (mergeResources / mergeConfigs)

The old mergeResources wrote merged values into source, then used Object.assign(target, source) to copy everything back to target. This was:

  • Fragile: Object.assign overwrote the already-merged resources object on target with the unmerged one from source.
  • Error-prone for satellite resources: Required two separate loops — one for source keys and one for target-only keys — because target was being replaced wholesale.

The new code writes directly to target, which:

  • Eliminates the need for the second satellite resources loop (target-only keys are already there).
  • Uses || [] fallbacks on source arrays for defensive handling of partial Assets objects.
  • Adds hash merge for the top-level Assets.hash manifest hash.

In mergeConfigs, the merged target.resources is saved before Object.assign(target, source) and restored afterward, so the scalar property copy doesn't overwrite the already-merged resources.

Missing coreVfs handling

The coreVfs field existed on the Assets type but was missing from both mergeResources and normalizeResources. Both functions now handle it.

@pavelsavara pavelsavara added this to the 11.0.0 milestone Apr 16, 2026
@pavelsavara pavelsavara self-assigned this Apr 16, 2026
@pavelsavara pavelsavara added the arch-wasm WebAssembly architecture label Apr 16, 2026
Copilot AI review requested due to automatic review settings April 16, 2026 21:44
@pavelsavara pavelsavara added area-Host os-browser Browser variant of arch-wasm labels Apr 16, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the browser/CoreCLR JavaScript loader configuration and asset handling to improve config merging and align asset metadata with the public API (hash/integrity handling).

Changes:

  • Adjust config merging to mutate the target config/resources directly (and add coreVfs normalization/merge).
  • Switch loadBootResourceCallback integrity argument from asset.integrity to asset.hash.
  • Remove integrity from internal asset entry typings.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
src/native/libs/Common/JavaScript/types/internal.ts Removes internal integrity field from AssetEntryInternal to rely on AssetEntry.hash.
src/native/libs/Common/JavaScript/loader/config.ts Reworks config/resource merge behavior; adds coreVfs normalization and merges resources.hash.
src/native/libs/Common/JavaScript/loader/assets.ts Passes asset.hash into loadBootResourceCallback where integrity was previously used.

Comment thread src/native/libs/Common/JavaScript/loader/config.ts Outdated
Comment thread src/native/libs/Common/JavaScript/loader/assets.ts Outdated
Comment thread src/native/libs/Common/JavaScript/loader/assets.ts Outdated
Comment thread src/native/libs/Common/JavaScript/loader/config.ts Outdated
Copilot AI review requested due to automatic review settings April 16, 2026 22:11
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread src/native/libs/Common/JavaScript/loader/config.ts Outdated
Comment thread src/native/libs/Common/JavaScript/loader/config.ts Outdated
@pavelsavara pavelsavara changed the title [browser][coreCLR] fix config processing [browser][coreCLR] Fix loadBootResourceCallback receiving undefined integrity and refactor config merge Apr 16, 2026
@pavelsavara pavelsavara marked this pull request as ready for review April 17, 2026 06:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@pavelsavara pavelsavara merged commit 7fdf077 into dotnet:main Apr 17, 2026
132 of 146 checks passed
@pavelsavara pavelsavara deleted the more_integrity branch April 17, 2026 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch-wasm WebAssembly architecture area-Host os-browser Browser variant of arch-wasm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants