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

Fix first line bp chrome 69 #352

Merged
merged 3 commits into from Sep 12, 2018

Conversation

Projects
None yet
4 participants
@digeff
Contributor

digeff commented Sep 11, 2018

In Chrome 68 and earlier with DOM Instrumentation Chrome used to send a Debugger.Paused on line 0,0, we would resume, and then Chrome would stop again on the first line of the file

In Chrome 69 this behavior changed, and now DOM Instrumentation Chrome sends a Debugger.Paused event and if we resume we continue directly on line 2 of the file, instead of line 1. So the logic doesn't work any more if you put a breakpoint on the first line of the file.

This change uses the same strategy that we use for regexp. If we stop on the first line of the file with DOM Instrumentation, we use that paused as the breakpoint paused.

@auchenberg

This comment has been minimized.

Show comment
Hide comment
@auchenberg

auchenberg Sep 11, 2018

Contributor

Have we open an issue in the Chrome tracker? This seems to be a version specific workaround for a platform regression.

Contributor

auchenberg commented Sep 11, 2018

Have we open an issue in the Chrome tracker? This seems to be a version specific workaround for a platform regression.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff
Contributor

digeff commented Sep 11, 2018

@@ -57,6 +58,10 @@ export class BreakOnLoadHelper {
return this._chromeDebugAdapter.scriptsById.get(scriptId).url;
}
public async setBrowserVersion(version: Version): Promise<void> {

This comment has been minimized.

@dhanvikapila

dhanvikapila Sep 11, 2018

Member

Please add some comments on why we care about the version number. Please open (and document here) a Chromium bug if you haven't already on this.

@dhanvikapila

dhanvikapila Sep 11, 2018

Member

Please add some comments on why we care about the version number. Please open (and document here) a Chromium bug if you haven't already on this.

This comment has been minimized.

@digeff

digeff Sep 11, 2018

Contributor

Done:
// 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.

@digeff

digeff Sep 11, 2018

Contributor

Done:
// 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.

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 11, 2018

Member

I think the problem isn't the lack of an "extra event", it's just that the instrumentation breakpoint now stops at the location of the first statement of the script, not always at 0,0. So it's at the same location as the breakpoint, and if we set a breakpoint on that location and continue, we won't hit that breakpoint. So I don't think they will fix it as a "bug", we should just check whether there is a real breakpoint at that same location. It looks like that's what your fix does, but only if Chrome passes a version check. So I think you can do the same fix without a version check, I suspect that if somehow the first valid bp location of a script is 0,0, then you'll see the same issue in 68. Unfortunately I can't test that because I just upgraded Chrome, how do you get an old version to test?

Member

roblourens commented Sep 11, 2018

I think the problem isn't the lack of an "extra event", it's just that the instrumentation breakpoint now stops at the location of the first statement of the script, not always at 0,0. So it's at the same location as the breakpoint, and if we set a breakpoint on that location and continue, we won't hit that breakpoint. So I don't think they will fix it as a "bug", we should just check whether there is a real breakpoint at that same location. It looks like that's what your fix does, but only if Chrome passes a version check. So I think you can do the same fix without a version check, I suspect that if somehow the first valid bp location of a script is 0,0, then you'll see the same issue in 68. Unfortunately I can't test that because I just upgraded Chrome, how do you get an old version to test?

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 11, 2018

Member

If you're able to test with 68, the case I'm thinking of is where the first line is foo(); or "test";, not let x = 1 or function foo()... or something like that. I'd be curious to see a log in that scenario.

Member

roblourens commented Sep 11, 2018

If you're able to test with 68, the case I'm thinking of is where the first line is foo(); or "test";, not let x = 1 or function foo()... or something like that. I'd be curious to see a log in that scenario.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Sep 11, 2018

Contributor

@roblourens I sent you the chrome log in 68 via e-mail.

Contributor

digeff commented Sep 11, 2018

@roblourens I sent you the chrome log in 68 via e-mail.

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 11, 2018

Member

Yeah, @digeff's case shows that in 68 we pause at 0,0 for instrumentation:scriptFirstStatement, then set the breakpoint, then resume, then pause again at 0,0 on the breakpoint. So I'm not sure how to do this without the version check.

Member

roblourens commented Sep 11, 2018

Yeah, @digeff's case shows that in 68 we pause at 0,0 for instrumentation:scriptFirstStatement, then set the breakpoint, then resume, then pause again at 0,0 on the breakpoint. So I'm not sure how to do this without the version check.

@auchenberg

This comment has been minimized.

Show comment
Hide comment
@auchenberg

auchenberg Sep 12, 2018

Contributor

If this is a regression/behavior change in Chrome, what's driving the need for us to land a workaround in the debugger, and not us driving the conversation with Chrome on getting this fixed in the platform?

Contributor

auchenberg commented Sep 12, 2018

If this is a regression/behavior change in Chrome, what's driving the need for us to land a workaround in the debugger, and not us driving the conversation with Chrome on getting this fixed in the platform?

@roblourens

This comment has been minimized.

Show comment
Hide comment
@roblourens

roblourens Sep 12, 2018

Member

The fact that we appear broken? 😁

@digeff Can you file an issue with clear steps to see the actual bug? I'm guessing the problem is basically that we won't hit a bp on the first line of a script when break-on-load is enabled. But I think we need to be more clear with Chrome about what the impact of their change is. The upstream issue should link to our issue.

Member

roblourens commented Sep 12, 2018

The fact that we appear broken? 😁

@digeff Can you file an issue with clear steps to see the actual bug? I'm guessing the problem is basically that we won't hit a bp on the first line of a script when break-on-load is enabled. But I think we need to be more clear with Chrome about what the impact of their change is. The upstream issue should link to our issue.

@digeff

This comment has been minimized.

Show comment
Hide comment
@digeff

digeff Sep 12, 2018

Contributor

I filed this issue on Chrome debug: Microsoft/vscode-chrome-debug#729

Contributor

digeff commented Sep 12, 2018

I filed this issue on Chrome debug: Microsoft/vscode-chrome-debug#729

@roblourens roblourens added this to the September 2018 milestone Sep 12, 2018

@roblourens roblourens merged commit 949773c into Microsoft:master Sep 12, 2018

2 checks passed

license/cla All CLA requirements met.
Details
vscode-chrome-debug-core-CI #20180911.3 succeeded
Details

@digeff digeff deleted the digeff:fix_first_line_bp_chrome_69 branch Sep 12, 2018

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