Living with a previously defined onerror-function

[gaiottino]

Hi,

I previously reported this to the Honeybadger issue list, but they informed me that the code in question was actually part of your project. So here goes...

I was working on getting Honeybadger integrated in our app and ran into an issue. It's a heavy client-side app and we have our own onerror-function defined. In this function we ignore quite a number of errors which occurs in various libraries (and browser extensions) which doesn't affect our site. When an error we can't recover from occurs we render a failure page.

This is where I found the issue. Notifications weren't being sent to Honeybadger. The reason is because Honeybadger dynamically adds elements to the document in order to perform a crossdomain post, but our failure-page-rendering removed everything from the body before it renders which removes the Honeybadger elements and thus nothing is sent.

The code in question is part of TraceKit and looks like this

notifyHandlers(stack, 'from window.onerror');

if (_oldOnerrorHandler) {
  return _oldOnerrorHandler.apply(this, arguments);
}

I believe there are two issues here. (1) You attempt to notify handlers before the old onerror handler has run (if it exists). (2) TraceKit does not respect the return of the old onerror method. Returning true prevents the firing of the default event handler (i.e. the error has been dealt with), and I feel that this should also be respected by TraceKit.

I'd like to see the code be changed to something similar to

if (_oldOnerrorHandler) {
  var errorHandled = _oldOnerrorHandler.apply(this, arguments);
  if (errorHandled) return true;
  notifyHandlers(stack, 'from window.onerror');
} else {
  notifyHandlers(stack, 'from window.onerror');
}

Now TraceKit notifies hanglers after the already defined error handling has been run, and only notifies handlers if the error hasn't already been dealt with.

[devinrhode2]

Hey @joshuap I just wrote this up quick, can you throw it in Honeybadger and make sure all basic functionality is still in tact?

@gaiottino, in your window.onerror functon, you'll have a 4th parameter from TraceKit that is a callback to notifyHandlers or not (see pull request) it'll be like:

window.onerror = function(a,b,c, notifyTraceKitHandlers) {
  if (???) {
    notifyTraceKitHandlers();
  }
};

Please fully test and let me know if this all works out for you guys in a few days.

For convenience, the fix: devinrhode2@e1c43c5

Last but not least, this needs to be documented before merging to master. cc @occ

[devinrhode2]

Perhaps a better api would be more like:

window.onerror = function(a,b,c) {
  ...
  this.notifyHandlers();
};

The this would be modified so no global pollution occurs.

Actually, since the return value of onerror really doesn't play a role in anything anywhere, you could just:

window.onerror = function(a,b,c) {
  return {notifyTraceKitHandlers: true};
  //or
};

I think the return {notifyTraceKitHandlers: true}; is actually the cleanest and best. Would you guys agree? Would you rather have this api instead?

The this.notifyHandlers api would depend on TraceKit calling it a certain way, however, if you stop using Honeybadger with TraceKit, the code breaks. The return {notifyTraceKitHandlers: true}; won't break your code if you remove these libraries. The same thing occurs with the API in the Pull Request I submitted, because you're depending on TraceKit to pass in that 4th parameter.

[devinrhode2]

@gaiottino and @joshuap, docs on new api here: https://github.com/devinrhode2/TraceKit/blob/patch-5/README.md#existing-windowonerror-function

Again, I just wrote this up quickly, so please grab the tracekit.js file here: https://github.com/devinrhode2/TraceKit/blob/patch-5/tracekit.js test it.

Let me know how this works for you guys.

[joshuap]

I added a branch with the new tracekit.js:

https://github.com/honeybadger-io/honeybadger-js/tree/devinrhode2-patch-5

Our tests are passing. :) I agree that it's best to keep the handler notification logic in tracekit.js. I'm personally not a big fan of using window.onerror for catching errors at all - we disable it by default, as I believe you guys also discourage it? That said, this new API seems like a nice way to filter out certain events from the parent function.

@gaiottino can you confirm that this fixes your issue of the dom being replaced after handlers are called?

Thanks for the quick turnaround @devinrhode2!

[gaiottino]

I'm currently away on holiday this week but will verify and update this thread first thing next week. Thanks for all your input :)

[gaiottino]

Just about to test this now. I don't understand why you chose a custom return value instead of relying on the existing definition which only return true or false.

https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers.onerror

With your new custom return value, users will have to modify existing onerror-functions to make them compatible with TraceKit/Honeybadger. It also means that the TraceKit onerror-function returns an invalid value when there's an existing function since you just forward the new custom return value.

I vote for relying on the existing return value-definition instead of introducing a new way to handle this.

[devinrhode2]

Do you have any suggestions on what the api would look like? I think the 4th parameter works well in this case.. this.notifyHandlers is interesting, but this is the global object here.. Will do some investigating

[gaiottino]

I think the API should just be true or false like it is defined in https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers.onerror

        if (_oldOnerrorHandler) {
            var _oldOnerrorReturn = _oldOnerrorHandler.apply(this, arguments);

            if (!_oldOnerrorReturn || _oldOnerrorReturn != true) {
                notifyHandlers(stack, 'from window.onerror');
            }

            return _oldOnerrorReturn; // preserve any potential return behavior of the old onerror function.
        } else {
            notifyHandlers(stack, 'from window.onerror');
        }

        return false;

[gaiottino]

Been thinking about this some more and realized that just relying on true/false isn't ideal. Returning true/false should be used as defined so that it's possible to prevent the firing of the default event handler and TraceKit should return this value as defined in the onerror-function.

But return {notifyTraceKitHandlers: true}; doesn't work since forwarding this would always result in a truthy return-value.

I do like this bit of code though

window.onerror = function(a,b,c, notifyTraceKitHandlers) {
  if (???) {
    notifyTraceKitHandlers();
  }
};

It would give the user complete control of when to notify e.g. Honeybadger but also tell the browser whether to fire the default handler using true or false as return-value.

[gaiottino]

@devinrhode2 any update/thoughts?

[devinrhode2]

Passing the function as a 4th arg is a better api, just have to find a
little time to code it up now :)
On Jul 29, 2013 5:03 AM, "Daniel Gaiottino" [email protected]
wrote:

@devinrhode2 https://github.com/devinrhode2 any update/thoughts?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/43#issuecomment-21715172
.

[devinrhode2]

After I wrote this, I realized that, really, the best API of all would be a simple function hanging off the TraceKit namespace. I'm working on re-doing a lot of stuff and should be able to include this easily.

For record, I'm leaving my internal monolog coming to this conclusion below. Warning: Please don't waste your time reading all this

Ok so now that Chrome introduced new arguments to onerror, I'm thinking the the this.notifyHandlers function would be the best api.

It would be implemented a little bit like this:

window.onerror = function(){
  if (this.notifyHandlers) {
    this.notifyHandlers();
  }
  console.log('Still access global properties via this? this.location.href:', this.location.href);
  return false; //always let the default event handler fire..
};

notifyHandlers = undefined;
function extendWindow() {
  //assert called with 'new'
  this.notifyHandlers = function(){console.log('notifyHandlers called');};
}
extendWindow.prototype = window;
console.log('yeah...', onerror.call(new extendWindow()));
console.log('notifyHandlers undefined?', notifyHandlers === undefined);

This creates the this.notifyHandlers without polluting global scope, however, this is no longer the global object, but it behaves like it. People could have errors trying to do this.createSomeGlobalVariable = ... but I can warn against this in the docs.

I personally prefer passing in the function as the last argument. Instead of

if (this.notifyHandlers) this.notifyHandlers();

You would do this:

arguments[arguments.length]();

Which is more straightforward but less readable. Perhaps the best of all is compensating for Chrome's new 5 argument onerror by always passing in the function as the 6th parameter.

window.onerror = function(message, lineNo, colNo, something, actualException, notifyTraceKitHandlers) {
};