Page MenuHomePhabricator

Run everuthing regarding NotificationsCryptoModule with native Android Accessible
ClosedPublic

Authored by marcin on Mar 6 2023, 12:47 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 21 2024, 5:50 AM
Unknown Object (File)
Feb 21 2024, 4:03 AM
Unknown Object (File)
Feb 21 2024, 4:03 AM
Unknown Object (File)
Feb 21 2024, 4:03 AM
Unknown Object (File)
Feb 21 2024, 4:03 AM
Unknown Object (File)
Feb 21 2024, 4:03 AM
Unknown Object (File)
Feb 20 2024, 11:31 PM
Unknown Object (File)
Feb 20 2024, 11:30 PM
Subscribers

Details

Summary

Calling NotificationsCryptoModule API directly in CommCoreModule results in a JNI unreachable crash. This diff provides a dirty but instant fix

Test Plan

Build android app. Ensure it is possible to log-in. Repeat for iOS

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin requested review of this revision.Mar 6 2023, 1:03 PM
ashoat requested changes to this revision.Mar 6 2023, 4:56 PM

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

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

This revision now requires changes to proceed.Mar 6 2023, 4:56 PM

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.

ashoat requested changes to this revision.Mar 7 2023, 5:51 AM

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

  1. Let's try and do it without touching CommCoreModule if possible
  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)
  3. Let's add parent diff to this diff that reverts my earlier solution
This revision now requires changes to proceed.Mar 7 2023, 5:51 AM

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

  1. 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.

Attach thread to JVM when calling CommSecureStore on Android to avoid JNI crash

ashoat requested changes to this revision.Mar 7 2023, 8:07 AM

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)

This revision now requires changes to proceed.Mar 7 2023, 8:07 AM

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.

marcin requested review of this revision.Mar 7 2023, 8:29 AM

Okay, great – let's ship it!

This revision is now accepted and ready to land.Mar 7 2023, 8:34 AM