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.

Sign up for GitHub

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

Jump to bottom

[wasm][debugger] Detect initial status of pause on exceptions. #54040

Merged
merged 9 commits into from
Jun 25, 2021
Merged

[wasm][debugger] Detect initial status of pause on exceptions. #54040

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
68080fd
Detect initial status of pause on exceptions.
thaystg Jun 11, 2021
de352a1
Changing what @radical suggested.
thaystg Jun 15, 2021
232322e
Changing more things.
thaystg Jun 15, 2021
cb1bc7f
Test case created.
thaystg Jun 17, 2021
6e4f0dd
Adding a test for Reload page with ALL set.
thaystg Jun 17, 2021
ae2a17c
Merge remote-tracking branch 'origin/main' into thays_fix_exception_i…
thaystg Jun 23, 2021
8a27dff
Fixing merge conflicts.
thaystg Jun 23, 2021
5900616
setting icordebug = false.
thaystg Jun 24, 2021
67b266f
Removing unrelated change.
thaystg Jun 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/mono/wasm/debugger/BrowserDebugProxy/DevToolsHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,9 @@ internal class ExecutionContext
public int Id { get; set; }
public object AuxData { get; set; }

public bool PauseOnUncaught { get; set; }
public bool PauseOnCaught { get; set; }

public List<Frame> CallStack { get; set; }

public string[] LoadedFiles { get; set; }
Expand Down
43 changes: 42 additions & 1 deletion src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ internal class MonoProxy : DevToolsProxy
private static HttpClient client = new HttpClient();
private HashSet<SessionId> sessions = new HashSet<SessionId>();
private Dictionary<SessionId, ExecutionContext> contexts = new Dictionary<SessionId, ExecutionContext>();
private const string sPauseOnUncaught = "pause_on_uncaught";
private const string sPauseOnCaught = "pause_on_caught";

public MonoProxy(ILoggerFactory loggerFactory, IList<string> urlSymbolServerList) : base(loggerFactory)
{
Expand Down Expand Up @@ -120,8 +122,40 @@ protected override async Task<bool> AcceptEvent(SessionId sessionId, string meth
return true;
}

case "Runtime.exceptionThrown":
{
if (!GetContext(sessionId).IsRuntimeReady)
Copy link
Member

Choose a reason for hiding this comment

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

might need to remove this check if you were trying the on attach version

Copy link
Member Author

Choose a reason for hiding this comment

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

Attach is the case when the page is already loaded and then we connect the debugger? I tested it and it's working.

Copy link
Member Author

Choose a reason for hiding this comment

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

I only tested debugging on Chrome, I didn't test with VS or VSCode.

{
string exceptionError = args?["exceptionDetails"]?["exception"]?["value"]?.Value<string>();
if (exceptionError == sPauseOnUncaught)
{
GetContext(sessionId).PauseOnUncaught = true;
return true;
}
if (exceptionError == sPauseOnCaught)
{
GetContext(sessionId).PauseOnCaught = true;
return true;
}
}
break;
}

case "Debugger.paused":
{
if (!GetContext(sessionId).IsRuntimeReady)
{
string reason = args?["reason"]?.Value<string>();
if (reason == "exception")
{
string exceptionError = args?["data"]?["value"]?.Value<string>();
if (exceptionError == sPauseOnUncaught || exceptionError == sPauseOnCaught)
{
await SendCommand(sessionId, "Debugger.resume", new JObject(), token);
return true;
}
}
}
//TODO figure out how to stich out more frames and, in particular what happens when real wasm is on the stack
string top_func = args?["callFrames"]?[0]?["functionName"]?.Value<string>();
switch (top_func) {
Expand Down Expand Up @@ -1078,6 +1112,11 @@ private async Task<DebugStore> RuntimeReady(SessionId sessionId, CancellationTok
Log("verbose", $"Failed to clear breakpoints due to {clear_result}");
}

if (context.PauseOnCaught && context.PauseOnUncaught)
await SendMonoCommand(sessionId, MonoCommands.SetPauseOnExceptions("all"), token);
else if (context.PauseOnUncaught)
await SendMonoCommand(sessionId, MonoCommands.SetPauseOnExceptions("uncaught"), token);

DebugStore store = await LoadStore(sessionId, token);

context.ready.SetResult(store);
Expand Down Expand Up @@ -1214,10 +1253,12 @@ private async Task AttachToTarget(SessionId sessionId, CancellationToken token)
// see https://github.com/mono/mono/issues/19549 for background
if (sessions.Add(sessionId))
{
string checkUncaughtExceptions = $"throw \"{sPauseOnUncaught}\";";
string checkCaughtExceptions = $"try {{throw \"{sPauseOnCaught}\";}} catch {{}}";
await SendMonoCommand(sessionId, new MonoCommands("globalThis.dotnetDebugger = true"), token);
Result res = await SendCommand(sessionId,
"Page.addScriptToEvaluateOnNewDocument",
JObject.FromObject(new { source = "globalThis.dotnetDebugger = true; delete navigator.constructor.prototype.webdriver" }),
JObject.FromObject(new { source = $"globalThis.dotnetDebugger = true; delete navigator.constructor.prototype.webdriver; {checkUncaughtExceptions} {checkCaughtExceptions}" }),
Copy link
Member

Choose a reason for hiding this comment

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

doing it here is only going to work on new documents, would we be out of sync in the attach case still? does it work if you add it to the SendMonoCommand part above?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tested two cases: The page is already loaded and then I connect debugger, it passes here and I can get the information that is expected.
The page is reloaded with the debugger already connected, it also passes here and I can get the information.
I'll test adding it to the SendMonoCommand.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not work if I add it to SendMonoCommand because it uses Runtime.Evaluate and Runtime.Evaluate does not call Debugger.Pause when an exception is threw.

Copy link
Member

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 understand the flow here.

  1. on attach, we throw two exceptions, so we can infer the pause-on-exceptions setting of the debugger;
  2. then I see two code paths:
    a. Runtime.exceptionThrown - where we do the inference, and set the value accordingly, in the proxy, and the app, and only when runtime is not ready
    b. Debugger.paused - where we resume, if it paused because of one of the above exceptions
  • so, what are the actual scenarios here?
  • And what does the flow of events look like, for them?

Also, it would be very useful to express that in tests too, but that might not be straight forward, so it can be in a follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we attach to an already loaded page it will only pass on Runtime.exceptionThrown and this would print a message on console if I don't return true.
If we refresh a page with the debugger attached it will pass on Debugger.paused, and we run the resume, and it will also pass on Runtime.exceptionThrown that would print a message if I don't return true.
In both cases before send the runtime ready we have already got the configuration so we can set it to mono debugger.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh no, I tested again, when the page is already loaded and we attach the debugger we receive the "Debugger.setPauseOnExceptions" with the correct information.
When we refresh the page that we are debugging, then we will receive the Debugger.paused and the Runtime.exceptionThrown.

Copy link
Member

Choose a reason for hiding this comment

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

If we attach to an already loaded page it will only pass on Runtime.exceptionThrown and this would print a message on console if I don't return true.

By "pass on foo", do you mean that foo's code path will be followed?

Question for scenario #1:

  1. attach to already loaded page
  2. we send a Page.addScriptToEvaluateOnNewDocument, with .. {checkUncaughtExceptions} {checkCaughtExceptions}
  3. and based on your last comment, we get Debugger.setPauseOnExceptions
  4. And then .. do we get Runtime.exceptionThrown? If yes, then for which case(caught/uncaught)?

If we refresh a page with the debugger attached it will pass on Debugger.paused, and we run the resume, and it will also pass on Runtime.exceptionThrown that would print a message if I don't return true.

  • Also, what happens for the {checkUncaughtExceptions} in the command that we send.

    • if the setting is uncaught, then debugger is paused, we get exceptionThrown, resume the debugger. Does the code then resume to the next line, and we execute checkCaughtExceptions`?

  • Is there any case where we might want/need to get the "current" setting from the app, and use that in the proxy?

Copy link
Member Author

@thaystg thaystg Jun 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I changed somethings in the PR.
For scenario #1:
We get Runtime.exceptionThrown in the case of the page already loaded for uncaught and we don't receive Debugger.Pause. Then we receive the Debugger.setPauseOnExceptions with the correct information.

For scenario #2:
If the setting is uncaught and we refresh the page we get:
Debugger.pause where we set the context status that will be send later to mono runtime. And the we receive a Runtime.exceptionThrown that will just return true to avoid printing the exception on the console.
If the setting is all and we refresh the page we get:
Debugger.pause where we set the context status PauseOnCaught to true then we resume, then we receive another Debugger.pause where we set the context status PauseOnUncaught to true then we resume. Then we receive Runtime.exceptionThrown for uncaught.
If while loading the page before the runtime is ready we change the status, we will receive a Debugger.setPauseOnExceptions with the right information, so I override the information that is in context for PauseOnCaught and PauseOnUncaught and when the runtime is ready I send the correct status.

We don't need the current setting from the app, the current setting should always be correct, because during the use of the debugger we receive the Debugger.setPauseOnExceptions with the correct information.

token);

if (sessionId != SessionId.Null && !res.IsOk)
Expand Down