Fix first line bp chrome 69 #352
Conversation
digeff
force-pushed
the
fix_first_line_bp_chrome_69
branch
from
37bc112 to
43078e3
Compare
|
Have we open an issue in the Chrome tracker? This seems to be a version specific workaround for a platform regression. |
|
Just filed a bug: https://bugs.chromium.org/p/chromium/issues/detail?id=882909 |
| @@ -57,6 +58,10 @@ export class BreakOnLoadHelper { | |||
| return this._chromeDebugAdapter.scriptsById.get(scriptId).url; | |||
| } | |||
|
|
|||
| public async setBrowserVersion(version: Version): Promise<void> { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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? |
|
If you're able to test with 68, the case I'm thinking of is where the first line is |
|
@roblourens I sent you the chrome log in 68 via e-mail. |
|
Yeah, @digeff's case shows that in 68 we pause at 0,0 for |
|
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? |
|
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. |
|
I filed this issue on Chrome debug: https://github.com/Microsoft/vscode-chrome-debug/issues/729 |
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.