Ensure "pause on uncaught" exceptions pauses in correct location.

[rwjblue]

Prior to this change (since 90f948) when an error occured after initial append (during the updating VM lifetime) using the dev tools feature for "pause on uncaught exceptions" would never pause execution in userland code, it would always pause at the location that the UpdatingVM was re-throwing the error.

This change restores the proper developer ergonomics of "pause on uncaught exceptions" while still preserving the original intent of the changes that were landed in this area for 90f948 (preventing invocation of resetTracking when "all is well").

[pzuraq]

@rwjblue can we wrap this entire try/finally in DEBUG? The original motivation was to avoid a depot which the try/finally was causing

[rwjblue]

The original motivation was to avoid a depot which the try/finally was causing

I think this is incorrect. The try/finally was not causing the deopts, the code in the finally was causing the deopts. That is only going to be ran when an error happens and shouldn't be deopting in prod.

can we wrap this entire try/finally in DEBUG?

I'd prefer not to duplicate the entire block for prod/debug (largely because we don't actually test prod), or do you mean to avoid the finally block contents in debug builds?

[pzuraq]

Another thing I just noticed, this updates the try/catch in the updating VM, we also need to update the try/catch in the append VM (which is where the deopt was originally occurring).

https://github.com/glimmerjs/glimmer-vm/blob/master/packages/@glimmer/runtime/lib/vm/append.ts#L559

I'd prefer not to duplicate the entire block for prod/debug (largely because we don't actually test prod), or do you mean to avoid the finally block contents in debug builds?

It'd probably be enough to create a separate method which does the wrapping:

execute() {
  if (DEBUG) {
    try {
      this._execute();
    } finally {
      // ...
    }
  } else {
    this._execute();
  }
}

This way we just avoid any funkiness with try/finally at all potentially. We could also verify that this new strategy does not deopt, it'll just take a while, and while I agree with your assessment, I don't actually understand why it was deopting in the first place still. None of the conditions should ever have been checked unless an error was thrown, and errors shouldn't occur at all in prod, yet it was still happening.

We didn't see a major perf win from fixing this specific deopt in the end, so if you think it would make more sense to just not wrap in DEBUG and do this strategy I think it's probably fine. We'll have to rework this anyways if we want to ever actually handle errors during render.

[rwjblue]

Thanks for the pointers there! Just updated, mind taking another look?