-
Notifications
You must be signed in to change notification settings - Fork 167
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
Added try catch for .caller, breaks in safari and maybe future browsers #70
base: master
Are you sure you want to change the base?
Added try catch for .caller, breaks in safari and maybe future browsers #70
Conversation
tracekit.js
Outdated
} | ||
} | ||
catch (error) { | ||
console.log(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove, we try and never log to console out if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, would re throwing the error be the preferred way to log the error? The concern with re throwing it is we are using this with Raygun4js and the actual error thrown causes another function not to return. Concern is this error failing silently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure, apart of me thinks that error libraries shouldn't ever throw an error and handle it. I haven't see this error thrown in the exceptionless client looking over thousands of error reports (and I primarily use strict mode and safari). If you can submit the stack trace I can help see if there is any other fix for this.
Thanks for the pr, I left a single comment. Have you come across any other strict issues? I haven't seen any myself after my last round of fixes with strict mode. Do you have the exact error that's thrown? I'd love to see the stack trace, the inner condition of the if should check to ensure caller is defined. I'm wondering if we should create a small function helper that try to get the caller of a function, which would simplify this change quite a bit and would probably be used in other places too |
After some further thought and discussion with others, I've removed the console.log . The way the error itself was generated was from a Raygun call. I'm still working on reproducing from outside our setup (which involves some iframes and such) in the normal mobile safari, I'll post some more information soon. |
hmm I haven't seen that, perhaps we should have a method like this: function getCaller(curr) {
try { return curr.callee } catch(ex) {}
} |
@Diophontine what's the status of this pr? Perhaps we should wrap it in a try catch as perhaps the last comment. |
3 similar comments
@Diophontine what's the status of this pr? Perhaps we should wrap it in a try catch as perhaps the last comment. |
@Diophontine what's the status of this pr? Perhaps we should wrap it in a try catch as perhaps the last comment. |
@Diophontine what's the status of this pr? Perhaps we should wrap it in a try catch as perhaps the last comment. |
Sorry I haven't looked at the code for many years, not sure if it's still relevant |
Line 1075 causes issues on our Safari instance when it cannot find the caller. As a result the computeStackTraceByWalkingCallerChain function returns with no message, even if a a valid stack trace had previously existed.
Seems like .caller is also obsolete, so until the function is rewritten, it would be worth try catching the errors.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/arguments/caller
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode/Transitioning_to_strict_mode