Adding a landing page to support hitting breakpoints on load #513
Conversation
src/chromeDebugAdapter.ts
Outdated
| @@ -86,6 +87,10 @@ export class ChromeDebugAdapter extends CoreDebugAdapter { | |||
| } | |||
|
|
|||
| if (launchUrl) { | |||
| // We store the launch file/url provided and temporarily launch and attach to about:blank page. Once we receive configurationDone() event, we redirect the page to this file/url | |||
| // This is done to facilitate hitting breakpoints on load | |||
| this._originalUrl = launchUrl; | |||
There was a problem hiding this comment.
I'd rename this to requestedUrl or userRequestedUrl or something like that
|
Why would we always navigate to about:blank and trigger another page load? |
|
@auchenberg: Chrome needs a url to start a debug session. When we specify the target url here, it may load the page first sometimes before all the setBreakpoints requests are received. For this reason we may miss hitting breakpoints on load. So as a workaround, we now start the debug session with "about:blank" and then navigate to the target url once all the setBreakpoints requests are completed. |
|
@rakatyal We will only need this for the regex-based break-on-load breakpoints and only in the case of breakpoints set before launching, right? We should only extra work such as an extra page.navigate when it's needed. |
src/chromeDebugAdapter.ts
Outdated
| @@ -111,6 +116,12 @@ export class ChromeDebugAdapter extends CoreDebugAdapter { | |||
| return super.attach(args); | |||
| } | |||
|
|
|||
| public configurationDone(): Promise<void> { | |||
| // This means all the setBreakpoints requests have been completed. So we can navigate to the original file/url. | |||
| this.chrome.Page.navigate({url: this._userRequestedUrl}); | |||
There was a problem hiding this comment.
This means all the requests have been sent, but not necessarily handled by the adapter. I'm not sure if that's an issue or not. Should this be waiting for us to finish handling the initial setBreakpoints?
There was a problem hiding this comment.
What do you mean by not handled by the adapter? As long as Chrome receives all the requests before we navigate we should be good. Wouldn't that be the case here?
There was a problem hiding this comment.
I guess so? I'm just wondering whether it would be a problem if we do something like
- Get setBreakpoints
- Send lots of setBreakpoint requests to Chrome
- Don't get responses yet from Chrome
- Tell Chrome to navigate
Maybe Chrome will finish processing the set breakpoint requests before it actually navigates. Or maybe there could be a race case where it could load the new page before it's actually set the breakpoints, I'm not sure.
There was a problem hiding this comment.
Hmm. Is there a way where we can be sure that Chrome has finished processing those requests?
There was a problem hiding this comment.
All I can think of is waiting on the responses from Chrome. But I think this is similar to what we do in node2 already, and I don't wait before doing a 'continue' and it's never caused a problem. So I think this is fine for now.
src/chromeDebugAdapter.ts
Outdated
| @@ -88,6 +89,10 @@ export class ChromeDebugAdapter extends CoreDebugAdapter { | |||
| } | |||
|
|
|||
| if (launchUrl) { | |||
| // We store the launch file/url provided and temporarily launch and attach to about:blank page. Once we receive configurationDone() event, we redirect the page to this file/url | |||
| // This is done to facilitate hitting breakpoints on load | |||
| this._userRequestedUrl = launchUrl; | |||
There was a problem hiding this comment.
Like @auchenberg said this should only be if breakOnLoad is enabled for 'regex'.
There was a problem hiding this comment.
Okay I was trying to solve this and it didn't work as expected as I thought and the page loaded without hitting any breakpoints. I think if we don't wait for the navigation problem can be 2 fold:
- Chrome can actually load the page before receiving the setInstrumentationBreakpoint request at all.
- Since in order to hit those breakpoints, we need Chrome to receive all the setBreakpoints requests anyway so that they are stored in the pendingBreakpoints array which I use to resolve the breakpoints when Chrome stops the execution on the first line.
So I think we will have to use this in both the cases for "regex" and "instrument" and not use it when it's "none"
Thoughts?
There was a problem hiding this comment.
To be fair, we can write code to basically not use pendingBreakpoints and call setBreakpoints after we stop on the scripts just for the "instrument" case but I believe it would be complicating stuff. Also in this case, I am only setting instrumentation breakpoint when setBreakpoint is called (or when user actually has a breakpoint before launch) and not in all cases. So due to this reason too, we need to have this landing page or else we need to figure out some other way to set the instrumentation point.
There was a problem hiding this comment.
Yeah it does make sense that we need to do this in both cases, either we are setting the BPs via regex or we are setting the Instrumentation BP.
|
@auchenberg: Good point. I will update the PR. |
|
@roblourens: Updated the PR with the change to navigate only when break on load strategy is not 'none' |
No description provided.