-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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][debug] Avoid pausing on breakpoint while invoking methods. #81162
Conversation
Tagging subscribers to this area: @thaystg Issue DetailsTrying to fix the error on CI while running test SteppingIntoLibrarySymbolsLoadedFromSymbolServerRemoveSymbolServerAndStepAgain:
I run it all the night locally and couldn't reproduce it again, the test was executed 812 times.
|
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsTrying to fix the error on CI while running test SteppingIntoLibrarySymbolsLoadedFromSymbolServerRemoveSymbolServerAndStepAgain:
I run it all the night locally and couldn't reproduce it again, the test was executed 812 times.
|
/azp run runtime-wasm-non-libtests |
Azure Pipelines successfully started running 1 pipeline(s). |
public async Task WaitForWarningMessage(string message) | ||
{ | ||
object llock = new(); | ||
var tcs = new TaskCompletionSource(); |
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 this method take a CancellationToken, cancel this tcs
when the token is cancelled?
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'm not sure, I copied from the method that you wrote WaitForScriptParsedEventsAsync
.
If you think we should please let me know and probably we should change both.
[ConditionalFact(nameof(RunningOnChrome))] | ||
public async Task EvaluateMethodAndCheckIsNotPausingOnBreakpoint() | ||
{ | ||
var waitForScript = WaitForWarningMessage("console.warning: MONO_WASM: Adding an id (0) that already exists in commands_received"); |
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.
What does this message mean, and why are we waiting to see that we don't get it?
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.
This message means that we are paused in a breakpoint and we are receiving another pause.
With the fix we don't see the message anymore.
@@ -870,5 +870,23 @@ public async Task EvaluatePropertyThatThrows() | |||
props = await GetObjectOnFrame(frame, "this"); | |||
CheckNumber(props, "a", 11); | |||
}); | |||
|
|||
[ConditionalFact(nameof(RunningOnChrome))] | |||
public async Task EvaluateMethodAndCheckIsNotPausingOnBreakpoint() |
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.
We should also add tests which do this but with an actual breakpoint.
void Foo()
{
// bp here
}
void Method1()
{
Method2();
}
void Method2()
{
... // bp set here
}
- set bp in Foo, and Method2
- if we are originally stopped at bp in
Foo
- then we call
Method1
, we should no break again at Method2
- And the same thing with a property getter - explicitly invoked, bp in the getter or a method called by that getter
- And the same thing with a property getter - implicitly invoked because we were evaluating properties on some local var
/azp run runtime-wasm-non-libtests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-wasm-dbgtests |
Azure Pipelines failed to run 1 pipeline(s). |
/azp run runtime-wasm-dbgtests |
Azure Pipelines successfully started running 1 pipeline(s). |
…e into thays_fix_symbolServer_CI
/azp run runtime-wasm-dbgtests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-wasm-dbgtests |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-wasm-dbgtests |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
LGTM! And good job on tracking this one down with almost no information, or being able to reproduce it!
You helped a lot, thanks! |
This is trying to proactively fix a potential issue. It was prompted by this error on CI while running test
SteppingIntoLibrarySymbolsLoadedFromSymbolServerRemoveSymbolServerAndStepAgain
:I run it all the night locally and couldn't reproduce it again, the test was executed 812 times.
Looking at the proxy log,
console.warning: MONO_WASM: Adding an id (0) that already exists in commands_received"
message was seen it. And based on reading the code, my understanding is:
This is a case where a method is called, while we are already PAUSEd.
For example, if we are paused in a method
foo.run()
foo.method(){}
[1]foo.method()
foo.method
should not cause any failures, or cause a second pauseThe missing fix here is to set ..
invoke_data.flags = INVOKE_FLAG_DISABLE_BREAKPOINTS;
.. when invoking the method in the runtime.
A related issue is seen when we are stepping through the code (global
stepping), and call a method while paused. The debugger agent behaves as it
is stepping through code while running this method.
invoked in the runtime.
Since, we failed to reproduce the original issue, the test has to depend on
the original message seen. And test checks for the absence of this message.
As long as the test passes it means that the above case does not cause any
failures
--
foo.method()
being set beforefoo.run()
is called at all