Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[browser][MT] fix stale memory on suspended thread #94299

Merged
merged 5 commits into from
Nov 7, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Nov 2, 2023

Alternative to #90771

Problem: when WebWorker is suspended in the browser, the other running threads could grow the linear memory in the meantime. After the thread is un-suspended it may to try to de-reference pointer which is beyond it's known view. This is likely V8 bug.

We believe that described problem is cause of MT problems with debugger tests, example log

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Nov 2, 2023
@pavelsavara pavelsavara added this to the 9.0.0 milestone Nov 2, 2023
@pavelsavara pavelsavara self-assigned this Nov 2, 2023
@ghost
Copy link

ghost commented Nov 2, 2023

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

Issue Details

testing alternative to #90771

Problem: when WebWorker is suspended in the browser, the other running threads could grow the linear memory in the meantime. After the thread is un-suspended it may to try to de-reference pointer which is beyond it's known view. This is likely V8 bug.

We believe that described problem is cause of MT problems with debugger tests, example log

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript, os-browser

Milestone: 9.0.0

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

This helps
image

@pavelsavara pavelsavara marked this pull request as ready for review November 2, 2023 16:08
src/mono/wasm/runtime/memory.ts Outdated Show resolved Hide resolved
src/mono/wasm/runtime/memory.ts Show resolved Hide resolved
@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

@thaystg I will wait for your feedback/approval

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

Hmm, so now all errors are "TypeError: Failed to fetch\n at Object.L [as fetch_like] (http://localhost:9400/_framework/dotnet.js:3:4721)\n at http://localhost:9400/_framework/dotnet.js:3:12002\n at http://localhost:9400/_framework/dotnet.js:3:12018" Log

@thaystg does that mean my fix didn't help ? Or it means there is another issue ?

Is debugger making the app to download something ?

Does suspending thread while fetching break the fetch ?

@thaystg
Copy link
Member

thaystg commented Nov 3, 2023

I think there are a lot of core dumps generated: chrome is crashing, not sure if it's related to your fix.

image

@thaystg
Copy link
Member

thaystg commented Nov 3, 2023

I also remember to have already tried it and it didn't help. But for me it's okay to try again. :)

@pavelsavara
Copy link
Member Author

I think there are a lot of core dumps generated: chrome is crashing, not sure if it's related to your fix.

Where is "there" ?

More details would help.

I also remember to have already tried it and it didn't help. But for me it's okay to try again. :)

Could you please elaborate on what you tried ?

@thaystg
Copy link
Member

thaystg commented Nov 6, 2023

I think there are a lot of core dumps generated: chrome is crashing, not sure if it's related to your fix.

Where is "there" ?

More details would help.

I also remember to have already tried it and it didn't help. But for me it's okay to try again. :)

Could you please elaborate on what you tried ?

Look at artifacts tab here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=459235&view=ms.vss-test-web.build-test-results-tab&runId=10361536&resultId=100252&paneView=dotnet-dnceng.dnceng-build-release-tasks.helix-test-information-tab

image

@thaystg
Copy link
Member

thaystg commented Nov 6, 2023

Could you please elaborate on what you tried ?

Unfortunately I don't remember details of what I tested, I tried a lot of things before the workaround in that PR.
If I can help you in anyway by testing your solution or anything else, please let me know.

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

image

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara
Copy link
Member Author

I think that the fix for memory views is good. Solid 10/10
image

There are problems with #89409 but that's unrelated issue

The active test run was aborted. Reason: Test host process crashed
Results File: /root/helix/work/workitem/uploads/xharness-output/logs/testResults.trx

Passed!  - Failed:     0, Passed:    80, Skipped:     0, Total:    80, Duration: 9 m - DebuggerTestSuite.dll (net8.0)
Test Run Aborted.

@pavelsavara pavelsavara merged commit bd27e27 into dotnet:main Nov 7, 2023
43 of 45 checks passed
@pavelsavara pavelsavara deleted the browser_mt_debugger_memory branch November 7, 2023 10:32
@thaystg
Copy link
Member

thaystg commented Nov 7, 2023

@pavelsavara thanks a lot and congrats for this fix!!!

@github-actions github-actions bot locked and limited conversation to collaborators Dec 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Debugger-mono os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants