Fix for javascript unhandled errors not being reported

[cpetzel]

Problem

On iOS, a majority of our unhandled javascript errors are not reaching our dashboard...

Reason

When the Bugsnag Javascript library tries to notify the Native SDK of the javascript error, that operation is NOT blocking. This means that in the very next line of code in the Bugsnag JS SDK, is to re-throw the Error, which will then terminate your app. If your error has a large number of stack frames (more than the Bugsnag Example App), then there is a chance that your error will NOT get reported to the dashboard because the Native SDK gets terminated from the re-thrown Error.

This is because the Bugsnag library does not wait for the internal native Bugsnag error reporting method to complete before it re-throws the exception. In the case of a large JS error, a few milliseconds later, React and CoreFoundation will simply re-throw the native error, which will cut the head off of the currently parsing/persisting javascript exception.

Steps to repro..
Create a complex call hierarchy for a javascript error.
Trigger this javascript error in a release/production build.
Observe that there is no report on the dashboard.

Changeset

The underlying problem is here.
On iOS, this previousHandler is called mere milliseconds after the original exception is still going across the bridge, or is currently being worked on by the native iOS SDK.
The previousHandler in this case is React/CoreFoundation which will terminate the process, which will not allow the Bugsnag SDK to fully process the unhandled javascript error.

The solution to this is to simply block/wait on ALL calls to notify.

I made the call to notify promise based (on BOTH platforms)

NativeClient.notify(payload).then(() => {
      //let the native SDK do its thing to persist the error
      setTimeout(() =>{
        if(postSendCallback){
          postSendCallback();
        }
      }, callbackDelay);
    });

We are going to maintain our fork until this gets resolved, but I imagine this is happening for many of your other customers...

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

[kattrali]

Thank you, @cpetzel, for the detailed write-up and patch. I'll need to spend some time going through it, as I have a few questions.

[cpetzel]

Happy to provide any info or answer any questions :)

[kattrali]

Thanks! I've poked around with this a bit and will make a few changes:

1. On Android, continue to use notifyBlocking as its guaranteed to delivery the report (rather than just write it to disk) before the system is torn down.
2. On iOS, there's a way to check that the report has finished writing to disk, so I can verify that it is complete before invoking the resolver.
3. Add a few integration tests to verify that this behavior does not degrade again.

These should handle a few edge cases and guarantee that no reports are lost due to payload processing time.

[cpetzel]

@kattrali Regarding your recent comment point # 1... Android and iOS now have a unified API. Under the hood, Android will always use the blocking API. This is now the same behavior that notifyBlocking was doing.

[kattrali]

Right, though if you call notify() from your own code (instead of from the exception handler), that should be a quick non-blocking operation. That's why there was logic to switch between the two.

[cpetzel]

I get that, but in the case of a quick notify call (non exception), the bugsnag JS SDK does not do anything with the result of that call (does not perform any action in the then callback of the promise).

If there was an action to take in the javascript SDK after a call to notify then you would be correct that we should not make that blocking... But because no action needs to be taken, it should matter if the native SDK is using the blocking API or not in this case.

In the case of an unhandled exception, that is when we do care about the callback/promise, so it should be blocking.

[kattrali]

Ah, I see what you're saying. You're right, it doesn't block the JS thread. My concern was that it would block the native thread called from JS, in case that needed to be reused or would block other JS-to-native operations.

[cpetzel]

Ah yes, your right. We don't want to hold up the native JS bridge execution thread if we don't have to (in the case of a normal handled call to notify).

It would be nice to keep the unified API cross platform. Perhaps we can change my existing notify to simply include a boolean? iOS can just ignore it?

[kattrali]

Yeah, that's what I went with! Extended this PR in #290.

[cpetzel]

Awesome! Thanks for fixing this in a timely manner :)

[cpetzel]

@kattrali

Regarding your points above...

1.) See my comment above in the code.
2.) Sounds good!
3.) Sounds great!