Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

making sure bp.actual location is not null/undefined before trying to access its line number #381

Merged
merged 1 commit into from
Nov 27, 2018

Conversation

Shenniey
Copy link
Contributor

Chrome core has a known issue where occasionally it returns an empty locations array: ChromeDevTools/devtools-protocol#103

When this happens, actual location is undefined and throws an error, causing the browser to hang without loading the page. Stack trace from logs:
"aggregated.exceptionStack":"[
"TypeError: Cannot read property 'lineNumber' of undefined\n
at committedBps.filter.bp (breakOnLoadHelper.js:100:96)\n
at Array.filter ()\n
at BreakOnLoadHelper. (breakOnLoadHelper.js:100:65)\n
at Generator.next ()\n at fulfilled (breakOnLoadHelper.js:7:58)\n
at process._tickCallback (internal/process/next_tick.js:68:7)"
]"

This change is just to make sure that the browser doesn't hang

@roblourens
Copy link
Member

@digeff can you review?

@roblourens
Copy link
Member

The locations array will be empty any time the breakpoint isn't bound to a location, so is this really a bug on their end, or just another case for us to handle?

@Shenniey
Copy link
Contributor Author

I am leaning more towards bug, but I'm not certain. From the issue it seemed as though the locations array shouldn't be empty. Also, when I was testing with some of the breakpoint locations where we are getting back empty location arrays, if I run the project without debugging through the editor and just use F12 in Chrome, the same breakpoints successfully bind to a location and pause at those locations.

Also, if I debug with Edge, we don't have this issue.

@digeff
Copy link
Contributor

digeff commented Nov 20, 2018

I'm slightly surprised that not all the elements of commitedBps have a valid location. I think this is a valid way to avoid the crash.

The other option we could consider is to modify: https://github.com/Microsoft/vscode-chrome-debug-core/blob/master/src/chrome/chromeDebugAdapter.ts#L1667 so that we don't consider the breakpoints commited if it doesn't have a valid location. I'm not sure what the consequences of that change would be, so I think that the current approach is okay.

👍

Copy link
Member

@roblourens roblourens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great but can you remove the comment? It's sort of just restating what the code does

@roblourens
Copy link
Member

I think that "committed" here means it has been sent to Chrome/Node but not necessarily "bound" to a location.

@roblourens
Copy link
Member

I'll merge and remove the comment

@roblourens roblourens merged commit 5498047 into microsoft:master Nov 27, 2018
@roblourens roblourens added this to the November 2018 milestone Dec 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants