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] Download Symbols from microsoft symbol server #40690

Merged

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Aug 12, 2020

Creating a draft to download symbols from microsoft symbol server when there is an exception and exceptions are turned on on debugger.

This is a workaround while VS doesn't work on it, which should be the final solution.

If you think that is in the right way, there is still a to do list:

  • Read url from config file, or receive as an startup parameter.
  • Create a flag to enable and disable this functionality, because this may cause some slowness when creating the callstack of the exception.

The callstack exception without this PR:
image

The callstack exception with this PR:
image

…n there is an exception and exceptions are turned on on debugger.

This is a workaround while VS doesn't work on it, which should be the final solution.
lewing
lewing previously requested changes Aug 13, 2020
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
@thaystg thaystg requested a review from lewing August 14, 2020 18:32
@thaystg
Copy link
Member Author

thaystg commented Aug 14, 2020

I'm not sure what is the best way to receive the symbol server url, I was thinking to receive in the same way that we receive browserUri, and pass to DevToolsProxy in run method. If anyone has any better idea, please let me know.

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

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

Only nitpicks, nothing to block merging. 👍

src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
@radical
Copy link
Member

radical commented Aug 15, 2020

I'm not sure what is the best way to receive the symbol server url, I was thinking to receive in the same way that we receive browserUri, and pass to DevToolsProxy in run method. If anyone has any better idea, please let me know.

What does the server url depend on? Does it change?

@thaystg
Copy link
Member Author

thaystg commented Aug 17, 2020

I'm not sure what is the best way to receive the symbol server url, I was thinking to receive in the same way that we receive browserUri, and pass to DevToolsProxy in run method. If anyone has any better idea, please let me know.

What does the server url depend on? Does it change?

On VsWindows for example, the user can choose a list of symbol server URL. From Microsoft I think that this is the only one.

image

@thaystg thaystg marked this pull request as ready for review August 17, 2020 14:09
@radical
Copy link
Member

radical commented Aug 24, 2020

re:url, you could send a custom message to the proxy to pass the url, and handle that in MonoProxy, similar to https://github.com/dotnet/runtime/blob/master/src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs#L340-L341 . We send that one in tests - https://github.com/dotnet/runtime/blob/master/src/mono/wasm/debugger/DebuggerTestSuite/Support.cs#L878 .

How will you get this url from VS though, to pass on to the proxy?

@thaystg
Copy link
Member Author

thaystg commented Aug 24, 2020

The custom message that you suggested is the same that I think we should do.
I have no idea how to get this url from VS, but if we create the custom message, VS team can call this message passing to Proxy the urls.

Co-authored-by: Larry Ewing <lewing@microsoft.com>
@thaystg
Copy link
Member Author

thaystg commented Aug 26, 2020

@lewing , I'm discussing with Diego about how they will pass this info for DebuProxy.

…request as discussed with Diego:

Diego: The response from vs code is that this shouldn't live in js-debug, so I think that passing it around in inspectUri would be the best approach for us
@thaystg
Copy link
Member Author

thaystg commented Sep 4, 2020

@lewing @radical now this is ready to be merged.
Diego asked to pass the urlSymbolServer as a parameter, like this they don't need to change js-debug. In any case, I let the protocol extension if we need this later and accept the parameter too.

@radical
Copy link
Member

radical commented Sep 14, 2020

Is there anything blocking this now?

@thaystg
Copy link
Member Author

thaystg commented Sep 14, 2020

I don't think so, I just removed the default microsoft symbol server URL, then we can try to merge it on rc 2.
@lewing can we merge now?

…il. When it's merged we will not get any side effect with this workaroud.
@thaystg
Copy link
Member Author

thaystg commented Sep 15, 2020

@lewing I removed the default URL of MS Symbol Server, so now, this will not affect the user unless he changes the LaunchSettings.json to use this InspectUri: "inspectUri": "{wsProtocol}://{url.hostname}:{url.port}/_framework/debug/ws-proxy?browser={browserInspectUri}?urlSymbolServer=http://msdl.microsoft.com/download/symbols"
Then the new implementation will be effective.

@thaystg thaystg merged commit eb6bcde into dotnet:master Sep 15, 2020
@thaystg
Copy link
Member Author

thaystg commented Sep 15, 2020

/backport to release/5.0-rc2

@github-actions
Copy link
Contributor

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/256085151

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants