Fix break-on-load breakpoint not hitting on the first line for Chrome #285
Conversation
src/chrome/breakOnLoadHelper.ts
Outdated
| const pausedScriptUrl = this.getPausedScriptUrlFromId(pausedLocation.scriptId); | ||
| // Important: We need to get the commited breakpoints only after all the pending breakpoints for this file have been resolved. If not this logic won't work | ||
| const commitedBps = this._chromeDebugAdapter.committedBreakpointsByUrl.get(pausedScriptUrl); | ||
| const anyBreakpointsAtPausedLocation = commitedBps.filter(bp => |
There was a problem hiding this comment.
We populate the committedBreakpointsByUrl on breakpointResolved right? Can there be a situation where we hit the stopOnEntry breakpoint and a user breakpoint on (1,1) hasn't been resolved yet?
There was a problem hiding this comment.
We block on resolvePendingBreakpointsOfPausedScript which should set all the pending breakpoints before returning. My understanding is that that will guarantee all the breakpoints are set before we continue. @roblourens: You know a lot more about the code than us, do you think that is a correct assumption?
There was a problem hiding this comment.
Hmm I think that's correct.
src/chrome/breakOnLoadHelper.ts
Outdated
|
|
||
| const pausedScriptUrl = this.getPausedScriptUrlFromId(pausedLocation.scriptId); | ||
| // Important: We need to get the commited breakpoints only after all the pending breakpoints for this file have been resolved. If not this logic won't work | ||
| const commitedBps = this._chromeDebugAdapter.committedBreakpointsByUrl.get(pausedScriptUrl); |
|
Could you add a test for this? |
src/chrome/breakOnLoadHelper.ts
Outdated
| @@ -67,12 +66,16 @@ export class BreakOnLoadHelper { | |||
| } | |||
| } | |||
|
|
|||
| private getPausedScriptUrlFromId(scriptId: string): string { | |||
There was a problem hiding this comment.
You are right. Renaming to getScriptUrlFromId
src/chrome/chromeDebugAdapter.ts
Outdated
| @@ -945,8 +945,8 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter { | |||
| } | |||
|
|
|||
| const committedBps = this._committedBreakpointsByUrl.get(script.url) || []; | |||
| if (committedBps.indexOf(params.breakpointId) === -1) { | |||
| committedBps.push(params.breakpointId); | |||
| if (committedBps.findIndex(committedBp => committedBp.breakpointId === params.breakpointId) === -1) { | |||
There was a problem hiding this comment.
I prefer if (!committedBps.find(... instead of checking index -1
The previous userBreakpointOnLine1Col1 logic depended on parts of the ScriptParsed method executing synchronically to set that instance variable before ScriptPaused started executing. In recent changes, I added some new awaits to ScriptParsed which make it not execute synchronical any more, so the existing logic doesn't work any more.