[Blob] Release underlying resources when JS instance is GC'ed on Android

[janicduplessis]

Summary

Android followup for #24745. This adds a jsi object that removes blobs when it is gc'ed. We don't have many modules with native code on Android so I've added the native code directly in the blob package as a separate .so. I used a similar structure as the turbomodule package.

Changelog

[Android] [Fixed] - [Blob] Release underlying resources when JS instance is GC'ed on Android

Test Plan

Tested in RNTester by adding a breakpoint in the method that removes blobs and made sure it is triggered when making an http request with fetch.

[janicduplessis]

I added the buck config but I can't confirm it works, RNTester currently crashes on master with 05-08 16:54:56.521 20634 20664 E SoLoader: couldn't find DSO to load: libreactnativejni.so when the app starts.

[facebook-github-bot]

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[cpojer]

Let's try to land this.

[facebook-github-bot]

@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @janicduplessis in a4f7e17.

^{When will my fix make it into a release? | Upcoming Releases}

[cpojer]

Heads up: this is causing an insta-crash in one of our apps. I'll try to get more details but we have to unland this change for now. In case you have a hunch of what could cause it, feel free to send the same PR again with a fix.

[janicduplessis]

@cpojer Any change you can get me the crash log? I get a feeling it's related to fb internal setup.

[cpojer]

Unfortunately I don't have anything actionable that I can share :(

[janicduplessis]

@cpojer The crash happens in one specific fb app, not all apps right?

I can't tell for sure but most crashes I experienced when working on the feature was with JNI refs or failing to load the .so file, but those should lead to a clear stacktrace so might not be that.

https://github.com/facebook/react-native/pull/24767/files#diff-8d838f0df357a8f8620e924ba05c9815R16 maybe this could be null and cause an obscure crash later in c++.

What would be the best way test changes and try to move this forward?

[cpojer]

@janicduplessis I'll put it on my queue and ask somebody at FB to help out re-land this.

[makovkastar]

@janicduplessis I found the reason of the crash in one of the FB apps, Proguard renamed the BlobModule.remove(...) method, so that the C++ couldn't find it:

auto removeMethod = jni::findClassStatic(kBlobModuleJavaDescriptor)->getMethod<void(jstring)>("remove");

I landed this diff by adding the @DoNotStrip annotation and it fixed the crash, but another app started crashing because jni::findClassStatic(kBlobModuleJavaDescriptor) cannot find this class.

The crash occurs in the debug build, so this time it's not related to Proguard apparently. It has something to do with the Java class loader probably. Do you have any ideas?

[janicduplessis]

@makovkastar Hmm this is strange. Does this happen at launch, at a specific point or randomly later? We inject the __blobCollectorProvider function in the java BlobModule class so it has to exist at some point. I guess it's possible the class gets unloaded?

Do you know if there is something at facebook that allows native modules to be unloaded during runtime? In that case it is possible that the class gets unloaded if all instances are gone while the c++ code is still injected in the js runtime.

[makovkastar]

@janicduplessis Sorry, it took me some time to start working on this issue again. The crash happens always at the same time, shortly after the app is started. At first I thought it was because the destructor is not called from a Java thread, so JNI doesn't have access to the Java class loader (https://developer.android.com/training/articles/perf-jni#faq_FindClass). But by putting a breakpoint into the destructor of BlobCollector, I verified that it was actually called from a Java thread.

We can use ThreadScope::WithClassLoader to run code that has full access to Java as though you were running in a Java thread (in case you are in a non-Java thread and trying to use jni::findClass), so I tried to wrap the code in the destructor inside ThreadScope::WithClassLoader and it worked. Somehow even though the destructor is run on a Java thread, it doesn't have access to the class loader. This remains a mystery for me.

I don't think unloading classes is the case, since jni::findClassStatic cannot event find com/facebook/react/ReactActivity.

[janicduplessis]

@makovkastar Nice find! I don't have much jni experience so I didn't know about this. Seems like a good fix to me.

I remember reading there was no guarantee on what thread jni objects destructors are called on (hence adding synchronization to BlobManager) but didn't about the difference between c++ and java threads.

[tlupo]

@makovkastar @janicduplessis Thank you very much for your work on this issue !

I work on a RN mobile application which has to make recurrent API calls and I had some memory issues after few hours of app usage (this app has to be active 24/7).

I have tested the fix for iOS on the new RN version (v0.60 : 9ef5107) and it works !

I would like to fix the Android version of the app (which seems to leak with new version). If I understand, it seems that the addition of « ThreadScope::WithClassLoader » in the destructor fixes the last side effect of this proposal ?

Do you think it’s ready to be used in production (I am looking for the best way to add this patch on my RN version, maybe via a fork, it’s gonna be hard to wait for the new RN version because this memory issue is critical for our app) ?

Again thank you very much for your help !

[janicduplessis]

@tlupo Yes, I've been using this in prod for a few months without issues. 88e18b6 landed on master a few days ago which is the final version of this PR.

I'll post in the 0.60 release thread to see if we can get this cherry-picked in 0.60. You can also use this with a fork (note that you have to build android from source in this case https://github.com/facebook/react-native/wiki/Building-from-source).

[tlupo]

@janicduplessis Great ! I will try this PR ! Should be great to get it in RN 0.60. Thank you very much !