Fix propagation of options in Raven.context and Raven.wrap.

[russelldavis]

Options passed to Raven.context or Raven.wrap weren't getting propagated (there are unit tests for that, but they only test that the options get passed to TraceKit.report). This means that options like 'tags' or 'extra' never actually got passed on to Sentry.

In addition, after calling Raven.captureException, any further calls to that function within two seconds would reuse the options from the first call.

The root of the issue was in TraceKit:

• Parameters passed to TraceKit.report() didn't get passed to notification
  handlers (unless the exception was swallowed and window.onerror didn't get
  called).
• Multiple calls to TraceKit.report() within a two second window would all reuse
  the options from the first call
• Notification handlers for TraceKit.report() didn't get called at all when
  TraceKit.collectWindowErrors was set to false (unless the exception was
  swallowed, window.onerror didn't get called, and a second call to
  TraceKit.report() was made).

I submitted a pull request to fix this directly in TraceKit, but sadly, the entire project seems to have been abandoned.

[mattrobenolt]

Yeah, my tracekit is pretty different from upstream anyways. :) There was an issue open for this somewhere. One second.

[mattrobenolt]

@russelldavis Would this fix #133 ?

[russelldavis]

Yep, this should fix #133

[mattrobenolt]

Sweet. Lemme try this out locally.

[mattrobenolt]

What's an easy way to reproduce the issue? When doing a normal Raven.captureException(err, {tags: {foo: 'bar'}}) for example, the tags are merged in properly when sent.

[russelldavis]

It only breaks when the exception gets propagated to window.onerror. So, try adding throw err after your call to captureException. Or add options to Raven.context or Raven.wrap, e.g.

Raven.context({tags: {foo: 'bar'}}, function() {
    throw new Error('foo');
});

[russelldavis]

(In addition to the above, it would break when calling Raven.captureException twice within 2 seconds, as in #133).

[mattrobenolt]

Still not able to reproduce. :/ Mind adding a breaking example into the examples/index.html file? Doing what you're saying works on master for me.

[russelldavis]

Doh... yeah, lemme find a breaking example.

[mattrobenolt]

Thanks a lot. :) I appreciate the effort. I like to be able to reproduce the bug in question before I blindly merge shit in so I can understand the context.

[russelldavis]

No prob, I do the same. It's the only sane way to do it. (:

So, I was able to reproduce using the Raven.context example I posted above. I committed it to examples/index.html. When clicking the 'test options' button in my branch and inspecting the resulting request to sentry, it correctly has "tags":{"foo":"bar"} in the query string. When doing that in the master branch, it does not. Are you seeing something different?

[mattrobenolt]

Yeah, that works just fine in Chrome 33.0.1750.70 beta on master. Which browser you testing this in?

[mattrobenolt]

Ah, just tried it in Firefox and it didn't work!

[mattrobenolt]

Ok, awesome, and your patch fixes it for FF. No idea what's happening on the Chrome side. Probably hitting the path of the nicer window.onerror, I'm assuming.

[mattrobenolt]

Those tests make the test suite 8s now though. :( 😢

[russelldavis]

Ah, I think I know what was going on -- newer versions of Chrome (must be >= 33, since I'm on 32 and seeing the old behavior) now pass the exception to window.onerror, which as of 8b52364 takes a different code path, which doesn't trigger the bug.

Looking at that code though, there may be a separate bug there, since that code isn't clearing lastException and related vars. I'll take a closer look and do some tests to be sure.

[russelldavis]

Yeah, I'm also sad about the time to run the tests. It's an unfortunate consequence of TraceKit having to wait two seconds for report to run it's full course. I suppose we could make that time configurable and have the tests set it to a lower number.

[mattrobenolt]

Not super concerning. :) Some day, TraceKit will be gone entirely. It's on my 2014 roadmap.

[russelldavis]

Cool. Speaking of Tracekit, do you happen to know any of the maintainers and/or what's going on with that repo? I'm wondering if we should open an issue there to let others know that it's been abandoned. Not sure what the proper etiquette is around that, but the issues and pull requests are piling up, with no responses whatsoever.

[mattrobenolt]

No idea. I talked with the one dood pretty regularly for a while, and I disagreed with the direction he wanted to take it and started on a new project that didn't go anywhere.

I've started this project a while ago, just haven't put much time into it: https://github.com/mattrobenolt/callsite-shim

My goal is to finish that, get full browser tests, and port Raven to using that. I'd be happy to collaborate if you're interested.

[mattrobenolt]

I also troll in #sentry on Freenode if you're interested in chatting about things.

[russelldavis]

Awesome. I don't have a ton of free time, but I'd be happy to help a bit when I can. Will look for you in #sentry.

[mattrobenolt]

Neither do I, hence why it hasn't been touched in a while. :)