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

Can't use "open loaded scripts" to load a URL with query params #23194

Closed
roblourens opened this issue Mar 25, 2017 · 13 comments
Closed

Can't use "open loaded scripts" to load a URL with query params #23194

roblourens opened this issue Mar 25, 2017 · 13 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues verification-needed Verification of issue is requested verified Verification succeeded

Comments

@roblourens
Copy link
Member

Chrome-debug supports .js files with query parameters (http://localhost/foo.js?bar). But if I load one with "Open loaded scripts", the query parameter is stripped. This happens here: https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/debug/browser/debugContentProvider.ts#L40 since fsPath doesn't include the query parameter. Any idea how to make this work? I don't think URI is expecting to have a URL with another scheme.

@isidorn
Copy link
Contributor

isidorn commented Mar 27, 2017

That normalize code should only be executed if the source could not be found - otherwise source should not be null and the rawSource should be passed to the adapter.
Do you know why is that?
As for the normalize(uri.fsPath) call - I am not married to it and we can change it to whatever uri massaging techinique would work for you and not break the existing behavior.
If we change this I would prefer to be in debt week -> april

@isidorn isidorn added the debug Debug viewlet, configurations, breakpoints, adapter issues label Mar 27, 2017
@isidorn isidorn added this to the April 2017 milestone Mar 27, 2017
@roblourens roblourens self-assigned this Mar 27, 2017
@roblourens
Copy link
Member Author

roblourens commented Apr 5, 2017

It tries to get the source from process.sources, but it looks like that is only populated when it breaks in there, right? Plus it's a different format, the script from the quickopen starts with debug: but the ones in process.sources don't.

@roblourens
Copy link
Member Author

I don't know about replacing uri.fsPath, resource.toString(true).substr('debug:'.length) would do it if that isn't too hacky.

@isidorn
Copy link
Contributor

isidorn commented Apr 5, 2017

Yes, process.sources is only populated once there is a call stack that contains that source - so if it breaks.
Yeah that feels like a total hack. Why don't you experiment with different solutions and create a PR where we can comment and try out different approaches if needed?

roblourens added a commit that referenced this issue Apr 5, 2017
@roblourens roblourens reopened this Sep 1, 2017
@roblourens
Copy link
Member Author

This regressed from ce177da adding back the query: '' in DebugContentProvider.getTextContent. @weinand is that needed?

@roblourens
Copy link
Member Author

Also, #33712

@roblourens roblourens modified the milestones: September 2017, April 2017 Sep 4, 2017
@isidorn isidorn assigned weinand and unassigned roblourens and isidorn Sep 4, 2017
@weinand weinand added the bug Issue identified by VS Code Team member as probable bug label Sep 5, 2017
@hbenl
Copy link

hbenl commented Sep 11, 2017

I think opening the file by putting its url into a debug: uri is very fragile, to say the least. I have listed some problems I've noticed while implementing a Loaded Scripts Explorer for the Firefox debug adapter in #32845.
My suggestion would be to add the ability to open files using the numerical sourceReference that the debug adapter assigned to them or using the Source object that is used for identifying the file in the messages between VS Code and the DA.
Whatever solution you adopt, keep in mind that VS Code should be able to determine that the file opened by clicking on a stackframe and the file opened by clicking in the Loaded Scripts Explorer are one and the same - otherwise setting breakpoints in a file opened through the Loaded Scripts Explorer will become problematic.

@weinand
Copy link
Contributor

weinand commented Sep 16, 2017

I've addressed this issue by making sure that the flow Source -> uri -> Source preserves the data so that the initial and resulting Source objects are identical. The "path" attribute of the source object can contain anything, e.g. a file system path or a full URL.

So if a Uri is constructed in the following way:

const sessionId: string = ...
const source: Source = ...

const uri = vscode.Uri.parse(`debug:${encodeURIComponent(source.path)}?session=${encodeURIComponent(sessionId)}&ref=${source.sourceReference}`);

the debugContentProvider inside VS Code will retrieve the content by sending a "sourceRequest" with a "Source" argument to the debug adapter identified by the debug session.

@weinand
Copy link
Contributor

weinand commented Sep 25, 2017

Verify that Rob's Chrome-debug case from above works.

@roblourens
Copy link
Member Author

It is escaping the query parameter, but works anyway using the new loadedSources API, because then we can rely on sourceReference instead of a string path.

To client: {"seq":0,"type":"event","event":"loadedSource","body":{"reason":"new","source":{"name":"test1.js?blah","path":"http://localhost:8080/out/client%20with%20space/test1.js?blah","sourceReference":1000}}}

From client: source({"sourceReference":1000,"source":{"path":"http://localhost:8080/out/client%20with%20space/test1.js%3Fblah","sourceReference":1000}})

@hbenl
Copy link

hbenl commented Sep 26, 2017

It is escaping the query parameter, but works anyway

I have also seen this (with the Firefox debug adapter) and couldn't find any user-visibile ill effects. In particular, the source url is displayed correctly in the tab opened from the Loaded Scripts Explorer (i.e. the query parameter is unescaped there and other escaped characters in the url remain escaped).

@weinand
Copy link
Contributor

weinand commented Sep 26, 2017

Added "Verified" label since both comments from above confirm that this works correctly now.

@weinand weinand added the verified Verification succeeded label Sep 26, 2017
@roblourens
Copy link
Member Author

It does rely on me updating chrome-debug, in case anybody else tries this. I'll do that this week.

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

4 participants