Generate a correct error message when null is passed to notify() #400
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
And utilise it within window.onerror plugin.
Enables session tracking by default, and makes some requisite changes to ensure the relevant endpoints are properly configured. The options `endpoint` and `sessionEndpoint` are soft-deprecated, and they are replaced by the `endpoints: { notify, sessions }` object.
* test: Add a failing test for known stacktrace parsing bug URL paths with "@" in them incorrectly parse in Firefox/Safari due to a bug in the error-stack-parser module * fix: Upgrade to latest stacktracejs modules error-stack-parser contains a bugfix for the test in the previous commit stack-generator published a long-term dangling PR which we were depending on in a fork, so this just switches back to the main package * chore: Rebuild * test: Remove unnecessary assertion which is different for different browsers * test: Generalise assertion to cater for variation across browsers
* test: Add filename assertions for unhandled syntax errors
This commit adds a failing test for the stacktrace of syntax errors in chrome. The stacktrace
doesn't have any frames, yet it gets passed over to error-stack-parser which incorrectly adds the
error message as the file name.
* fix: Don't attempt to parse a stacktrace if it doesn't contain any frames
SyntaxErrors in Chrome have an error.stack property which is just `${err.name}: ${err.message}` and
no stack frames but because the err.stack value is truthy we attempted to parse it with
error-stack-parser. The result of this was that the filename proeprty of the stackframes was not
correct and caused issues with grouping. This change causes Report.getStacktrace(err) to generate a
backtrace instead and then the file/line/col properties get filled in with the values from
window.onerror.
* chore: Rebuild
In iOS9 Safari, a request that violates the Content Security Policy (CSP) rules causes a `DOMException` to be thrown. Since v4.7.0, bugsnag calls `startSession()` synchronously upon startup, which means that any thrown error in that tick of the event loop results in the libary (and anything else in that synchronous execution context) to be unusable. The behaviour of iOS Safari was fixed in version 10, but since we support 9, this needs a workaround. The xml-http-request transport mechanism already wrapped request.send() with a try/catch, but the error in this instance was coming from the request.open() call. The try/catch block was moved out out to include all of the synchronous code inside sendReport()/sendSession().
The badge key was out of date and referred to a CI run from a long time ago. The tests are now run in a different way such that the status isn't reported to BrowserStack anyway, so it's best just to remove this.
This is dropped by the UI and never shown as we can't render arrays in the breadcrumbs from what I can see.
BrowserStack appear to have removed emulators from their js testing product now, so the oldest version of iOS we can test on is 10.3.
It looks like BrowserStack removed iOS enirely from their js testing product, according to this page: https://www.browserstack.com/list-of-browsers-and-platforms?product=js_testing
Dont send stacktrace as breadcrumb metadata
…-types fix: Add missing properties to breadcrumb.d.ts
|
Hi. This PR was closed automatically when I switched out a new branch to |
bengourley
added a commit
that referenced
this pull request
Apr 4, 2019
The existing logic prevented "null" from being reported correctly. This cleans up the logic and makes sure it's all contained in the normaliseError() function. Fixes #400.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goal
Currently, calling
notify(null)incorrectly generates this error:Bugsnag usage error. notify() expected error/opts parameters, got unsupported object.The intent from the code appears to be that this would instead generate:
Bugsnag usage error. notify() expected error/opts parameters, got nothingThe issue is caused by
normaliseErrorcallingtypeof error, which isobjectfor null. So this code path was being hit: https://github.com/bugsnag/bugsnag-js/compare/master...bobjflong:BL/null?expand=1#diff-d038e8e69f5ea867f38281dc1f40df19R240There is existing null check code, but it wasn't being hit as a result.
Changeset
Added
Removed
Changed
I've added a null check to the last branch in
normaliseError.Tests
I've added a specific unit test for this case.
Discussion
Alternative Approaches
Outstanding Questions
Linked issues
Review
For the submitter, initial self-review:
For the pull request reviewer(s), this changeset has been reviewed for: