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

[digeff]

We were running into an issue where we send 'remove' myscript.js before we sent the 'add' myscript.js when we went into a page, and quickly redirected into another page.

That happened because we have some async code in how we process onScriptParsed, so that processing can be on-flight, and onExecutionContextsCleared very quickly sends the 'remove' events, and we weren't waiting for the onScriptParsed events to be completed.
This fixes solve part of the issue.

I think there is another unrelated reason that I'm investigating after this.

[roblourens]

I don't think this will fix your problem. await won't keep the code in onExecutionContextsCleared from running. Your change will keep the rest of the code in onScriptParsed from running before all the scriptParsed notifications are sent, but won't stop other event handlers.

You probably want a promise that you use as an async queue, like

private _scriptParsedQueue: Promise<any>;

...

onScriptParsed() {
  this._scriptParsedQueue = this._scriptParsedQueue.then(() => send load events)
}

onExecutionContextsCleared() {
  this._scriptParsedQueue.then(() => send delete events)
}

[roblourens]

The problem is that we are getting a breakpointResolved event for the new script before you have processed the scriptLoaded event, so the breakpointResolved handler stops at this line: https://github.com/Microsoft/vscode-chrome-debug-core/blob/master/src/chrome/chromeDebugAdapter.ts#L930

Two options:

1. Only delay the sending of the events to the client, not the whole onScriptParsed handler
2. Have the breakpointResolved handler also wait on the scriptParsedQueue

I think the first one is better for perf, and would hopefully prevent other issues with things that depend on the _scriptsById map.

[roblourens]

Could you add a test to the PR as well, that adds some scripts, refreshes the page, and ensures that the script load/unload events are sent in the proper order? You can take some inspiration from this failing test on how to do that.

[digeff]

Ran into a bug with the newest typescript: microsoft/TypeScript#21491
changing to an older version

[roblourens]

The mixing of async/await and promises is a little confusing here (await ... .then()

[digeff]

Done

[roblourens]

The async/await inside map is redundant, just return the promise from this.sendLoadedSourceEvent

[digeff]

Done

[roblourens]

action returns a Promise

[digeff]

The use inside onExecutionContextsCleared() doesn't return a promise

[roblourens]

This is confusing, asyncOperations will not be populated by this point because it's populated by an async function that we don't wait on. I'm not sure what's supposed to happen.

[digeff]

Nice catch... I missed this...

[roblourens]

I think this method picked up an extra level of indentation

[digeff]

Done

[roblourens]

This is a confusing flow now. The asyncOperations array isn't needed if doAfterProcessingSourceEvents is forcing this to happen after sending the script removed events are sent.

Also, you could iterate the scripts inside the doAfterProcessingSourceEvent. That will be faster because the script processing doesn't have to happen one by one. You can use the await Promise.all(scriptById.map(async () => ... pattern again.

[digeff]

I rewrote this to use async/wait. What do you think?

[roblourens]

I think this is right. Async code is hard :) Thanks!