Fix `Unhandled error: Invalid arguments` bug by elenatanasoiu · Pull Request #2104 · github/vscode-codeql

elenatanasoiu · 2023-02-21T15:26:41Z

When we added an extra optional param isTutorialDatabase to openDatabase 1 , the tests all passed but there is at least one call 2 where this triggers an Unhandled arguments error.

This is because the method call doesn't know which optional param we're passing (displayName or isTutorialDatabase).

Let's revert back to the original method signature where we had just one optional parameter (the database's displayName) and set it to a special "CodeQL Tutorial Database" value (which we want to do anyway).

We can then use this to skip creating a skeleton pack for the tutorial database since it comes with a checked-in QL pack.

NB: Unfortunately the error message doesn't link to any further information or a stack trace but after testing with this change, the error is resolved.

extensions/ql-vscode/src/local-databases-ui.ts

elenatanasoiu · 2023-02-21T17:35:44Z

Unfortunately this doesn't fix all of the occurrences of "Unhandled arguments" and I don't have a stack trace to work with in the codespace when these errors are thrown. So I'm thinking of disabling this functionality of catching unhandled rejections for the codespace as it's preventing us from running the code tour successfully at the moment.

elenatanasoiu · 2023-02-21T18:57:38Z

Chatted to @aeisenberg about this. It looks like the unhandled rejection listener will pick up errors from other extensions, not just our own, which is why it's not working with the code tour:

Stack trace

stack trace: Error: Invalid arguments
    at new i (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:94:15029)
    at apply (/Users/elenatanasoiu/.vscode/extensions/vsls-contrib.codetour-0.0.58/src/player/decorator.ts:97:23)
    at Array.map (<anonymous>)
    at Proxy.map (/Users/elenatanasoiu/.vscode/extensions/vsls-contrib.codetour-0.0.58/node_modules/mobx/lib/mobx.module.js:3381:30)
    at /Users/elenatanasoiu/.vscode/extensions/vsls-contrib.codetour-0.0.58/src/player/decorator.ts:96:43
    at Generator.next (<anonymous>)
    at s (/Users/elenatanasoiu/.vscode/extensions/vsls-contrib.codetour-0.0.58/dist/extension-node.js:16:122957)

This means that our extension might not play well if the user has other extensions installed.

For now I'm disabling it for the code tour extension but we probably want to consider only logging these errors if they're coming from the ql-vscode extension.

When we added an extra optional param `isTutorialDatabase` to `openDatabase` [1] , the tests all passed but there is at least one call [2] where this triggers an `Unhandled arguments` error. This is because the method call doesn't know which optional param we're passing. Let's revert back to the original method signature where we had just one optional parameter (the database name) and set it to a special "CodeQL Tutorial Database" value. We can then use this to understand that the extension is running in the codespace and we want to skip creating a skeleton pack for it since it's the tutorial database. [1]: https://github.com/github/vscode-codeql/blob/5e51bb57f5f76b57768fb6a3543f153f5752c23c/extensions/ql-vscode/src/local-databases.ts#L610 [2]: https://github.com/github/vscode-codeql/blob/5e51bb57f5f76b57768fb6a3543f153f5752c23c/extensions/ql-vscode/src/databaseFetcher.ts#L257-L262

I'm extracting the call to `openDatabase` where we set a custom display name into a separate method: `openTutorialDatabase`. I've added comments to it to specify the display name is significant. I've also added a test that will fail if you ever change the name of the tutorial database.

This was implemented here: [1]. It is throwing up a lot of `Unhandled error: Invalid arguments` errors without any way to see a stack trace for them. Since we're not able to debug this from the codespace, I'm choosing to turn it off in the codespace template as it's preventing us from running the Code Tour successfully.

aeisenberg

If this is blocking your progress, then I think we can go ahead with this change, but realize that this is technical debt that should be addressed later.

aeisenberg · 2023-02-21T21:05:11Z

extensions/ql-vscode/src/extension.ts

+  if (!isCodespacesTemplate()) {
+    addUnhandledRejectionListener();
+  }
+


I'm concerned about just disabling the unhandled rejection listener for the template. It means that none of our new users will get the benefits of it. I think we'd be better off inspecting the stack frames for references to our sources (as we discussed on our chat).

My biggest concern is that we're going to display errors from other extensions (I think it's even possible for this to happen now, just no one has reported it yet).

robertbrignull · 2023-02-22T09:44:04Z

This is because the method call doesn't know which optional param we're passing (displayName or isTutorialDatabase).

Could you explain more? I've never heard of this happening before, and it sounds like a serious bug in javascript if this is what's happening. Have you found any documentation / bug reports / anyone else who had the same error?

The closest I've found online is https://un5puftqgj2eayzdj31ea7zq.julianrbryant.com/docs/Web/JavaScript/Reference/Errors/Typed_array_invalid_arguments which implies it could be a case of abusing some javascript builtin function and it's throwing this error. Unfortunately doesn't really help tracking down where it's happening if the exception didn't have a stack trace.

NB: Unfortunately the error message doesn't link to any further information or a stack trace but after testing with this change, the error is resolved.

We could change the "unhandled error" listener to also log the stack trace. I think it currently only logs the message and sends the stack trace to the telemetry. It could be useful if it logs the stack trace as well so debugging error reports from users is easier.

robertbrignull · 2023-02-22T09:53:15Z

Chatted to @aeisenberg about this. It looks like the unhandled rejection listener will pick up errors from other extensions, not just our own, which is why it's not working with the code tour.

Hmm, I wasn't aware it did this. I thought / hoped that each extension would be its own node process and therefore we'd be isolated from each other and not able to break each other by changing global properties. This is potentially problematic. I agree we should make it filter the errors to only those that occur in our extension. That doesn't have to / shouldn't be this PR though. I'll open an issue to investigate it.

This means that our extension might not play well if the user has other extensions installed.

It will show more errors to the user, but as far as I'm aware it won't change the behaviour of the code producing the errors. So in the case of this "invalid arguments" error I think it's happening regardless and we just aren't aware of it without the error listener. Happy to be convinced otherwise if there's evidence though.

For now I'm disabling it for the code tour extension

Is the code tour a separate extension from the main CodeQL extension, or instead an optional feature/mode of the CodeQL extension? I'm not that aware of how the code tour in implemented. Probably doesn't affect what we do in this PR but I'm keen to understand.

robertbrignull · 2023-02-22T10:08:11Z

I've opened #2107 which reverts the PR that introduced the listener. I've also opened an internal issue to investigate what's going on here regarding errors from other extensions.

elenatanasoiu · 2023-02-22T10:10:31Z

Could you explain more? I've never heard of this happening before, and it sounds like a serious bug in javascript if this is what's happening. Have you found any documentation / bug reports / anyone else who had the same error?

I think we can actually drop those changes related to optional parameters. I was guessing where the problem was at that point because I couldn't see a stack trace. I think the only thing to fix is to disable the listener for the code tour.

We could change the "unhandled error" listener to also log the stack trace. I think it currently only logs the message and sends the stack trace to the telemetry. It could be useful if it logs the stack trace as well so debugging error reports from users is easier.

You can actually see the stack trace in the extension. If you look closely, the error is in .vscode/extensions/vsls-contrib.codetour-0.0.58/src/player/decorator.ts. It would be great if we could make it clear the error is being thrown by the listener.

I'll open an issue to investigate it.

Thanks!

Happy to be convinced otherwise if there's evidence though.

It happens when you move through each of the steps in code tour so it looks like it's broken. But other than that, I'm not sure how visible this would be. I think it depends what other extensions you have installed. They would have the error but the CodeQL extension would be the one that would throw the error so it would look broken.

Is the code tour a separate extension from the main CodeQL extension, or instead an optional feature/mode of the CodeQL extension?

It's a separate extension: https://github.com/microsoft/codetour.

elenatanasoiu · 2023-02-22T10:11:17Z

@robertbrignull Thanks for opening #2107! I'm going to close this since that fixes my problem.

I was initially trying to understand why this method was failing due to an unrelated error [1] so I ended up over-engineering the path parsing. We can use the path from the first workspace folder, like we do in other places in the extension. [1]: #2104

elenatanasoiu requested a review from a team as a code owner February 21, 2023 15:26

norascheuch reviewed Feb 21, 2023

View reviewed changes

extensions/ql-vscode/src/local-databases-ui.ts Outdated Show resolved Hide resolved

elenatanasoiu changed the title ~~Fix Unhandled arguments bug~~ Fix Unhandled error: Invalid arguments bug Feb 21, 2023

elenatanasoiu added the secexp label Feb 21, 2023

elenatanasoiu requested a review from a team February 21, 2023 17:44

elenatanasoiu added 3 commits February 21, 2023 19:16

elenatanasoiu force-pushed the elena/fix-unhandled-arguments branch from ddf2abf to 5269e0b Compare February 21, 2023 19:16

aeisenberg reviewed Feb 21, 2023

View reviewed changes

robertbrignull mentioned this pull request Feb 22, 2023

Revert "Add unhandled rejection listener" #2107

Merged

elenatanasoiu closed this Feb 22, 2023

elenatanasoiu deleted the elena/fix-unhandled-arguments branch February 22, 2023 11:34

elenatanasoiu mentioned this pull request Feb 22, 2023

Install dependencies for tutorial query in codespace #2090

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `Unhandled error: Invalid arguments` bug#2104

Fix `Unhandled error: Invalid arguments` bug#2104
elenatanasoiu wants to merge 3 commits intomainfrom
elena/fix-unhandled-arguments

elenatanasoiu commented Feb 21, 2023 •

edited

Loading

Uh oh!

Uh oh!

elenatanasoiu commented Feb 21, 2023 •

edited

Loading

Uh oh!

elenatanasoiu commented Feb 21, 2023 •

edited

Loading

Uh oh!

aeisenberg left a comment

Uh oh!

aeisenberg Feb 21, 2023

Uh oh!

robertbrignull commented Feb 22, 2023

Uh oh!

robertbrignull commented Feb 22, 2023

Uh oh!

robertbrignull commented Feb 22, 2023

Uh oh!

elenatanasoiu commented Feb 22, 2023

Uh oh!

elenatanasoiu commented Feb 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

elenatanasoiu commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

elenatanasoiu commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elenatanasoiu commented Feb 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

aeisenberg Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

robertbrignull commented Feb 22, 2023

Uh oh!

robertbrignull commented Feb 22, 2023

Uh oh!

robertbrignull commented Feb 22, 2023

Uh oh!

elenatanasoiu commented Feb 22, 2023

Uh oh!

elenatanasoiu commented Feb 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elenatanasoiu commented Feb 21, 2023 •

edited

Loading

elenatanasoiu commented Feb 21, 2023 •

edited

Loading

elenatanasoiu commented Feb 21, 2023 •

edited

Loading