Merge pull request #274 from digeff/block_while_parsing_scripts

Block while parsing scripts to prevent sending 'removes' before 'adds'

package.json

@@ -47,7 +47,7 @@
     "tslint-eslint-rules": "^1.5.0",
     "tslint-microsoft-contrib": "^2.0.10",
     "typemoq": "^0.3.3",
-    "typescript": "^2.4.1",
+    "typescript": "2.6.2",
     "vscode-nls-dev": "^2.1.6"
   },
   "scripts": {

src/chrome/chromeDebugAdapter.ts

@@ -136,6 +136,10 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
 
     private _breakOnLoadHelper: BreakOnLoadHelper | null;
 
+    // Queue to synchronize new source loaded and source removed events so that 'remove' script events
+    // won't be send before the corresponding 'new' event has been sent
+    private _sourceLoadedQueue: Promise<void> = Promise.resolve(null);
+
     public constructor({ chromeConnection, lineColTransformer, sourceMapTransformer, pathTransformer, targetFilter, enableSourceMapCaching }: IChromeDebugAdapterOpts, session: ChromeDebugSession) {
         telemetry.setupEventHandler(e => session.sendEvent(e));
         this._session = session;
@@ -449,32 +453,37 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
      * This event tells the client to begin sending setBP requests, etc. Some consumers need to override this
      * to send it at a later time of their choosing.
      */
-    protected sendInitializedEvent(): void {
+    protected async sendInitializedEvent(): Promise<void> {
         // Wait to finish loading sourcemaps from the initial scriptParsed events
         if (this._initialSourceMapsP) {
             const initialSourceMapsP = this._initialSourceMapsP;
             this._initialSourceMapsP = null;
 
-            initialSourceMapsP.then(() => {
-                this._session.sendEvent(new InitializedEvent());
-                this._earlyScripts.forEach(script => this.sendLoadedSourceEvent(script));
-                this._earlyScripts = null;
-            });
+            await initialSourceMapsP;
+
+            this._session.sendEvent(new InitializedEvent());
+            await Promise.all(this._earlyScripts.map(script => this.sendLoadedSourceEvent(script)));
+            this._earlyScripts = null;
         }
     }
 
+    public doAfterProcessingSourceEvents(action: () => void): Promise<void> {
+        return this._sourceLoadedQueue = this._sourceLoadedQueue.then(action);
+    }
+
     /**
      * e.g. the target navigated
      */
-    private onExecutionContextsCleared(): void {
-        const asyncOperations = [];
-        this._scriptsById.forEach(scriptedParseEvent => {
-            asyncOperations.push(this.scriptToLoadedSourceEvent('removed', scriptedParseEvent).then(scriptEvent =>
-                this._session.sendEvent(scriptEvent)));
-        });
+    private onExecutionContextsCleared(): Promise<void> {
+        const cachedScriptParsedEvents = Array.from(this._scriptsById.values());
+        return this.doAfterProcessingSourceEvents(async () => { // This will not execute until all the on-flight 'new' source events have been processed
+            for (let scriptedParseEvent of cachedScriptParsedEvents) {
+                const scriptEvent = await this.scriptToLoadedSourceEvent('removed', scriptedParseEvent);
+                this._session.sendEvent(scriptEvent);
+            }
 
-        Promise.all(asyncOperations).then(() =>
-            this.clearTargetContext());
+            this.clearTargetContext();
+        });
     }
 
     protected async onPaused(notification: Crdp.Debugger.PausedEvent, expectingStopReason = this._expectingStopReason): Promise<void> {
@@ -639,11 +648,19 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
     }
 
     protected async onScriptParsed(script: Crdp.Debugger.ScriptParsedEvent): Promise<void> {
-        if (typeof this._columnBreakpointsEnabled === 'undefined') {
-            this.detectColumnBreakpointSupport(script.scriptId).then(() => {
-                this.sendInitializedEvent();
-            });
-        }
+        this.doAfterProcessingSourceEvents(async () => { // This will block future 'removed' source events, until this processing has been completed
+            if (typeof this._columnBreakpointsEnabled === 'undefined') {
+                await this.detectColumnBreakpointSupport(script.scriptId).then(async () => {
+                    await this.sendInitializedEvent();
+                });
+            }
+
+            if (this._earlyScripts) {
+                this._earlyScripts.push(script);
+            } else {
+                await this.sendLoadedSourceEvent(script);
+            }
+        });
 
         if (script.url) {
             script.url = utils.fixDriveLetter(script.url);
@@ -700,12 +717,6 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
         if (this._initialSourceMapsP) {
             this._initialSourceMapsP = <Promise<any>>Promise.all([this._initialSourceMapsP, sourceMapsP]);
         }
-
-        if (this._earlyScripts) {
-            this._earlyScripts.push(script);
-        } else {
-            this.sendLoadedSourceEvent(script);
-        }
     }
 
     protected async sendLoadedSourceEvent(script: Crdp.Debugger.ScriptParsedEvent, loadedSourceEventReason: LoadedSourceEventReason = 'new'): Promise<void> {

test/chrome/chromeDebugAdapter.test.ts

@@ -25,6 +25,7 @@ import * as utils from '../../src/utils';
 
 /** Not mocked - use for type only */
 import {ChromeDebugAdapter as _ChromeDebugAdapter, LoadedSourceEventReason} from '../../src/chrome/chromeDebugAdapter';
+import { InitializedEvent, LoadedSourceEvent, Source } from 'vscode-debugadapter/lib/debugSession';
 
 const MODULE_UNDER_TEST = '../../src/chrome/chromeDebugAdapter';
 suite('ChromeDebugAdapter', () => {
@@ -404,6 +405,8 @@ suite('ChromeDebugAdapter', () => {
         const FILE_NAME = 'file:///a.js';
         const SCRIPT_ID = '1';
         function emitScriptParsed(url = FILE_NAME, scriptId = SCRIPT_ID, otherArgs: any = {}): void {
+            mockSourceMapTransformer.setup(m => m.scriptParsed(It.isValue(undefined), It.isValue(undefined)))
+                .returns(() => Promise.resolve([]));
             otherArgs.url = url;
             otherArgs.scriptId = scriptId;
 
@@ -450,6 +453,38 @@ suite('ChromeDebugAdapter', () => {
             });
         });
 
+        function createSource(name: string, path?: string, sourceReference?: number, origin?: string): Source {
+            return <Source>{
+                name: name,
+                path: path,
+                // if the path exists, do not send the sourceReference
+                sourceReference: sourceReference,
+                origin
+            };
+        }
+
+        test('When a page refreshes, finish sending the "new" source events, before sending the corresponding "removed" source event', async () => {
+            const expectedEvents: DebugProtocol.Event[] = [
+                new InitializedEvent(),
+                new LoadedSourceEvent('new', createSource("about:blank", "about:blank", 1000)),
+                new LoadedSourceEvent('removed', createSource("about:blank", "about:blank", 1000)),
+                new LoadedSourceEvent('new', createSource("localhost:61312", "http://localhost:61312/", 1001))
+              ];
+
+              const receivedEvents: DebugProtocol.Event[] = [];
+              sendEventHandler = (event: DebugProtocol.Event) => { receivedEvents.push(event); };
+
+            await chromeDebugAdapter.attach(ATTACH_ARGS);
+            emitScriptParsed('about:blank', "1");
+
+            mockEventEmitter.emit('Debugger.globalObjectCleared');
+            mockEventEmitter.emit('Runtime.executionContextsCleared');
+            emitScriptParsed('http://localhost:61312/', "2");
+
+            await chromeDebugAdapter.doAfterProcessingSourceEvents(() => {
+                assert.deepEqual(receivedEvents, expectedEvents);
+            });
+        });
     });
 
     suite('evaluate()', () => {