Skip to content

Conversation

@cmelchior
Copy link
Contributor

This PR fixes an issue where crashes inside background change listeners would not get correctly detected, thus crashing the JavaBindingContext in unrelated ways when accessing the JNIEnv.

Sending notifications on background looper threads involves a number of callbacks from and to JNI and each of these steps needs a guard against this. It isn't possible to test for each specific callback, so I just added a test for the most important one: Exceptions inside the actual callback.

This case was detected when using coroutines with a custom looper Dispatcher. Canceling the scope would result in a CoroutineException inside the callback, but instead of the "real" exception people would see JNI DETECTED ERROR IN APPLICATION: JNI NewLocalRef called with pending exception kotlinx.coroutines.JobCancellationException: Job was cancelled in unrelated code.

… thus causing native crashes when accessing the JNIEnv.

Refactor RealmNotifier tests to use the BlockingLooperThread class.
Copy link
Contributor

@rorbech rorbech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is adding these ExceptionChecks sufficient? Shouldn't it rather be handling this on backside of the actual call throwing the exception? But, I it is a big improvement and with tests verifying that it does the right thing for the notification callbacks I guess it LGTM.

@cmelchior
Copy link
Contributor Author

Is adding these ExceptionChecks sufficient? Shouldn't it rather be handling this on backside of the actual call throwing the exception?

The BindingContext is being called from C++ that doesn't understand Java exceptions. In this case, it has a flow where it calls back and forth 3-4 times. Right now there is no way to signal errors in this path, so we need to shortcut all the steps.

@cmelchior cmelchior requested a review from rorbech September 6, 2021 05:33
{
try {
auto& config = *reinterpret_cast<Realm::Config*>(j_native_config_ptr);
_impl::RealmCoordinator::get_coordinator(config)->create_session(config);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the session created now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is created automatically by Object Store when you open the Realm.

@cmelchior cmelchior merged commit c151300 into releases Sep 6, 2021
@cmelchior cmelchior deleted the cm/bug/notification-exceptions branch September 6, 2021 07:33
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants