Add listeners for unhandled errors to web views#2139
Conversation
|
There's also a bug I'm still working out, but I decided to open the PR anyway while I look into it. We're getting duplicate |
|
Seems the error firing twice is due to react error boundaries. Possibly due to the default error boundary re-throwing. See facebook/react#11499 I'm going to try implementing an error boundary and see how it behaves. That will only affect errors thrown when rendering. From reading the issue it seems we could still end up with duplicate errors from event handlers. We can decide if that's acceptable, or whether to try fixing it by remembering which errors we've seen previously. It doesn't appear to show a duplicate popup, so the user won't notice that there are two errors. It'll only show up in our telemetry and make the numbers a bit inflated. |
| import { vscode } from "../vscode-api"; | ||
|
|
||
| const unhandledErrorListener = (event: ErrorEvent) => { | ||
| vscode.postMessage({ |
There was a problem hiding this comment.
Does the stack checking you implemented earlier to avoid printing stack traces for not-our-code also work for the webview? This is something to verify at least.
There was a problem hiding this comment.
I'll try to do a test with another extension to make sure, but I'm currently pretty confident that webviews are isolated and so the errors from one are not reported in another. I say this because the various webviews in our application are not interfering with each other. We get errors from a webview if and only if a listener has been registered in that particular view, and having another view open with a listener registered will not make it report errors from other open views.
|
I've pushed some new changes which include code for remembering which errors we're seen previously and avoiding reporting telemetry for them again. Please let me know what you think about the approach or remember previous errors. I haven't seen a better way of avoiding the duplicate reporting, but I'm very open to other options. I've done some investigation of using an error boundary, and I found that it did indeed stop the duplicate error reporting for errors during rendering but not for errors from event handlers. This is as we thought it would be, but it's nice to confirm it. I think error boundaries can be considered separately from this PR so I've opened an issue to track them. |
| }; | ||
|
|
||
| const unhandledRejectionListener = (event: PromiseRejectionEvent) => { | ||
| if (shouldReportError(event.reason)) { |
There was a problem hiding this comment.
I'm not 100% sure but I think the problem of duplicate errors only happens for thrown errors and not rejected promises. So this check here may be unnecessary, but it also shouldn't hurt so I decided to keep it in. I'm not really sure about this though and I'm happy to remove it.
koesie10
left a comment
koesie10
left a comment
There was a problem hiding this comment.
I think this approach makes sense, and if we can't filter out duplicate errors for event handlers anyway, then I think this behaviour is exactly what we need.
I have a few minor suggestions, but nothing blocking.
| } | ||
| }; | ||
|
|
||
| const unhandledRejectionListener = (event: PromiseRejectionEvent) => { |
There was a problem hiding this comment.
Should we be making a distinction between the unhandled errors and unhandled rejections? As far as I am aware, unhandled rejections don't cause any user-visible behaviour (except perhaps some data not loading), but unhandled errors might cause the complete view to disappear. It would be useful to make that distinction in the telemetry.
There was a problem hiding this comment.
For now I'm going to leave them combined. I don't think it's quite as well defined as saying unhandled errors are always more important. Missing or incorrect data is also bad, and maybe worse if the user don't know the data they're looking at is incorrect. Unhandled errors can also happen outside of rendering.
Also, we can certainly split these up in the future, and doing it after this PR will be fine. The telemetry is quite schema-less and we only keep it for a short period, so changing the format is quite cheap to do.
|
I've done some more tests and this seems to be working correctly, so I think this is good to merge. |
3 participants
Listens for unhandled errors and rejected promises in the webviews. We then send a message with info on the error so we can display the error and potentially log telemetry on it.
There's a few points worth noting here:
Errorobject in a message is problematic. It just doesn't seem to work. And it's not like it just loses the prototype bits but keeps themessageandstackfields. Everything gets a bit messed up and you end up with an empty object on the other end. To work around this I've made it send over an error-like object in the message, and I've made the logging and telemetry code handle this type just like it doesErrorobjects.Checklist
ready-for-doc-reviewlabel there.