Fix some sourcemap path handling

microsoft · Mar 21, 2016 · 9175a28 · 9175a28
1 parent 326dd27
commit 9175a28
Show file tree

Hide file tree

Showing 10 changed files with 81 additions and 70 deletions.
diff --git a/.vscode/launch.json b/.vscode/launch.json
@@ -4,21 +4,21 @@
         {
             "name": "launch as server",
             "type": "node",
-            "program": "./out/webkit/webKitDebug.js",
+            "program": "${workspaceRoot}/out/webkit/webKitDebug.js",
 			"runtimeArgs": ["--harmony"],
             "stopOnEntry": false,
             "args": [ "--server=4712" ],
             "sourceMaps": true,
-            "outDir": "out"
+            "outDir": "${workspaceRoot}/out"
         },
         {
             "name": "test",
             "type": "node",
-            "program": "./node_modules/gulp/bin/gulp.js",
+            "program": "${workspaceRoot}/node_modules/gulp/bin/gulp.js",
             "stopOnEntry": false,
             "args": [ "test" ],
             "sourceMaps": true,
-            "outDir": "out"
+            "outDir": "${workspaceRoot}/out"
         }
     ]
 }
diff --git a/README.md b/README.md
@@ -103,7 +103,7 @@ When your launch config is set up, you can debug your project! Pick a launch con
 General things to try if you're having issues:
 * Ensure `webRoot` is set correctly if needed
 * If sourcemaps are enabled, try setting `sourceRoot` to be a file URL. `sourceRoot` is a property in the .map file which is usually specified in your project's build config.
-* Close other running instances of Chrome - if Chrome is already running, the extension may not be able to attach, when using launch mode. Chrome can even stay running in the background when all its windows are closed, which will interfere.
+* Close other running instances of Chrome - if Chrome is already running, the extension may not be able to attach, when using launch mode. Chrome can even stay running in the background when all its windows are closed, which will interfere - check the taskbar or kill the process if necessary.
 * Ensure nothing else is using port 9222, or specify a different port in your launch config
 * Check the console for warnings that this extension prints in some cases when it can't attach
 * Ensure the code in Chrome matches the code in Code. Chrome may cache an old version.

diff --git a/adapter/sourceMaps/sourceMapTransformer.ts b/adapter/sourceMaps/sourceMapTransformer.ts
@@ -6,6 +6,7 @@ import * as path from 'path';
 
 import {ISourceMaps, SourceMaps} from './sourceMaps';
 import * as utils from '../../webkit/utilities';
+import {Logger} from '../../webkit/utilities';
 
 interface IPendingBreakpoint {
     resolve: () => void;
@@ -58,7 +59,7 @@ export class SourceMapTransformer implements IDebugTransformer {
                 const argsPath = args.source.path;
                 const mappedPath = this._sourceMaps.MapPathFromSource(argsPath);
                 if (mappedPath) {
-                    utils.Logger.log(`SourceMaps.setBP: Mapped ${argsPath} to ${mappedPath}`);
+                    Logger.log(`SourceMaps.setBP: Mapped ${argsPath} to ${mappedPath}`);
                     args.authoredPath = argsPath;
                     args.source.path = mappedPath;
 
@@ -67,11 +68,11 @@ export class SourceMapTransformer implements IDebugTransformer {
                     const mappedLines = args.lines.map((line, i) => {
                         const mapped = this._sourceMaps.MapFromSource(argsPath, line, /*column=*/0);
                         if (mapped) {
-                            utils.Logger.log(`SourceMaps.setBP: Mapped ${argsPath}:${line}:0 to ${mappedPath}:${mapped.line}:${mapped.column}`);
+                            Logger.log(`SourceMaps.setBP: Mapped ${argsPath}:${line}:0 to ${mappedPath}:${mapped.line}:${mapped.column}`);
                             mappedCols[i] = mapped.column;
                             return mapped.line;
                         } else {
-                            utils.Logger.log(`SourceMaps.setBP: Mapped ${argsPath} but not line ${line}, column 0`);
+                            Logger.log(`SourceMaps.setBP: Mapped ${argsPath} but not line ${line}, column 0`);
                             mappedCols[i] = 0;
                             return line;
                         }
@@ -99,10 +100,10 @@ export class SourceMapTransformer implements IDebugTransformer {
                     });
                 } else if (this._allRuntimeScriptPaths.has(argsPath)) {
                     // It's a generated file which is loaded
-                    utils.Logger.log(`SourceMaps.setBP: SourceMaps are enabled but ${argsPath} is a runtime script`);
+                    Logger.log(`SourceMaps.setBP: SourceMaps are enabled but ${argsPath} is a runtime script`);
                 } else {
                     // Source (or generated) file which is not loaded, need to wait
-                    utils.Logger.log(`SourceMaps.setBP: ${argsPath} can't be resolved to a loaded script.`);
+                    Logger.log(`SourceMaps.setBP: ${argsPath} can't be resolved to a loaded script.`);
                     this._pendingBreakpointsByPath.set(argsPath, { resolve, reject, args, requestSeq });
                     return;
                 }
@@ -130,10 +131,10 @@ export class SourceMapTransformer implements IDebugTransformer {
                     response.breakpoints.forEach(bp => {
                         const mapped = this._sourceMaps.MapToSource(args.source.path, bp.line, bp.column);
                         if (mapped) {
-                            utils.Logger.log(`SourceMaps.setBP: Mapped ${args.source.path}:${bp.line}:${bp.column} to ${mapped.path}:${mapped.line}`);
+                            Logger.log(`SourceMaps.setBP: Mapped ${args.source.path}:${bp.line}:${bp.column} to ${mapped.path}:${mapped.line}`);
                             bp.line = mapped.line;
                         } else {
-                            utils.Logger.log(`SourceMaps.setBP: Can't map ${args.source.path}:${bp.line}:${bp.column}, keeping the line number as-is.`);
+                            Logger.log(`SourceMaps.setBP: Can't map ${args.source.path}:${bp.line}:${bp.column}, keeping the line number as-is.`);
                         }
 
                         this._requestSeqToSetBreakpointsArgs.delete(requestSeq);
@@ -195,7 +196,7 @@ export class SourceMapTransformer implements IDebugTransformer {
             this._sourceMaps.ProcessNewSourceMap(event.body.scriptUrl, event.body.sourceMapURL).then(() => {
                 const sources = this._sourceMaps.AllMappedSources(event.body.scriptUrl);
                 if (sources) {
-                    utils.Logger.log(`SourceMaps.scriptParsed: ${event.body.scriptUrl} was just loaded and has mapped sources: ${JSON.stringify(sources) }`);
+                    Logger.log(`SourceMaps.scriptParsed: ${event.body.scriptUrl} was just loaded and has mapped sources: ${JSON.stringify(sources) }`);
                     sources.forEach(sourcePath => {
                         this.resolvePendingBreakpointsForScript(sourcePath);
                     });
@@ -208,9 +209,9 @@ export class SourceMapTransformer implements IDebugTransformer {
      * Resolve any pending breakpoints for this script
      */
     private resolvePendingBreakpointsForScript(scriptUrl: string): void {
-        utils.Logger.log(`SourceMaps.scriptParsed: Resolving pending breakpoints for ${scriptUrl}`);
-
         if (this._pendingBreakpointsByPath.has(scriptUrl)) {
+            Logger.log(`SourceMaps.scriptParsed: Resolving pending breakpoints for ${scriptUrl}`);
+
             let pendingBreakpoints = this._pendingBreakpointsByPath.get(scriptUrl);
             this._pendingBreakpointsByPath.delete(scriptUrl);
 

diff --git a/adapter/sourceMaps/sourceMaps.ts b/adapter/sourceMaps/sourceMaps.ts
@@ -241,11 +241,8 @@ enum Bias {
 
 class SourceMap {
 	private _generatedPath: string;		// the generated file for this sourcemap
-	private _sources: string[];			// the sources of generated file (relative to sourceRoot)
-	private _absSourceRoot: string;		// the common prefix for the source (can be a URL)
+	private _sources: string[];			// list of authored files (absolute paths)
 	private _smc: SourceMapConsumer;	// the source map
-    private _webRoot: string;           // if the sourceRoot starts with /, it's resolved from this absolute path
-    private _sourcesAreURLs: boolean;   // if sources are specified with file:///
 
     /**
      * pathToGenerated - an absolute local path or a URL
@@ -255,40 +252,39 @@ class SourceMap {
 	public constructor(generatedPath: string, json: string, webRoot: string) {
         Logger.log(`SourceMap: creating SM for ${generatedPath}`)
 		this._generatedPath = generatedPath;
-        this._webRoot = webRoot;
 
 		const sm = JSON.parse(json);
-		this._absSourceRoot = PathUtils.getAbsSourceRoot(sm.sourceRoot, this._webRoot, this._generatedPath);
+		const absSourceRoot = PathUtils.getAbsSourceRoot(sm.sourceRoot, webRoot, this._generatedPath);
 
         // Overwrite the sourcemap's sourceRoot with the version that's resolved to an absolute path,
         // so the work above only has to be done once
-        sm.sourceRoot = utils.pathToFileURL(this._absSourceRoot);
+        sm.sourceRoot = null; // probably get rid of this._sourceRoot?
 
-        sm.sources = sm.sources.map((sourcePath: string) => {
-            // special-case webpack:/// prefixed sources which is kind of meaningless
+        // sm.sources are relative paths or file:/// urls - (or other URLs?) read the spec...
+        // resolve them to file:/// urls, using absSourceRoot.
+        // note - the source-map library doesn't like backslashes, but some tools output them.
+        // Which is wrong? Consider filing issues on source-map or tools that output backslashes?
+        // In either case, support whatever works
+        this._sources = sm.sources.map((sourcePath: string) => {
+            // Special-case webpack:/// prefixed sources which is kind of meaningless
             sourcePath = utils.lstrip(sourcePath, 'webpack:///');
+            sourcePath = utils.canonicalizeUrl(sourcePath);
 
-            // Force correct format for sanity
-            return utils.fixDriveLetterAndSlashes(sourcePath);
-        });
-
-        this._smc = new SourceMapConsumer(sm);
-
-        // rewrite sources as absolute paths
-        this._sources = sm.sources.map((sourcePath: string) => {
-            if (sourcePath.startsWith('file:///')) {
-                // If one source is a URL, assume all are
-                this._sourcesAreURLs = true;
+            // If not already an absolute path, make it an absolute path with this._absSourceRoot. Also resolves '..' parts.
+            if (!Path.isAbsolute(sourcePath)) {
+                sourcePath = Path.resolve(absSourceRoot, sourcePath);
             }
 
-            sourcePath = utils.lstrip(sourcePath, 'webpack:///');
-            sourcePath = PathUtils.canonicalizeUrl(sourcePath);
-            if (Path.isAbsolute(sourcePath)) {
-                return utils.fixDriveLetterAndSlashes(sourcePath);
-            } else {
-                return Path.join(this._absSourceRoot, sourcePath);
-            }
+            return sourcePath;
+        });
+
+        // Rewrite sm.sources to same as this._sources but forward slashes and file url
+        sm.sources = this._sources.map(sourceAbsPath => {
+            // Convert to file: url. After this, it's a file URL for an absolute path to a file on disk with forward slashes.
+            return utils.pathToFileURL(sourceAbsPath);
         });
+
+        this._smc = new SourceMapConsumer(sm);
 	}
 
     /*
@@ -316,7 +312,6 @@ class SourceMap {
 	 * finds the nearest source location for the given location in the generated file.
 	 */
 	public originalPositionFor(line: number, column: number, bias: Bias = Bias.GREATEST_LOWER_BOUND): SourceMap.MappedPosition {
-
 		const mp = this._smc.originalPositionFor(<any>{
 			line: line,
 			column: column,
@@ -334,25 +329,14 @@ class SourceMap {
 	 * finds the nearest location in the generated file for the given source location.
 	 */
 	public generatedPositionFor(src: string, line: number, column: number, bias = Bias.GREATEST_LOWER_BOUND): SourceMap.Position {
-        if (this._sourcesAreURLs) {
-            src = utils.pathToFileURL(src);
-        } else if (this._absSourceRoot) {
-            // make input path relative to sourceRoot
-			src = Path.relative(this._absSourceRoot, src);
-
-            // source-maps use forward slashes unless the source is specified with file:///
-            if (process.platform === 'win32') {
-                src = src.replace(/\\/g, '/');
-            }
-		}
+        src = utils.pathToFileURL(src);
 
 		const needle = {
 			source: src,
 			line: line,
 			column: column,
 			bias: bias
 		};
-
 		return this._smc.generatedPositionFor(needle);
 	}
 }
diff --git a/test/webkit/utilities.test.ts b/test/webkit/utilities.test.ts
@@ -435,11 +435,15 @@ suite('Utilities', () => {
 
     suite('pathToFileURL', () => {
         test('converts windows-style paths', () => {
-            assert.equal(getUtilities().pathToFileURL('c:/code/app.js'), 'file:///c:/code/app.js');
+            assert.equal(getUtilities().pathToFileURL('c:\\code\\app.js'), 'file:///c:/code/app.js');
         });
 
         test('converts unix-style paths', () => {
             assert.equal(getUtilities().pathToFileURL('/code/app.js'), 'file:///code/app.js');
         });
+
+        test('encodes as URI', () => {
+            assert.equal(getUtilities().pathToFileURL('c:\\path with spaces'), 'file:///c:/path%20with%20spaces');
+        });
     });
 });
diff --git a/testapp/.vscode/launch.json b/testapp/.vscode/launch.json
@@ -1,6 +1,6 @@
 {
     "version": "0.1.0",
-    // "debugServer": "4712",
+    "debugServer": "4712",
     "configurations": [
         {
             "name": "test chrome",
@@ -9,16 +9,16 @@
             "url": "http://localhost:8080/index.html",
             "sourceMaps": true,
             "diagnosticLogging": true,
-            "webRoot": "wwwroot"
+            "webRoot": "${workspaceRoot}/wwwroot"
         },
         {
             "name": "launch for file",
             "type": "chrome",
             "request": "launch",
-            "file": "wwwroot/index.html",
+            "file": "${workspaceRoot}/wwwroot/index.html",
             "sourceMaps": true,
             "diagnosticLogging": true,
-            "webRoot": "wwwroot/out/client with space"
+            "webRoot": "${workspaceRoot}/wwwroot/out/client with space"
         },
         {
             "name": "attach to chrome",
@@ -27,7 +27,7 @@
             "request": "attach",
             "sourceMaps": true,
             "diagnosticLogging": true,
-            "webRoot": "./wwwroot"
+            "webRoot": "${workspaceRoot}/wwwroot"
         }
     ]
 }
diff --git a/testapp/gulpfile.js b/testapp/gulpfile.js
@@ -14,7 +14,7 @@ var filter = require('gulp-filter');
 
 var sources = [
     'wwwroot/client with space'
-].map(function (tsFolder) { return tsFolder + '/**/*.ts'; });
+].map(function (tsFolder) { return tsFolder + '\\**\\*.ts'; });
 
 var projectConfig = {
     target: 'ES6',
@@ -34,7 +34,7 @@ gulp.task('build', function () {
         .pipe(gulp.dest('./wwwroot/out'));
 });
 
-gulp.task('serve', ['build'], function (done) {
+function serve(done) {
     browserSync({
         online: false,
         open: false,
@@ -43,7 +43,10 @@ gulp.task('serve', ['build'], function (done) {
             baseDir: ['./wwwroot']
         }
     }, done);
-});
+}
+
+gulp.task('serve', serve);
+gulp.task('buildAndServe', ['build'], serve);
 
 gulp.task('bs-reload', ['build'], function() {
     browserSync.reload();

diff --git a/webkit/utilities.ts b/webkit/utilities.ts
@@ -245,7 +245,7 @@ export function webkitUrlToClientPath(webRoot: string, aUrl: string): string {
  * The client can handle urls in this format too.
  * file:///D:\\scripts\\code.js => d:/scripts/code.js
  * file:///Users/me/project/code.js => /Users/me/project/code.js
- * c:\\scripts\\code.js => c:/scripts/code.js
+ * c:/scripts/code.js => c:\\scripts\\code.js
  * http://site.com/scripts/code.js => (no change)
  * http://site.com/ => http://site.com
  */
@@ -263,6 +263,14 @@ export function canonicalizeUrl(aUrl: string): string {
     return aUrl;
 }
 
+/**
+ * Replace any backslashes with forward slashes
+ * blah\something => blah/something
+ */
+export function forceForwardSlashes(aUrl: string): string {
+    return aUrl.replace(/\\/g, '/');
+}
+
 /**
  * Ensure lower case drive letter and \ on Windows
  */
@@ -413,7 +421,9 @@ export function lstrip(s: string, lStr: string): string {
  * C:/code/app.js => file:///C:/code/app.js
  * /code/app.js => file:///code/app.js
  */
-export function pathToFileURL(path: string): string {
-    return (path.startsWith('/') ? 'file://' : 'file:///') +
-        path;
+export function pathToFileURL(absPath: string): string {
+    absPath = forceForwardSlashes(absPath);
+    absPath = (absPath.startsWith('/') ? 'file://' : 'file:///') +
+        absPath;
+    return encodeURI(absPath);
 }
diff --git a/webkit/webKitConnection.ts b/webkit/webKitConnection.ts
@@ -53,8 +53,13 @@ class ResReqWebSocket extends EventEmitter {
                 resolve(ws);
             });
             ws.on('message', msgStr => {
-                Logger.log('From target: ' + msgStr);
-                this.onMessage(JSON.parse(msgStr));
+                const msgObj = JSON.parse(msgStr);
+                if (msgObj && !(msgObj.method === "Debugger.scriptParsed" && msgObj.params && msgObj.params.isContentScript) && !(msgObj.params && msgObj.params.url && msgObj.params.url.indexOf('extensions::') === 0)) {
+                    // Not really the right place to examine the content of the message, but don't log annoying extension script notifications.
+                    Logger.log('From target: ' + msgStr);
+                }
+
+                this.onMessage(msgObj);
             });
             ws.on('close', () => {
                 Logger.log('Websocket closed');

diff --git a/webkit/webKitDebugAdapter.ts b/webkit/webKitDebugAdapter.ts
@@ -320,6 +320,10 @@ export class WebKitDebugAdapter implements IDebugAdapter {
         }
     }
 
+    public setFunctionBreakpoints(): Promise<any> {
+        return Promise.resolve();
+    }
+
     private _clearAllBreakpoints(url: string): Promise<void> {
         if (!this._committedBreakpointsByUrl.has(url)) {
             return Promise.resolve<void>();