EXC_BAD_ACCESS when releasing lambda

[brettwillis]

I am using Firebase Firestore on iOS from Kotlin native. I pass a Kotlin lambda to -addSnapshotListener:, and the Firestore library asynchronously calls that lambda on the main thread.

This all works fine.

Now, when the "snapshot listener" is removed, EXC_BAD_ACCESS error happens. This seems to happen when the lambda is released on a worker thread.

This is the call stack:

How can I figure out how to fix this?

[olonho]

What if you freeze() the lambda before the call, as described in https://github.com/JetBrains/kotlin-native/blob/master/CONCURRENCY.md#object-transfer-and-freezing?

[ildarsharafutdinov]

I seem to have faced the same issue: delegate returns a lambda, which is supposed to be called in another thread by a framework. I do get EXC_BAD_ACCESS as well. freeze() doesn't help either.

Here is obj-c sample https://github.com/TextureGroup/Texture/blob/master/examples/ASViewController/Sample/ViewController.m:

- (ASCellNodeBlock)tableNode:(ASTableNode *)tableNode nodeBlockForRowAtIndexPath:(NSIndexPath *)indexPath
{
    // As the block is executed on a background thread we need to cache the image category string outside
    NSString *imageCategory = self.imageCategories[indexPath.row];
    return ^{
        ASTextCellNode *textCellNode = [ASTextCellNode new];
        textCellNode.text = [imageCategory capitalizedString];
        return textCellNode;
    };
}

It's ported to kotlin native as follows:

    override fun tableView(tableView: ASTableView, nodeBlockForRowAtIndexPath: NSIndexPath): ASCellNodeBlock? {
        val category = imageCategories[nodeBlockForRowAtIndexPath.row.toInt()]

        NSLog("my nodeBlockForRowAtIndexPath: $category")
        return {
            val cell = ASTextCellNode()
            cell.text = category
            cell
        }.freeze()
    }

[olonho]

Could you please provide self-contained reproducer?

[ildarsharafutdinov]

I'm using this commit

git show
commit 49eee47d7e7137df3c7fb52a91e8db6561491e28 (HEAD -> master, origin/master, origin/HEAD)
Author: Svyatoslav Scherbina <[email protected]>
Date:   Tue Sep 4 18:47:06 2018 +0300

My working dir is kotlin-native/samples/uikit - I'm using your sample.

Here is git diff: patch.diff.zip

Additionally def directory is supposed to contain the following symlinks so that cinterop could find headers(any recomendations on how to do that better are welcome):

def/AsyncDisplayKit -> ../Carthage/Build/iOS/AsyncDisplayKit.framework/Headers
def/PINCache -> ../Carthage/Build/iOS/PINCache.framework/Headers
def/PINOperation -> ../Carthage/Build/iOS/PINOperation.framework/Headers
def/PINRemoteImage -> ../Carthage/Build/iOS/PINRemoteImage.framework/Headers

Build steps:

1. setup carthage https://github.com/Carthage/Carthage
2. resolve & build dependencies carthage update --platform ios
3. apply patch & create symlinks
4. open xcodeproj & run

In order to reproduce the crash comment Works section & uncomment Doesn't work

    // Works
    override fun tableView(tableView: ASTableView, nodeForRowAtIndexPath: NSIndexPath) = ASTextCellNode().apply { 
        text = imageCategories[nodeForRowAtIndexPath.row.toInt()]
    }

    // Doesn't work
    // override fun tableView(tableView: ASTableView, nodeBlockForRowAtIndexPath: NSIndexPath): ASCellNodeBlock? {
    //     val category = imageCategories[nodeBlockForRowAtIndexPath.row.toInt()]
    //
    //     NSLog("my nodeBlockForRowAtIndexPath: $category")
    //     return {
    //         val cell = ASTextCellNode()
    //         cell.text = category
    //         cell
    //     }.freeze()
    // }

[SvyatoslavScherbina]

Have you called initRuntimeIfNeeded()?
Like in e.g.

kotlin-native/samples/objc/src/main/kotlin/Window.kt

Line 38 in 71405c3

initRuntimeIfNeeded()

[ildarsharafutdinov]

No.

I've just tried to call initRuntimeIfNeeded as follows and it seems to be working.

    override fun tableView(tableView: ASTableView, nodeBlockForRowAtIndexPath: NSIndexPath): ASCellNodeBlock? {
        val category = imageCategories[nodeBlockForRowAtIndexPath.row.toInt()]
    
        NSLog("my nodeBlockForRowAtIndexPath: $category")
        return {
            initRuntimeIfNeeded()
            val cell = ASTextCellNode()
            cell.text = category
            cell
        }.freeze()
    }

Is it a supposed way to use initRuntimeIfNeeded & freeze?

[SvyatoslavScherbina]

Is it a supposed way to use initRuntimeIfNeeded & freeze?

Yes.

[olonho]

So this way it works for you?

[ildarsharafutdinov]

Yes.

[olonho]

@brettwillis what about you, could you please elaborate a bit, if freezing help you?

[brettwillis]

@olonho thank you for your support, I'll give freeze() a try, but I won't get around to it until tomorrow.

In the meantime may I ask, will freeze() not also freeze every variable that is captured by the lambda, and then every object reachable from those variables?

It is deep, so if an object has a pointer to other objects - transitive closure of such objects will be frozen.

If so, I fear that this would effectively freeze the entire business logic of my application.

[olonho]

So could you only pass to the worker state which is either immutable or shall belong to worker only? Otherwise, your program has race, which is essentially detected for you by K/N.

[brettwillis]

Well I use the "listener" lambda to emit items into a BroadcastChannel which then feeds the entire program. Can a channel be frozen? And perhaps it will also freeze all the channels that are subscribed to the BroadcastChannel at the time the lambda is frozen?

I guess we'll find out when I can give it a try :)

[olonho]

Note that one could use AtomicReference to pass immutable state between workers in the way similar to channel.

[brettwillis]

Hi @olonho, so I gave it a try. As I suspected, when I freeze the lambda, I get invalid mutability errors all the way down the chain of my program.

Curiously the first of such errors is on an instance of AtomicRef quite far down the chain, which by your suggestion, I would have thought would have been able to be frozen...?

I have worked around this by explicitly retaining a reference to the lambda in my code, and ensuring it gets released only after the Firestore backend has released its reference. This way I ensure it gets released on the main thread. However this required a hack to the Firestore backend code (because it releases the reference asynchronously), so not a great solution at all.

This use case would seem to me to be quite common (that is, converting callbacks to Channels or Deferreds), and we don’t always have control of which threads the library uses.

How would you consider solving this in Kotlin/Native? Would it be solved when multi threading is properly implemented?

[olonho]

Please provide reproducer, so we could give meaningful feedback. There's no AtomicRef in Kotlin/Native, only AtomicReference, and its update operation can work on frozen values, for example

import kotlin.native.concurrent.*
fun main() {
  val ref = AtomicReference<String>("one")
  ref.freeze()
  ref.value = "two"
  println(ref.value)
}

will print two. Generally, you just need to realize expected concurrency behavior of your application, and rework properly.

Not sure what do you mean by "when multi threading is properly implemented" - it is already there, and not much changes planned here. Just concurrency in Kotlin/Native provides automated concurrency correctness analysis, so hidden bugs and races are explicit.

[brettwillis]

Not sure what do you mean by "when multi threading is properly implemented"

I was referring to kotlinx.coroutines issue 462. I gather from what you are saying that the issue is not with Kotlin/Native, but incomplete coroutines implementation on the Kotlin/Native platform, am I correct?

There's no AtomicRef in Kotlin/Native, only AtomicReference

So the kotlinx.atomicfu implementation of AtomicRef doesn’t work once frozen, I must use AtomicReference. Is there any plan to unify those classes?

To implement my use-case using AtomicReference, do you suggest that I use AtomicReference to pass the channel into the lambda, or actually use AtomicReference instead of the channel? If it’s the latter, could you provide an example of how to receive from the AtomicReference in an asynchronous/suspending way?

[irgaly]

I'm using Kotlin 1.3.0-rc-57, Kotlin/Native 0.9.2-4008.
My app uses NSURLSession with kotlin, and few users are crashed.

It seems the crash appears in releasing kotlin lambda.

The crashing devices are iOS 10.3.3, 11.4.0, 11.4.1, 12.0.0 ... so I think it is not specific iOS version, but I can't reproduce it in my development devices.

My code is below:

actual fun request(callback: (MyResult) -> Unit) {
    val request = NSMutableURLRequest.requestWithURL(...)
    ...
    NSURLSession.sharedSession.dataTaskWithRequest(request, { data: NSData?, response: NSURLResponse?, error: NSError? ->
        try {
            kotlin.native.initRuntimeIfNeeded()
            ...
            if (response is NSHTTPURLResponse) {
                val body = data?.let { ... } ?: throw Throwable("...")
                val result = MyResult(...)
                callback(result)
            } else {
            ...
            }
        } catch (e: Throwable) {
            ...
        }
    }.freeze() ).resume()
}

[SvyatoslavScherbina]

@irgaly the exact circumstances of this crash aren't clear without a reproducer, however I have an assumption and the next release is likely to include the fix that may help.

[SvyatoslavScherbina]

@irgaly Have you tried Kotlin 1.3.0?

[irgaly]

I have updated to Kotlin 1.3.0-rc-202 / Kotlin Native 1.0.0 last week.
My app will be released in few days. After that, I will report the result here 👍

[SvyatoslavScherbina]

This version includes the fix. Do you still encounter the same issue with EXC_BAD_ACCESS as before?

[brettwillis]

I have updated to Kotlin 1.3.0, reverted the workaround which averted the crash, and unfortunately I was still able to reproduce the problem.

I know you're still waiting for a reproducer but my first priority is heading towards a first release and my workaround works for now.

[SvyatoslavScherbina]

@brettwillis as far as I remember, in your case a crash happens when a lambda is released on worker thread. Do I understand correctly that the lambda is not frozen?

[brettwillis]

Correct. The lambda is released on a worker thread (controlled by the Firebase 3rd party library), and it is not frozen because that also freezes the captured variables (such as the Channel it feeds values into, and all sorts of other objects downstream).

The workaround is to modify the Firebase library to ensure the lambda is released on the main thread.

[SvyatoslavScherbina]

There is an alternative workaround which doesn't involve Firebase library modification.
You can create special "shared" lambda using something like this:

val lambda = { ... } // Your lambda
val lambdaStableRef = StableRef.create(lambda)
val lambdaPtr =  lambdaStableRef.asCPointer()
val sharedLambda = {
    val lambda = lambdaPtr.asStableRef<() -> Unit>().get()
    lambda()
}.freeze()

But in this case lambdaStableRef should eventually be disposed.

[brettwillis]

Thank you for this suggestion. Because the Firebase backend removes the "listener" (lambda) asynchronously, I can't actually know when the backend has finished.

Therefore when I remove the listener and dispose lambdaStableRef, I fear that I am creating a race condition where lambdaStableRef is disposed by the Firebase backend may still try to call sharedLambda.

Is there any way to check inside sharedLambda if lambdaPtr has been disposed or not? Even so, without synchronisation/lock/mutex there would still be a race condition...

[SvyatoslavScherbina]

Is there any way to check inside sharedLambda if lambdaPtr has been disposed or not?

No.

Even so, without synchronisation/lock/mutex there would still be a race condition...

If you dispose lambdaPtr on main thread, then I guess it will be enough to have a simple boolean flag indicating whether it is already disposed or not, since sharedLambda invocation also happens on main thread.

[irgaly]

@SvyatoslavScherbina
The crash issue ( #2052 (comment) ) has not happened with Kotlin 1.3.0 / Kotlin Native 1.0.1 ( and no changes in my lambda code ). It is no crash report from our users in a week, so I think that bug is fixed.
Thank you 😄

[SvyatoslavScherbina]

@irgaly Thank you for the feedback 😄

[yusuf-a]

I think we are also this same issue, where a lambda inside our K/N framework is being deallocated on another thread and it's something we have no control over. We also can't use freeze as we do require some mutation. We are using K/N 1.3.30.

[dasdranagon]

We were facing a similar problem with Firebase.
We call a completion block created by K/N inside the listener's completion handler:

class FirebaseFactory: Factory {
    let db = Firestore.firestore()
    func listener(block: @escaping (Any) -> KotlinUnit) -> Removable {
        return FirebaseListener(db: db, success: block)
    }
}

class FirebaseListener: Removable {
    var listener: ListenerRegistration?
    init(db: Firestore, success: @escaping (Any) -> KotlinUnit) {
        listener = db.collection("users").addSnapshotListener { snapshot, error in
            if let snapshot = snapshot {
                _ = success(snapshot.documents)
            }
        }
    }

    func remove() {
        listener?.remove()
    }
}

If we remove the listener, the application crashes:

We can prevent a crash by releasing completion block (created by K/N) in a few seconds after remove was called.
This crash can be reproduced without Firebase just by releasing completion block from a non-main thread:

Freezing the completion block doesn't solve this issue.

You can reproduce crash using: the sample project
We are using K/N 1.3.31

[SvyatoslavScherbina]

@dasdranagon

> Freezing the completion block doesn't solve this issue.

What exactly did you try to freeze? Could you add an example of doing this to your sample project?

Yes, I can confirm that freezing doesn't help here.

We are considering improving the situation with releasing objects from other threads.

[dasdranagon]

@SvyatoslavScherbina
Thanks! I've added freezing to the sample project in order to be able to reproduce this issue.

[SvyatoslavScherbina]

Fix will be shipped with Kotlin 1.3.60.