From 8e42c594c7cc1b79470bd5c4d0ccb8e971e23697 Mon Sep 17 00:00:00 2001 From: Diego Geffner Date: Wed, 20 Jun 2018 09:55:14 -0700 Subject: [PATCH 1/2] Fix possible race condition for new and changed scripts --- .vscode/launch.json | 2 +- src/chrome/chromeDebugAdapter.ts | 9 ++-- test/chrome/chromeDebugAdapter.test.ts | 73 ++++++++++++++++++++++++++ 3 files changed, 77 insertions(+), 7 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index f0b22488c..276d5c918 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -21,7 +21,7 @@ "request": "launch", "protocol": "inspector", "program": "${workspaceFolder}/node_modules/mocha/bin/_mocha", - "args": [ "--no-timeouts", "${workspaceFolder}/out/**/*.js", "--ui", "tdd", "--grep", "" ], + "args": [ "--no-timeouts", "${workspaceFolder}/out/**/*.js", "--ui", "tdd", "--grep", "fs.access" ], "cwd": "${workspaceFolder}", "smartStep": true, "skipFiles": [ diff --git a/src/chrome/chromeDebugAdapter.ts b/src/chrome/chromeDebugAdapter.ts index 53c390f17..fdd891e24 100644 --- a/src/chrome/chromeDebugAdapter.ts +++ b/src/chrome/chromeDebugAdapter.ts @@ -857,6 +857,8 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter { } protected async sendLoadedSourceEvent(script: Crdp.Debugger.ScriptParsedEvent, loadedSourceEventReason: LoadedSourceEventReason = 'new'): Promise { + const source = await this.scriptToSource(script); + // This is a workaround for an edge bug, see https://github.com/Microsoft/vscode-chrome-debug-core/pull/329 switch (loadedSourceEventReason) { case 'new': @@ -878,7 +880,7 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter { telemetry.reportEvent('LoadedSourceEventError', { issue: 'Unknown reason', reason: loadedSourceEventReason }); } - const scriptEvent = await this.scriptToLoadedSourceEvent(loadedSourceEventReason, script); + const scriptEvent = new LoadedSourceEvent(loadedSourceEventReason, source as any); this._session.sendEvent(scriptEvent); } @@ -1944,11 +1946,6 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter { }; } - private async scriptToLoadedSourceEvent(reason: LoadedSourceEventReason, script: Crdp.Debugger.ScriptParsedEvent): Promise { - const source = await this.scriptToSource(script); - return new LoadedSourceEvent(reason, source as any); - } - private async scriptToSource(script: Crdp.Debugger.ScriptParsedEvent): Promise { const sourceReference = this.getSourceReferenceForScriptId(script.scriptId); const origin = this.getReadonlyOrigin(script.url); diff --git a/test/chrome/chromeDebugAdapter.test.ts b/test/chrome/chromeDebugAdapter.test.ts index 6758afae4..2c2821a8a 100644 --- a/test/chrome/chromeDebugAdapter.test.ts +++ b/test/chrome/chromeDebugAdapter.test.ts @@ -22,6 +22,7 @@ import { Protocol as Crdp } from 'devtools-protocol'; import * as testUtils from '../testUtils'; import * as utils from '../../src/utils'; +import * as fs from 'fs'; /** Not mocked - use for type only */ import {ChromeDebugAdapter as _ChromeDebugAdapter } from '../../src/chrome/chromeDebugAdapter'; @@ -603,6 +604,78 @@ suite('ChromeDebugAdapter', () => { }); }); + // This is needed for Edge debug adapter, please keep the logic of sendLoadedSourceEvent() + test('tests that sendLoadedSourceEvent will set the `reason` parameter based on our internal view of the events we sent to the client even if fs.access takes unexpected times while blocking async', async () => { + let eventIndex = 0; + sendEventHandler = (event) => { + switch (eventIndex) { + case 0: + assert.equal('loadedSource', event.event); + assert.notEqual(null, event.body); + assert.equal('new', event.body.reason); + break; + case 1: + assert.equal('loadedSource', event.event); + assert.notEqual(null, event.body); + assert.equal('changed', event.body.reason); + break; + default: + throw new RangeError('Unexpected event index'); + } + ++eventIndex; + }; + + await chromeDebugAdapter.attach(ATTACH_ARGS); + + const originalFSAccess = fs.access; + let callIndex = 0; + let callbackForFirstEvent = null; + + /* Mock fs.access so the first call will block until the second call is finished */ + (fs as any).access = (path, callback) => { + if (callIndex === 0) { + callbackForFirstEvent = callback; + console.log("Blocking first fs.access until second call is finished"); + ++callIndex; + } else { + callback(); + + if (callbackForFirstEvent !== null) { + console.log("Second call when thorugh. Unblocking first call"); + setTimeout(callbackForFirstEvent, 50); + callbackForFirstEvent = null; + } + } + }; + + try { + const firstEvent = (chromeDebugAdapter).sendLoadedSourceEvent({ + scriptId: 1, + url: '', + startLine: 0, + startColumn: 0, + endLine: 0, + endColumn: 0, + executionContextId: 0, + hash: '' + }); + const secondEvent = (chromeDebugAdapter).sendLoadedSourceEvent({ + scriptId: 1, + url: '', + startLine: 0, + startColumn: 0, + endLine: 0, + endColumn: 0, + executionContextId: 0, + hash: '' + }); + + await Promise.all([firstEvent, secondEvent]); + } finally { + (fs as any).access = originalFSAccess; + } + }); + function createSource(name: string, path?: string, sourceReference?: number, origin?: string): Source { return { name: name, From 414e64586fcc83fa60c613989e7fafbae2b815a7 Mon Sep 17 00:00:00 2001 From: Diego Geffner Date: Wed, 20 Jun 2018 09:57:05 -0700 Subject: [PATCH 2/2] fix --- .vscode/launch.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index 276d5c918..f0b22488c 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -21,7 +21,7 @@ "request": "launch", "protocol": "inspector", "program": "${workspaceFolder}/node_modules/mocha/bin/_mocha", - "args": [ "--no-timeouts", "${workspaceFolder}/out/**/*.js", "--ui", "tdd", "--grep", "fs.access" ], + "args": [ "--no-timeouts", "${workspaceFolder}/out/**/*.js", "--ui", "tdd", "--grep", "" ], "cwd": "${workspaceFolder}", "smartStep": true, "skipFiles": [