Skip to content

Fix onerror handling when the browser passes an exception arg. #190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 10, 2014

Conversation

russelldavis
Copy link

I haven't had (and may not have for a little while) time to test this much, but the basic idea is:

  • Need to check for lastExceptionStack first. If that exists, we always want to do processLastException, which will use the options that were originally passed via Raven.wrap or Raven.context, and clear the vars related to that so they don't get reused on the next call to TraceKit.report.
  • Just check ex rather than !isUndefined(ex) since browsers will sometimes pass null (this happens when running example/index.html -- it's this error) and in that case, it makes more sense to fall down to the final else block
  • Call notifyHandlers directly rather than report. The latter does a bunch of stuff we don't want/need here (including the 2000ms timeout thing). I think this means we can get rid of the rethrow arg on report as well (I think this was the only thing using it).

Would also be nice to add some tests that exercise the code path where onerror is called with the exception arg.

@russelldavis
Copy link
Author

Note that the current symptom of this is that in browsers where the exception arg is passed to window.onerror, Raven's handleStackInfo function gets called twice for every exception. I think this will normally result in two requests getting made to sentry, but haven't tested yet outside of example/index.html, and in that case, the second error never gets sent because the exception arg gets passed as null (see http://stackoverflow.com/questions/5913978/cryptic-script-error-reported-in-javascript-in-chrome-and-firefox). This ends up getting filtered out in processException due to message being null.

@russelldavis
Copy link
Author

(you'll probably want to set breakpoints in tracekit's onerror function to get a feel for what's happening here)

@mattrobenolt
Copy link
Contributor

Awesome, looks sane to me!

mattrobenolt added a commit that referenced this pull request Feb 10, 2014
Fix onerror handling when the browser passes an exception arg.
@mattrobenolt mattrobenolt merged commit edeedd2 into getsentry:master Feb 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants