Fix CSP bug in iOS9

[bengourley]

Goal

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.

Changeset

Changed

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().

Tests

A test that would fail for this browser was added to the end-to-end tests. However, BrowserStack does not support any iOS9 devices via their webdriver protocol, which is what we use. The oldest version available is iOS10.3, which does not exhibit this behaviour.

It is possible to verify locally with an iOS 9 simulator:

• npm run build
• run the tests suite in any browser to ensure the dependencies are loaded BROWSER=chrome_61 bundle exec maze-runner features/csp.feature
• serve the test fixtures PORT=9020 ruby features/lib/server.rb
• navigate to http://localhost:9020/csp/script/a.html?ENDPOINT=/abc
• after 5 seconds the text PENDING should change. If it changes to DONE the test passes, ERROR then the test fails

Here is a screenshot of it having run in my simulator:

Discussion

Alternative Approaches

We could consider switch to SauceLabs, where you can use older iOS devices. This would require a lot more work.

Linked issues

Fixes #357.

Review

For the submitter, initial self-review:

• Commented on code changes inline explain the reasoning behind the approach
• Reviewed the test cases added for completeness and possible points for discussion
• A changelog entry was added for the goal of this pull request
• Check the scope of the changeset - is everything in the diff required for the pull request?
• This pull request is ready for:
  • Initial review of the intended approach, not yet feature complete
  • Structural review of the classes, functions, and properties modified
  • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

• Consistency across platforms for structures or concepts added or modified
• Consistency between the changeset and the goal stated above
• Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
• Usage friction - is the proposed change in usage cumbersome or complicated?
• Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
• Concurrency concerns - if components are accessed asynchronously, what issues will arise
• Thoroughness of added tests and any missing edge cases
• Idiomatic use of the language

[bengourley]

This adds a way for the test fixture app to communicate that it is complete, but that an error happened.

[fractalwrench]

This looks like a good changeset (and has a nice detailed description). I'm happy to approve this with the provision that an entry is added to the changelog documenting the fix.

[kattrali]

Same as the above - looks good though needs a changelog entry.

[bengourley]

Thanks! Yeah until we arrange some time to rejig the automated release process it still prompts for the changelog entry at the time of the release.