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

[wasm][debugger] Showing "Frame not in module" after vscode-js-debug bump on VS #87154

Merged

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Jun 5, 2023

In a blazor app with authentication enabled, 4 execution contexts are created, but the last two of them are created for the authentication thing and then they are destroyed when the login process is over. Once they are destroyed BrowserDebugProxy should not send scriptID's from this context when there is a Debugger.Pause, it should send the scriptId of the last not destroyed context.

Before this PR we were sending the scriptId of a context that was already destroyed and vscode-js-debug extension started to consider this information that was ignored before.

In my opinion we should revert vscode-js-debug change until we have versions that fixes it on .net6, .net7 and .net8.

fixes #86754, #87407
@joj @connor4312 @radical @danroth27

…and vscode-js-debug extension started to consider this information that was ignored before.
@thaystg thaystg added arch-wasm WebAssembly architecture area-Debugger-mono labels Jun 5, 2023
@thaystg thaystg requested a review from radical as a code owner June 5, 2023 22:41
@ghost ghost assigned thaystg Jun 5, 2023
@ghost
Copy link

ghost commented Jun 5, 2023

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

Issue Details

We were sending the scriptId of a context that was already destroyed and vscode-js-debug extension started to consider this information that was ignored before.
Blazor Wasm debugging is completely broken in apps that have authentication.
In my opinion we should revert vscode-js-debug change until we have versions that fixes it on .net6, .net7 and .net8.

fixes #86754
@joj @connor4312 @radical

Author: thaystg
Assignees: -
Labels:

arch-wasm, area-Debugger-mono

Milestone: -

@connor4312
Copy link

Node.js also has a similar problem that we have a workaround for. I'm okay putting in a similar workaround for Blazor until this gets fixed. Or maybe the debugger should just create execution context IDs if they don't exist and shrug its shoulder at implementors who "misbehave". This means scripts will not get cleared correctly on navigation or refresh, but it's better than breaking.

@radical
Copy link
Member

radical commented Jun 6, 2023

cc @javiercn

@thaystg
Copy link
Member Author

thaystg commented Jun 7, 2023

@radical can you please review?

@radical
Copy link
Member

radical commented Jun 7, 2023

We were sending the scriptId of a context that was already destroyed and vscode-js-debug extension started to consider this information that was ignored before.

When was this scriptId being sent? And how does that change with this PR?

@thaystg
Copy link
Member Author

thaystg commented Jun 7, 2023

We were sending the scriptId of a context that was already destroyed and vscode-js-debug extension started to consider this information that was ignored before.

When was this scriptId being sent? And how does that change with this PR?

When we have a debugger.paused, the scriptId of the callstack is being sent.

As we were considering the CURRENT context the last one created, but this one was already destroyed, so we need to send the scriptID for the last not deleted context.

@radical
Copy link
Member

radical commented Jun 7, 2023

We were sending the scriptId of a context that was already destroyed and vscode-js-debug extension started to consider this information that was ignored before.

When was this scriptId being sent? And how does that change with this PR?

When we have a debugger.paused, the scriptId of the callstack is being sent.

As we were considering the CURRENT context the last one created, but this one was already destroyed, so we need to send the scriptID for the last not deleted context.

And how does this happen? Is the previous not deleted context still valid?

@thaystg
Copy link
Member Author

thaystg commented Jun 7, 2023

We were sending the scriptId of a context that was already destroyed and vscode-js-debug extension started to consider this information that was ignored before.

When was this scriptId being sent? And how does that change with this PR?

When we have a debugger.paused, the scriptId of the callstack is being sent.
As we were considering the CURRENT context the last one created, but this one was already destroyed, so we need to send the scriptID for the last not deleted context.

And how does this happen? Is the previous not deleted context still valid?

In a page that has OIDC authentication for example.
It creates context 1 2 3 and 4, destroys 3 and 4, and the 1 and 2 continues valid. So we need to send the scriptID of the context 2.

@radical
Copy link
Member

radical commented Jun 7, 2023

Can you please add these details to the PR description?

@ilonatommy
Copy link
Member

/azp run runtime-wasm-dbgtests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ilonatommy ilonatommy merged commit 5fe7b06 into dotnet:main Jun 19, 2023
24 checks passed
@ilonatommy
Copy link
Member

/backport to release/7.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/5310830931

@github-actions
Copy link
Contributor

@ilonatommy backporting to release/7.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: We were sending the scriptId of a context that was already destroyed and vscode-js-debug extension started to consider this information that was ignored before.
Using index info to reconstruct a base tree...
M	src/mono/wasm/debugger/BrowserDebugProxy/Firefox/FirefoxMonoProxy.cs
M	src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
Falling back to patching base and 3-way merge...
Auto-merging src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
CONFLICT (content): Merge conflict in src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
Auto-merging src/mono/wasm/debugger/BrowserDebugProxy/Firefox/FirefoxMonoProxy.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 We were sending the scriptId of a context that was already destroyed and vscode-js-debug extension started to consider this information that was ignored before.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@ilonatommy an error occurred while backporting to release/7.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

thaystg added a commit to thaystg/runtime that referenced this pull request Jul 3, 2023
thaystg added a commit that referenced this pull request Jul 10, 2023
…fter vscode-js-debug bump on VS (#88351)

* Backport #87154 #87870 #87979

* fix compilation error
thaystg added a commit that referenced this pull request Jul 10, 2023
…fter vscode-js-debug bump on VS (#88336)

* Backport #87154 #87870 #87979

* fix compilation

* Backporting more changes
@ghost ghost locked as resolved and limited conversation to collaborators Jul 19, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Frame not in module when hitting a breakpoint in Blazor WASM
4 participants