Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

Eval'ing a script with sourceURL set to the name of a loaded script will cause breakpoints to be set incorrectly for that script #56

Open
roblourens opened this issue Jul 6, 2016 · 8 comments
Labels

Comments

@roblourens
Copy link
Member

I handle scripts with no URL but also need to handle scripts with the same URL. System.js was doing this for some reason.

@roblourens roblourens added the bug label Jul 6, 2016
@lijunle
Copy link
Member

lijunle commented Jul 7, 2016

I made a deep debugging on the system.js. on the instantiate step of an AMD module, it needs to eval the code.

The call stack is:

  1. amd.js#L22
  2. global-eval.js#L77

What will happens if the eval happens for the chrome debugger?

Maybe related to #36


As a workaround, we can provide the absolute URL to SystemJS eval function.

@lijunle
Copy link
Member

lijunle commented Jul 10, 2016

@roblourens I saw you are using setBreakpoint method for eval case. Why not always use setBreakpoint method for all the cases? The scriptId can be got from scriptParsed method.

@roblourens
Copy link
Member Author

https://github.com/Microsoft/vscode-chrome-debug-core/blob/master/src/chrome/chromeDebugAdapter.ts#L382

Using setBreakpointByURL, Chrome will automatically rebind it when refreshing the page, and if it's in global code, it will be hit. Otherwise if I rebind it by ID manually after a refresh, we'll miss the global code.

@lijunle
Copy link
Member

lijunle commented Jul 10, 2016

I see. In such case, could we change the logic about ChromeUtils.targetUrlToClientPath which is used in PathTransformer.scriptParsed. If the URL starts from http:// or https://, update this._clientPathToTargetUrl and this._targetUrlToClientPath, otherwise, treat them as eval cases, to update another two more maps - clientPathToTargetId and targetIdToClientPath?

@roblourens
Copy link
Member Author

roblourens commented Jul 10, 2016

If I remember correctly, in your case, the script that's eval'd is not the script that's on disk, right? It's a wrapper or something. Most eval scripts can't be associated with a client path. It shouldn't be necessary as long as the sourcemaps can be resolved. Why do you want those maps?

@lijunle
Copy link
Member

lijunle commented Jul 10, 2016

There are two situations here:

  1. The eval script is auto-generated and not map to any file on the disk. If we want to support this, looks like to create a temporary file on the disk. This looks a big scenario to me. I am leaving it to you.
  2. Load AMD module with SystemJS, this is more specified. The SystemJS will fetch and then manually eval the script. It leads twice scriptParsed events - the first on fetch happens, the second on eval happens. On the first scriptParsed event, the full HTTP URL is sent to debugger, on the second event, a relative URL is sent to debugger. The second scriptParsed event overrides the this._clientPathToTargetUrl mapping and makes the following up setBP cannot be verified.

So, I am thinking about, can we do something to resolve the second case?

@roblourens
Copy link
Member Author

  1. This is actually a common scenario, it's already supported. No need to put a file on disk, it just uses the URL instead of a path.
  2. A fetch by itself shouldn't trigger a scriptParsed event, it would have to actually load the script by adding it to the DOM or something. What I remember seeing is that the first one was the real one, and the second was just a few lines that were eval'd. But I do need to distinguish between the two scripts, which we have the info to do.

@lijunle
Copy link
Member

lijunle commented Jul 10, 2016

The second case triggers the scriptParsed because scriptLoad=true, it uses <script> tag to load the script.

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

No branches or pull requests

2 participants