Added error handling on webview message processing

[charisk]

Original approach of the PR

Currently a UserCancellationException is thrown with silent: true and there is no catch statement around the operation that triggered the exception, the UserCancellationException is unhandled and the global error handler is hit.

This PR adds a check for silent UserCancellationException in the global error handler.

Updated approach of the PR

Add error handling around webview message processing and use the same logic as we do for commands. We use the same logic by extracting it into a reusable function.

Checklist

N/A:

• CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
• Issues have been created for any UI or other user-facing changes made by this pull request.
• [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

[koesie10]

I'm not sure whether this makes sense. I think that the global error handler is a fallback if we forget to add a proper error handler and seeing a UserCancellationException as an unhandled error is actually more likely to find a bug where we're not catching the error. When we receive "Unhandled error: Cancelled." it means that users might get an unhandled error for a genuine exception which is not caught and reported in the proper place (i.e. closer to where the exception is thrown with a custom and more useful error message).

[charisk]

I'm not sure whether this makes sense. I think that the global error handler is a fallback if we forget to add a proper error handler and seeing a UserCancellationException as an unhandled error is actually more likely to find a bug where we're not catching the error. When we receive "Unhandled error: Cancelled." it means that users might get an unhandled error for a genuine exception which is not caught and reported in the proper place (i.e. closer to where the exception is thrown with a custom and more useful error message).

I think that means catching UserCancellationExceptions wherever there could be a cancellation. Generally we don't do much with that - we just skip the error notification. So I wonder if we can just make it a global thing and then not having if (e instanceof UserCancellationException) checks in various places.

[koesie10]

I think that means catching UserCancellationExceptions wherever there could be a cancellation. Generally we don't do much with that - we just skip the error notification. So I wonder if we can just make it a global thing and then not having if (e instanceof UserCancellationException) checks in various places.

I think we should be catching UserCancellationExceptions wherever they happen since the "Unhandled error" is a fallback case that's harder to debug for other errors: if we don't catch UserCancellationExceptions, we are also not catching genuine errors that may happen. In most cases we are already catching the UserCancellationException because these are automatically caught when running in a command handler. I think my main concern is that I'm considering everything that hits the global error handler as a bug, and the errors should have been caught at another level.

[robertbrignull]

Instead of ignoring it completely we could log it, like we do in commands.ts:

vscode-codeql/extensions/ql-vscode/src/common/vscode/commands.ts

Lines 62 to 68 in 9777cb1

	if (e instanceof UserCancellationException) {
	// User has cancelled this action manually
	if (e.silent) {
	void logger.log(errorMessage.fullMessage);
	} else {
	void showAndLogWarningMessage(logger, errorMessage.fullMessage);
	}

[robertbrignull]

I don't have any strong feelings on whether we catch the error at every location or decide to ignore it in the global error handler. I can see arguments for both sides.

What kind of bugs could this hide if we do add this logic to the global error handler?

[koesie10]

What kind of bugs could this hide if we do add this logic to the global error handler?

For example, for the issue that this PR is trying to solve, the "Unhandled error" for a cancellation is an indication that it would show an "Unhandled error" when the pack download fails. It should probably show "Failed to resolve code scanning pack: Failed to download codeql/java-queries" instead of this:

I changed the downloaded pack to produce this error, but it can happen when e.g. GHCR is unavailable.

In this case, progress is also not properly reported (i.e. "Stop evaluation" is still shown), so it does point to a real bug:

[charisk]

That isn't related to user cancellation though right? Just an error that happened down the line (and a bug we have around not catching that).

Or do you mean this is why you think that whatever ends up hitting the global error handler is a bug?

[koesie10]

That isn't related to user cancellation though right? Just an error that happened down the line (and a bug we have around not catching that).

Or do you mean this is why you think that whatever ends up hitting the global error handler is a bug?

Yes, exactly. Because this "harmless" user cancellation exception hits the global error handler, we were able to find a place where we should be catching errors. There should never be a code path where we throw a user cancellation exception without having some error handling code somewhere in its call stack. In this case, we would have caught the actual bug here much later if we didn't see this unhandled error for a cancellation exception (since we're less likely to run into pack download failing). It also means that merging this PR won't resolve the issue that this PR is linked to since you can still trigger an unhandled error when pack download fails (which results in infinite progress on the evaluation).

[charisk]

Discussed with @koesie10 offline and agreed to treat code that falls into the global error handler as a bug, but that we'd create common error handling code that can be re-used for commands and webview messages (which are the main triggers of tasks).

I've updated the PR to do this.

[koesie10]

LGTM, thanks!