From 43078e3b959bf1a8979b3a960eaf0a58b372e9ff Mon Sep 17 00:00:00 2001 From: Diego Geffner Date: Mon, 10 Sep 2018 17:40:08 -0700 Subject: [PATCH 1/3] Fix break on load on first line of Chrome 69 --- src/chrome/breakOnLoadHelper.ts | 10 ++++- src/chrome/chromeConnection.ts | 8 ++-- src/chrome/chromeDebugAdapter.ts | 4 ++ src/chrome/chromeTargetDiscoveryStrategy.ts | 41 +++++++++++++------ src/index.ts | 4 +- .../chromeTargetDiscoveryStrategy.test.ts | 8 ++-- 6 files changed, 51 insertions(+), 24 deletions(-) diff --git a/src/chrome/breakOnLoadHelper.ts b/src/chrome/breakOnLoadHelper.ts index 0ea037a55..e1d14f36f 100644 --- a/src/chrome/breakOnLoadHelper.ts +++ b/src/chrome/breakOnLoadHelper.ts @@ -10,7 +10,7 @@ import { ChromeDebugAdapter } from './chromeDebugAdapter'; import * as ChromeUtils from './chromeUtils'; import * as assert from 'assert'; import { InternalSourceBreakpoint } from './internalSourceBreakpoint'; -import { utils } from '..'; +import { utils, Version } from '..'; export interface UrlRegexAndFileSet { urlRegex: string; @@ -18,6 +18,11 @@ export interface UrlRegexAndFileSet { } export class BreakOnLoadHelper { + private _doesDOMInstrumentationRecieveExtraEvent = false; + + public async setBrowserVersion(version: Version): Promise { + this._doesDOMInstrumentationRecieveExtraEvent = !version.isAtLeastVersion(69, 0); + } private _instrumentationBreakpointSet = false; @@ -77,7 +82,8 @@ export class BreakOnLoadHelper { // Now we wait for all the pending breakpoints to be resolved and then continue await this._chromeDebugAdapter.getBreakpointsResolvedDefer(pausedScriptId).promise; logger.log('BreakOnLoadHelper: Finished waiting for breakpoints to get resolved.'); - return true; + let shouldContinue = this._doesDOMInstrumentationRecieveExtraEvent || await this.handleStopOnEntryBreakpointAndContinue(notification); + return shouldContinue; } return false; diff --git a/src/chrome/chromeConnection.ts b/src/chrome/chromeConnection.ts index 122f27d32..0714e09ff 100644 --- a/src/chrome/chromeConnection.ts +++ b/src/chrome/chromeConnection.ts @@ -9,7 +9,7 @@ import { StepProgressEventsEmitter, IObservableEvents, IStepStartedEventsEmitter import * as errors from '../errors'; import * as utils from '../utils'; import { logger } from 'vscode-debugadapter'; -import { ChromeTargetDiscovery, ProtocolSchema } from './chromeTargetDiscoveryStrategy'; +import { ChromeTargetDiscovery, TargetVersions } from './chromeTargetDiscoveryStrategy'; import { Client, LikeSocket } from 'noice-json-rpc'; @@ -27,7 +27,7 @@ export interface ITarget { type: string; url?: string; webSocketDebuggerUrl: string; - version: Promise; + version: Promise; } export type ITargetFilter = (target: ITarget) => boolean; @@ -169,7 +169,7 @@ export class ChromeConnection implements IObservableEventsthis.api.Runtime).run() ]) - .then(() => { }, e => { }); + .then(() => { }, () => { }); } public close(): void { @@ -180,7 +180,7 @@ export class ChromeConnection implements IObservableEvents { + public get version(): Promise { return this._attachedTarget.version; } } \ No newline at end of file diff --git a/src/chrome/chromeDebugAdapter.ts b/src/chrome/chromeDebugAdapter.ts index 71ac27b87..380f7df05 100644 --- a/src/chrome/chromeDebugAdapter.ts +++ b/src/chrome/chromeDebugAdapter.ts @@ -586,6 +586,10 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter { } catch (e) { // Not supported by older runtimes, ignore it. } + + if (this._breakOnLoadHelper) { + this._breakOnLoadHelper.setBrowserVersion((await this._chromeConnection.version).browser); + } } } diff --git a/src/chrome/chromeTargetDiscoveryStrategy.ts b/src/chrome/chromeTargetDiscoveryStrategy.ts index 7c3b2743e..70902741c 100644 --- a/src/chrome/chromeTargetDiscoveryStrategy.ts +++ b/src/chrome/chromeTargetDiscoveryStrategy.ts @@ -14,9 +14,16 @@ import { ITargetDiscoveryStrategy, ITargetFilter, ITarget } from './chromeConnec import * as nls from 'vscode-nls'; const localize = nls.loadMessageBundle(); -export class ProtocolSchema { - public static unknownVersion(): ProtocolSchema { - return new ProtocolSchema(0, 0); // Using 0.0 will make behave isAtLeastVersion as if this was the oldest possible version +export class Version { + static parse(versionString: string): Version { + const majorAndMinor = versionString.split('.'); + const major = parseInt(majorAndMinor[0], 10); + const minor = parseInt(majorAndMinor[1], 10); + return new Version(major, minor); + } + + public static unknownVersion(): Version { + return new Version(0, 0); // Using 0.0 will make behave isAtLeastVersion as if this was the oldest possible version } constructor(private _major: number, private _minor: number) {} @@ -26,6 +33,10 @@ export class ProtocolSchema { } } +export class TargetVersions { + constructor(public readonly protocol: Version, public readonly browser: Version) {} +} + export class ChromeTargetDiscovery implements ITargetDiscoveryStrategy, IObservableEvents { private logger: Logger.ILogger; private telemetry: telemetry.ITelemetryReporter; @@ -67,7 +78,7 @@ export class ChromeTargetDiscovery implements ITargetDiscoveryStrategy, IObserva return this._getMatchingTargets(targets, targetFilter, targetUrl); } - private async _getVersionData(address: string, port: number): Promise { + private async _getVersionData(address: string, port: number): Promise { const url = `http://${address}:${port}/json/version`; this.logger.log(`Getting browser and debug protocol version via ${url}`); @@ -78,9 +89,10 @@ export class ChromeTargetDiscovery implements ITargetDiscoveryStrategy, IObserva try { if (jsonResponse) { const response = JSON.parse(jsonResponse); - const versionString = response['Protocol-Version'] as string; - this.logger.log(`Got browser version: ${response.Browser}`); - this.logger.log(`Got debug protocol version: ${versionString}`); + const protocolVersionString = response['Protocol-Version'] as string; + const browserWithPrefixVersionString = response['Browser'] as string; + this.logger.log(`Got browser version: ${browserWithPrefixVersionString }`); + this.logger.log(`Got debug protocol version: ${protocolVersionString}`); /* __GDPR__ "targetDebugProtocolVersion" : { @@ -88,16 +100,21 @@ export class ChromeTargetDiscovery implements ITargetDiscoveryStrategy, IObserva "${include}": [ "${DebugCommonProperties}" ] } */ + + const chromePrefix = 'Chrome/'; + let browserVersion = Version.unknownVersion(); + if (browserWithPrefixVersionString.startsWith(chromePrefix)) { + const browserVersionString = browserWithPrefixVersionString.substr(chromePrefix.length); + browserVersion = Version.parse(browserVersionString); + } + this.telemetry.reportEvent('targetDebugProtocolVersion', { debugProtocolVersion: response['Protcol-Version'] }); - const majorAndMinor = versionString.split('.'); - const major = parseInt(majorAndMinor[0], 10); - const minor = parseInt(majorAndMinor[1], 10); - return new ProtocolSchema(major, minor); + return new TargetVersions(Version.parse(protocolVersionString), browserVersion); } } catch (e) { this.logger.log(`Didn't get a valid response for /json/version call. Error: ${e.message}. Response: ${jsonResponse}`); } - return ProtocolSchema.unknownVersion(); + return new TargetVersions(Version.unknownVersion(), Version.unknownVersion()); } private async _getTargets(address: string, port: number): Promise { diff --git a/src/index.ts b/src/index.ts index 033b08cfc..425349b9a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -28,7 +28,7 @@ import { NullLogger } from './nullLogger'; import * as executionTimingsReporter from './executionTimingsReporter'; import { Protocol as Crdp } from 'devtools-protocol'; -import { ProtocolSchema } from './chrome/chromeTargetDiscoveryStrategy'; +import { Version } from './chrome/chromeTargetDiscoveryStrategy'; export { chromeConnection, @@ -55,7 +55,7 @@ export { NullLogger, executionTimingsReporter, - ProtocolSchema, + Version, Crdp }; diff --git a/test/chrome/chromeTargetDiscoveryStrategy.test.ts b/test/chrome/chromeTargetDiscoveryStrategy.test.ts index 60eef5f7e..86074b18c 100644 --- a/test/chrome/chromeTargetDiscoveryStrategy.test.ts +++ b/test/chrome/chromeTargetDiscoveryStrategy.test.ts @@ -10,7 +10,7 @@ import { ITargetDiscoveryStrategy } from '../../src/chrome/chromeConnection'; import { NullLogger } from '../../src/nullLogger'; import { NullTelemetryReporter } from '../../src/telemetry'; -import { ProtocolSchema } from '../../src'; +import { Version } from '../../src'; const MODULE_UNDER_TEST = '../../src/chrome/chromeTargetDiscoveryStrategy'; suite('ChromeTargetDiscoveryStrategy', () => { @@ -218,7 +218,7 @@ suite('ChromeTargetDiscoveryStrategy', () => { }); test('ProtocolSchema return if the version is at least', () => { - const schema0dot1 = new ProtocolSchema(0, 1); + const schema0dot1 = new Version(0, 1); assert.ok(schema0dot1.isAtLeastVersion(0, 0)); assert.ok(schema0dot1.isAtLeastVersion(0, 1)); assert.ok(!schema0dot1.isAtLeastVersion(0, 2)); @@ -226,7 +226,7 @@ suite('ChromeTargetDiscoveryStrategy', () => { assert.ok(!schema0dot1.isAtLeastVersion(1, 1)); assert.ok(!schema0dot1.isAtLeastVersion(1, 2)); - const schema0dot2 = new ProtocolSchema(0, 2); + const schema0dot2 = new Version(0, 2); assert.ok(schema0dot2.isAtLeastVersion(0, 0)); assert.ok(schema0dot2.isAtLeastVersion(0, 1)); assert.ok(schema0dot2.isAtLeastVersion(0, 2)); @@ -234,7 +234,7 @@ suite('ChromeTargetDiscoveryStrategy', () => { assert.ok(!schema0dot2.isAtLeastVersion(1, 1)); assert.ok(!schema0dot2.isAtLeastVersion(1, 2)); - const schema1dot0 = new ProtocolSchema(1, 0); + const schema1dot0 = new Version(1, 0); assert.ok(schema1dot0.isAtLeastVersion(0, 0)); assert.ok(schema1dot0.isAtLeastVersion(0, 1)); assert.ok(schema1dot0.isAtLeastVersion(0, 2)); From cbd2060fe2014de936194a07a90922fe1f746371 Mon Sep 17 00:00:00 2001 From: Diego Geffner Date: Tue, 11 Sep 2018 08:36:08 -0700 Subject: [PATCH 2/3] tslint --- src/chrome/breakOnLoadHelper.ts | 8 ++++---- src/chrome/chromeTargetDiscoveryStrategy.ts | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/chrome/breakOnLoadHelper.ts b/src/chrome/breakOnLoadHelper.ts index e1d14f36f..75f7c5e11 100644 --- a/src/chrome/breakOnLoadHelper.ts +++ b/src/chrome/breakOnLoadHelper.ts @@ -20,10 +20,6 @@ export interface UrlRegexAndFileSet { export class BreakOnLoadHelper { private _doesDOMInstrumentationRecieveExtraEvent = false; - public async setBrowserVersion(version: Version): Promise { - this._doesDOMInstrumentationRecieveExtraEvent = !version.isAtLeastVersion(69, 0); - } - private _instrumentationBreakpointSet = false; // Break on load: Store some mapping between the requested file names, the regex for the file, and the chrome breakpoint id to perform lookup operations efficiently @@ -62,6 +58,10 @@ export class BreakOnLoadHelper { return this._chromeDebugAdapter.scriptsById.get(scriptId).url; } + public async setBrowserVersion(version: Version): Promise { + this._doesDOMInstrumentationRecieveExtraEvent = !version.isAtLeastVersion(69, 0); + } + /** * Handles the onpaused event. * Checks if the event is caused by a stopOnEntry breakpoint of using the regex approach, or the paused event due to the Chrome's instrument approach diff --git a/src/chrome/chromeTargetDiscoveryStrategy.ts b/src/chrome/chromeTargetDiscoveryStrategy.ts index 70902741c..12c33ea71 100644 --- a/src/chrome/chromeTargetDiscoveryStrategy.ts +++ b/src/chrome/chromeTargetDiscoveryStrategy.ts @@ -90,7 +90,7 @@ export class ChromeTargetDiscovery implements ITargetDiscoveryStrategy, IObserva if (jsonResponse) { const response = JSON.parse(jsonResponse); const protocolVersionString = response['Protocol-Version'] as string; - const browserWithPrefixVersionString = response['Browser'] as string; + const browserWithPrefixVersionString = response.Browser as string; this.logger.log(`Got browser version: ${browserWithPrefixVersionString }`); this.logger.log(`Got debug protocol version: ${protocolVersionString}`); From c566a90c569a2c8b0896d585b81cf302147d86e8 Mon Sep 17 00:00:00 2001 From: Diego Geffner Date: Tue, 11 Sep 2018 10:37:19 -0700 Subject: [PATCH 3/3] Feedback --- src/chrome/breakOnLoadHelper.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/chrome/breakOnLoadHelper.ts b/src/chrome/breakOnLoadHelper.ts index 75f7c5e11..23c180ec1 100644 --- a/src/chrome/breakOnLoadHelper.ts +++ b/src/chrome/breakOnLoadHelper.ts @@ -59,6 +59,8 @@ export class BreakOnLoadHelper { } public async setBrowserVersion(version: Version): Promise { + // On version 69 Chrome stopped sending an extra event for DOM Instrumentation: See https://bugs.chromium.org/p/chromium/issues/detail?id=882909 + // On Chrome 68 we were relying on that event to make Break on load work on breakpoints on the first line of a file. On Chrome 69 we need an alternative way to make it work. this._doesDOMInstrumentationRecieveExtraEvent = !version.isAtLeastVersion(69, 0); }