-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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] Fixing race condition #64394
[wasm][debugger] Fixing race condition #64394
Conversation
Tagging subscribers to this area: @thaystg Issue DetailsUnfortunatelly I wasn't able to reproduce it in a test case. To reproduce it using a Blazor app was also very difficult. I created a separated buffer to receive the "pauses"(breakpoint, exception, user_break, etc.) from the debugger and another buffer to uses to the responses that we receive from debugger when we ask for some information (locals value, invoke method, object values, etc;) Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1427671 Test case to reproduce: Change the content of FetchData.razor in a Blazor app from the template:
Add a breakpoint on line: 46, step into moving the mouse over the
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we make the
@@ -700,7 +700,7 @@ public PointerValue(long address, int typeId, string varName) | |||
internal class MonoSDBHelper | |||
{ | |||
private static int debuggerObjectId; | |||
private static int cmdId; | |||
private static int cmdId = 1; | |||
private static int GetId() {return cmdId++;} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this to GetNewId
?
@@ -43,7 +43,7 @@ export function mono_wasm_add_dbg_command_received(res_ok: boolean, id: number, | |||
value: base64String | |||
} | |||
}; | |||
commands_received = buffer_obj; | |||
commands_received.set(id, buffer_obj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if there is an existing entry for id
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if there is, should I print a warning?
@@ -700,7 +700,7 @@ public PointerValue(long address, int typeId, string varName) | |||
internal class MonoSDBHelper | |||
{ | |||
private static int debuggerObjectId; | |||
private static int cmdId; | |||
private static int cmdId = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmdId == 0
is a special one now. Can you add a comment for that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small comment but looks good
src/mono/wasm/runtime/debug.ts
Outdated
const { res_ok, res } = commands_received.get(id) as CommandResponse; | ||
commands_received.delete(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest making a method that combines get and delete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…nt_and_get_debugger_informations
Unfortunatelly I wasn't able to reproduce it in a test case. To reproduce it using a Blazor app was also very difficult.
I created a separated buffer to receive the "pauses"(breakpoint, exception, user_break, etc.) from the debugger and another buffer to uses to the responses that we receive from debugger when we ask for some information (locals value, invoke method, object values, etc;)
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1427671
Fixes #64474
Test case to reproduce:
Change the content of FetchData.razor in a Blazor app from the template:
Add a breakpoint on line: 46, step into hovering the mouse over the
WeatherForecast[]
onRetForecasts
method.