New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated code so now we resolve pending breakpoints onScriptParsed instead onScriptPaused when using break on load #290

Merged
merged 8 commits into from Mar 6, 2018

Conversation

Projects
None yet
2 participants
@digeff
Copy link
Contributor

digeff commented Feb 15, 2018

The general idea of this code change is that now all the pending breakpoints will get resolved onScriptParsed. Because we don't know in which order things will be executed, we'll have a defer used as a semaphore. The break-on-load logic inside onScriptPaused won't continue until the associated onScriptParsed has finished resolving all the pending breakpoints.

We need to make this change because of files that start with a function declaration, 0,0 is inside the function, so the break-on-load breakpoint is sometimes never hit, so then even after we refresh the page, no breakpoints are hit in that file because they are all still pending.

Is there a better way to solve this issue?

Some specific question:
I had to remove: "&& !pendingBP.bpsSet?" from line 697 to make this work. I'm not sure how that code was being used. It removing that code the right thing to do?
How should I handle the refresh case? Do we need to do anything special for that case? Should I call this._scriptIdToBreakpointsAreResolvedDefer.clear(); inside clearTargetContext?

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Feb 16, 2018

Is there no guarantee that a scriptParsed event will come before a pause event on that script? Have you ever seen them come in the wrong order or is it just being careful?

I had to remove: "&& !pendingBP.bpsSet?" from line 697 to make this work. I'm not sure how that code was being used. It removing that code the right thing to do?

That code is for the case where we set a BP and believe we have the correct sourcemapped location for it, but the script is not yet loaded. That exact check was added recently in c125224 and the bug was quite bad so the check should stay. But I suspect that when I fixed that, I actually broke something else which is what bpsSet was originally added for - the case when we think we have the correct location, but actually don't (so the pending BP still needs to be resolved) but I'll look at that later. Why did you have to remove it? I don't think your pending BPs should have that flag set.

How should I handle the refresh case? Do we need to do anything special for that case?

I'm not sure, what do you mean by "refresh case" exactly?

@digeff

This comment has been minimized.

Copy link
Contributor

digeff commented Feb 16, 2018

After discussing this with Dhanvi, we decided not to take this change.

@digeff digeff closed this Feb 16, 2018

@digeff digeff reopened this Feb 20, 2018

@digeff

This comment has been minimized.

Copy link
Contributor

digeff commented Feb 20, 2018

@roblourens: After discussing this in more depth, we decided to merge this, although we are not sure if we want to merge it for this release, or the next one. Please help us get this into a state where it's ready to merge, and after we get to that state we'll decide whether to merge it in this release, or in the next release

@digeff

This comment has been minimized.

Copy link
Contributor

digeff commented Feb 20, 2018

Is there no guarantee that a scriptParsed event will come before a pause event on that script? Have you ever seen them come in the wrong order or is it just being careful? --> I'm assuming that you are asking why do we need this "waiting" logic. If that's the case, onScriptParsed and onScriptPaused do always arrive in the correct order. The issue is that we are not blocking the thread until all the work gets done. The resolved breakpoints are set in a non-blocking async manner, so it's possible that onScriptPaused executes while parts of onScriptParsed are still executing (in particular before we resolve all the breakpoints).

@digeff

This comment has been minimized.

Copy link
Contributor

digeff commented Feb 20, 2018

I'm not sure, what do you mean by "refresh case" exactly? --> I'm not sure whether we need some special logic on clearTargetContext or something like that. What happens if we navigate or we refresh while we are in the middle of resolving breakpoints onScriptParsed and we navigate to the same page, or a different page? Can we have two onScriptParsed handlers for the same url been executed at the same time (one for before the refresh, one for after)? How do we handle things properly in that case?

@digeff digeff force-pushed the digeff:fix_not_hitting_bps branch from c8d0f70 to f38ce75 Feb 20, 2018

@digeff

This comment has been minimized.

Copy link
Contributor

digeff commented Feb 20, 2018

@roblourens I've updated the PR to do something reasonable with bpSet, rather than just remove it :).

const anyBreakpointsAtPausedLocation = committedBps.filter(bp =>
bp.actualLocation.lineNumber === pausedLocation.lineNumber && bp.actualLocation.columnNumber === pausedLocation.columnNumber).length > 0;

// If there were any pending breakpoints resolved and any of them was at (1,1) we shouldn't continue
if (anyPendingBreakpointsResolved && anyBreakpointsAtPausedLocation) {
// If there were any breakpoints at (1,1) we shouldn't continue

This comment has been minimized.

@roblourens

roblourens Feb 22, 2018

Member

"at (1,1)" is this still accurate? It can be anywhere now?

This comment has been minimized.

@digeff

digeff Feb 22, 2018

Contributor
    // If there were any breakpoints at this location (Which generally should be (1,1)) we shouldn't continue

if (script.url) {
script.url = utils.fixDriveLetter(script.url);
public breakpointsResolvedDefer(scriptId: string): PromiseDefer<void> {

This comment has been minimized.

@roblourens

roblourens Feb 22, 2018

Member

What's the verb in this method name, should it be a "get..."?

This comment has been minimized.

@digeff

digeff Feb 22, 2018

Contributor

getBreakpointsResolvedDefer

try {
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 () => {

This comment has been minimized.

@roblourens

roblourens Feb 22, 2018

Member

Don't mix async/await and promises when possible

This comment has been minimized.

@digeff

digeff Feb 22, 2018

Contributor
            if (typeof this._columnBreakpointsEnabled === 'undefined') {
                await this.detectColumnBreakpointSupport(script.scriptId);
                await this.sendInitializedEvent();
            }
@@ -1316,7 +1341,7 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
};
}

private targetBreakpointResponsesToClientBreakpoints(url: string, responses: ISetBreakpointResult[], requestBps: DebugProtocol.SourceBreakpoint[], ids?: number[]): DebugProtocol.Breakpoint[] {
private targetBreakpointResponsesToClientBreakpoints(url: string, responses: ISetBreakpointResult[], requestBps: DebugProtocol.SourceBreakpoint[], ids?: number[]): BreakpointSetResult[] {

This comment has been minimized.

@roblourens

roblourens Feb 22, 2018

Member

method name isn't accurate with this change

This comment has been minimized.

@digeff

digeff Feb 22, 2018

Contributor

targetBreakpointResponsesToBreakpointSetResults

return undefined;
}

if (sources) {

This comment has been minimized.

@roblourens

roblourens Feb 22, 2018

Member

When is this undefined?

This comment has been minimized.

@digeff

digeff Feb 22, 2018

Contributor

This code was added for this: #106 in 1bf19e8 so I'm assuming that the scenario is:
"sourcemaps can't be resolved ahead of time, or don't exist on disk."

if (body.breakpoints.every(bp => !bp.verified)) {
// If all breakpoints are set, we mark them as set. If not, we mark them as un-set so they'll be set
const areAllSet = setBpResultBody.breakpoints.every(setBpResult => setBpResult.isSet);

This comment has been minimized.

@roblourens

roblourens Feb 22, 2018

Member

I think this changes the way it works - bpsSet was true if the bp was attempted to be set - not that it was bound. The check below response.breakpointId !== undefined is only true when the bp is bound.

This comment has been minimized.

@digeff

digeff Feb 22, 2018

Contributor

My understanding was that Debugger.setBreakpointByUrl always returns the breakpointId if the parameters are valid even if the breakpoint is unbound, and it also returns the actualLocation if the breakpoint was bound.

This comment has been minimized.

@roblourens

roblourens Feb 23, 2018

Member

Sorry, you're correct.

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Feb 22, 2018

I'm not sure refresh is an issue, because of doAfterProcessingSourceEvents and once pending breakpoints are resolved, they don't need to become pending again because once the script has been loaded once during the lifetime of the adapter, we have set the bp in the correct location.

This is a complicated change and I'm not sure I follow the whole thing. Take a look at the feedback and let's try to have a call about it in the next day or two.

@digeff

This comment has been minimized.

Copy link
Contributor

digeff commented Feb 22, 2018

I'm adding some more awaits because if not the second call to breakpointsAreResolvedDefer.resolve() might be called to early...

@digeff digeff force-pushed the digeff:fix_not_hitting_bps branch from c1900ab to 022bb8a Feb 22, 2018

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Feb 22, 2018

Please add some integration tests for this too

return <DebugProtocol.Breakpoint>{
// If we don't have an actualLocation nor a breakpointId this is a pseudo-breakpoint because we are using break-on-load
// so we mark the breakpoint as not set, so i'll be set after we load the actual script that has the breakpoint
return { isSet: response.breakpointId !== undefined, breakpoint: <DebugProtocol.Breakpoint>{

This comment has been minimized.

@roblourens

roblourens Feb 23, 2018

Member

Nitpick, please format these like

{
	isSet...
	breakpoint: {
		...
	}
}

as I didn't notice there was a second object there at first.

This comment has been minimized.

@digeff

digeff Feb 23, 2018

Contributor

Changed

this._pendingBreakpointsByUrl.delete(source);
breakpointsAreResolvedDefer.resolve();
} else {
breakpointsAreResolvedDefer.resolve();

This comment has been minimized.

@roblourens

roblourens Feb 23, 2018

Member

You can just pull these two out of the if/else

This comment has been minimized.

@digeff

digeff Feb 23, 2018

Contributor

I didn't noticed that, thanks :)... It's actually a bug to do these here, so I'm removing both of them....

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Feb 23, 2018

Never mind about the call. I think this will be fine. If you are comfortable with this, go ahead and merge it.

});
if (this._initialSourceMapsP) {
this._initialSourceMapsP = <Promise<any>>Promise.all([this._initialSourceMapsP, sourceMapsP]);
await sourceMapsP;

This comment has been minimized.

@roblourens

roblourens Feb 24, 2018

Member

Should this always be awaited outside the if block? And why does it need to be?

This comment has been minimized.

@digeff

digeff Mar 5, 2018

Contributor

You are right, it should be outside.

We want this so we don't call breakpointsAreResolvedDefer.resolve(); until this has finished for sure. If we call breakpointsAreResolvedDefer.resolve(); too early that will cause issues.

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Feb 24, 2018

Left one more comment but you should have permission to merge it, please do that when you think it's ready. Let me know if it doesn't work, I'll do it.

@digeff

This comment has been minimized.

Copy link
Contributor

digeff commented Mar 5, 2018

@roblourens Any more feedback?

@roblourens roblourens merged commit f999465 into Microsoft:master Mar 6, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@digeff digeff deleted the digeff:fix_not_hitting_bps branch Mar 6, 2018

@roblourens roblourens added this to the March 2018 milestone Apr 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment