In D6959#207073, @ashoat wrote:

Open to it if there is some benefit over approach in D6958, but would like to see a discussion of tradeoffs.

In summary:

1. Your approach refactors the class so that we can query SecureStore from main thread that CommCoreModule runs on. It appears that this thread is automatically attached to JVM so this query succeeds.
2. My approach leaves the class untouched, and temporarily attaches cryptoThread (which is not attached to JVM by default) to JVM so that we can query SecureStore from this thread without causing a crash.

There can be a couple of advantages of choosing 2:

• Querying SecureStore happens in NotificationsCryptoModule code we don't have to repeat code to query SecureStore everywhere this class is used. This class will be used in different places in the code.
• Querying SecureStore happens on different thread than CommCoreModule runs on so CommCoreModule is not blocked on disk operations.
• If we hypothetically have tu use NotificationsCryptoModule API in place where we don't have access to main CommCoreModule thread, we will need to code to attach to JVM anyway even if we choose 1.

So in summary I think 1. allows for more flexibility regarding which thread is used and prevents some repeated code. Additionally if we ever run NotificationsCryptoModule totally outside of CommCoreModule code we will have to use some sort of variation of 2, where we directly attach the thread to JVM using JNI API.

Now that I might have more time to land this differential (assuming it will be accepted based on reasoning above), I would actually refactor it so that attaching to JVM thread is implemented in NotificationsCryptoModule class. It shouldn't happen in CommCoreModule code. I put it there initially since I was in a rush to create this diff.

Separately, can you rebase on master and potentially include a revert of D6958?

Sure, I will do so before any changes are made to this differential.

In D6959#207341, @ashoat wrote:

Okay, based on that it sounds like your solution is probably better than mine.

2. Let's try to do it without macros if possible (it doesn't seem like we've needed them before, but maybe I'm wrong)

There was a period when we were using macros for this purpose: https://phab.comm.dev/D6416. Now it is refactored not to use macros at the cost of implementing this in Android specific directory. Basically if we want to share a C++, which could potentially cause JNI error, between platforms we have to use macros. If we don't want to use macros we have to make sure that methods called in common C++ that are implemented in Android specific directory are wrapped with NativeAndroidAccessible. In this particular case it is the get method of CommSecureStore that has to be wrapped with NativeAndroidAccessible mirroring what has already been done for set method. I talked to @kamil who wrapped set method with NativeAndroidAccessible. He agreed that similar change should have been done for get.

In D6959#207434, @ashoat wrote:

I considered a solution like this, but I was worried that we would need some synchronization. @marcin, can you share some resources that show that NativeAndroidAccessProvider::runTask is "blocking", ie. it guarantees that the lambda passed to it will finish executing before it returns?

(Passing back with question)

1. NativeAndroidAccessible calls facebook::jni::ThreadScope::WithClassLoader, which is implemented here: https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/Environment.cpp#L374. It instantiates ThreadScope then calls facebook::jni::JThreadScopeSupport::runStdFunction with our lambda as argument.
2. The former calls runStdFunction of java class ThreadScopeSupport: https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/Environment.cpp#L348,
3. This java class calls JThreadScopeSupport C++ class again, by calling its runStdFunctionImpl method: https://github.com/facebookincubator/fbjni/blob/main/java/com/facebook/jni/ThreadScopeSupport.java#L31
4. This method simply invokes the lambda: https://github.com/facebookincubator/fbjni/blob/main/cxx/fbjni/detail/Environment.cpp#L354

So basically how facebook::jni::ThreadScope::WithClassLoader works is that it firstly instantiates ThreadScope which attaches thread to JVM. Then it invokes our lambda by firstly passing it to Java and back to C++ from Java. The reason for this hack-like solution is explained in their comment https://github.com/facebookincubator/fbjni/blob/main/java/com/facebook/jni/ThreadScopeSupport.java#L28:

// This is just used for ThreadScope::withClassLoader to have a java function
// in the stack so that jni has access to the correct classloader.

There are no asynchronous calls in the process. Thread is attached to JVM (it happens in ThreadScope constructor) and then lambda is synchronously passed C++ -> Java -> C++ and finally invoked.