Skip to content

Better error handling in SubtleCrypto workers - #71693

Merged
eerhardt merged 5 commits into
dotnet:mainfrom
eerhardt:SubtleCryptoErrorHandling2
Jul 7, 2022
Merged

eerhardt merged 5 commits into
dotnet:mainfrom
eerhardt:SubtleCryptoErrorHandling2

Conversation

@eerhardt

@eerhardt eerhardt commented Jul 6, 2022

Copy link
Copy Markdown
Member

Handle exceptions from SubtleCrypto by catching and logging exceptions coming from the crypto stack.

Reset web worker when a request fails.

Also, fix race conditions where the web worker can read its own response as part of the next request.

Contributes to #69740

Handle exceptions from SubtleCrypto by catching and logging exceptions coming from the crypto stack.

Reset web worker when a request fails.

Also, fix race conditions where the web worker can read its own response as part of the next request.

Contributes to dotnet#69740
@eerhardt
eerhardt requested a review from radical July 6, 2022 01:56
@eerhardt
eerhardt requested a review from lewing as a code owner July 6, 2022 01:56
@ghost

ghost commented Jul 6, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Handle exceptions from SubtleCrypto by catching and logging exceptions coming from the crypto stack.

Reset web worker when a request fails.

Also, fix race conditions where the web worker can read its own response as part of the next request.

Contributes to #69740

Author: eerhardt
Assignees: -
Labels:

area-System.Security

Milestone: -

@ghost ghost assigned eerhardt Jul 6, 2022
Comment thread src/mono/wasm/runtime/crypto-worker.ts Outdated
@eerhardt eerhardt added the arch-wasm WebAssembly architecture label Jul 6, 2022
@ghost

ghost commented Jul 6, 2022

Copy link
Copy Markdown

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

Handle exceptions from SubtleCrypto by catching and logging exceptions coming from the crypto stack.

Reset web worker when a request fails.

Also, fix race conditions where the web worker can read its own response as part of the next request.

Contributes to #69740

Author: eerhardt
Assignees: eerhardt
Labels:

arch-wasm, area-System.Security

Milestone: -

Format the code
Fix args bug in setup_proxy_console
Comment thread src/mono/wasm/runtime/workers/dotnet-crypto-worker.js Outdated
@radical

radical commented Jul 6, 2022

Copy link
Copy Markdown
Member

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@radical radical left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We agreed, offline, to leave the debug messages as-is for now. LGTM 👍

@radical
radical requested a review from lambdageek July 6, 2022 18:51
Comment thread src/mono/wasm/runtime/debug.ts Outdated
@radical
radical requested a review from kg July 6, 2022 19:16
@bartonjs

bartonjs commented Jul 6, 2022

Copy link
Copy Markdown
Member

Generally seems OK; but I sort of skimmed the js parts.

@radical

radical commented Jul 6, 2022

Copy link
Copy Markdown
Member

WasmBuildTests failures are unrelated, and can be ignored.

@eerhardt

eerhardt commented Jul 6, 2022

Copy link
Copy Markdown
Member Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@radical

radical commented Jul 7, 2022

Copy link
Copy Markdown
Member

The wasm failures are unrelated to this PR.

for (; ;) {
const lock_state = Atomics.load(this.comm, this.LOCK_IDX);
if (lock_state !== this.LOCK_UNLOCKED)
continue;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This tight spin loop is unfortunate, but as this is all part of a large workaround for sync-over-async, I guess we have nothing better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unfortunately, we don't 😞

@eerhardt
eerhardt merged commit a2990b5 into dotnet:main Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasm WebAssembly architecture area-System.Security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants