Skip to content

Commit

Permalink
Release underlying resources when JS instance is GC'ed on Android
Browse files Browse the repository at this point in the history
Summary:
[Android] [Added] - Release underlying resources when JS instance is GC'ed on Android

D15826082 was reverted because it introduced a crash in Ads Manager for Android (see P67222724).

This diff fixes the crash and re-applies D15826082. The problem was that `jni::findClassStatic` in the destructor of BlobCollector.cpp couldn't find the Java class `com/facebook/react/modules/blob/BlobModule` and crashed the app.

JNI didn't seem to have access to the Java class loader probably because the destructor was called from a non-Java thread (https://our.intern.facebook.com/intern/wiki/Fbjni/environment-and-thread-management/?vitals_event=wiki_click_navigation_link#threads). The fix is to wrap the code in the destructor inside `ThreadScope::WithClassLoader `, which will allow to run code that has full access to Java as though you were running in a Java thread.

Reviewed By: shergin

Differential Revision: D16122059

fbshipit-source-id: 12f14fa4a58218242a482c2c3e2149bb6770e8ec
  • Loading branch information
makovkastar authored and facebook-github-bot committed Jul 9, 2019
1 parent d656878 commit 88e18b6
Show file tree
Hide file tree
Showing 11 changed files with 215 additions and 12 deletions.
1 change: 1 addition & 0 deletions RNTester/android/app/BUCK
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ rn_android_library(
react_native_target("java/com/facebook/react:react"),
react_native_target("java/com/facebook/react/common:build_config"),
react_native_target("java/com/facebook/react/modules/core:core"),
react_native_target("java/com/facebook/react/views/text:text"),
react_native_target("java/com/facebook/react/shell:shell"),
react_native_target("jni/prebuilt:android-jsc"),
# .so files are prebuilt by Gradle with `./gradlew :ReactAndroid:packageReactNdkLibsForBuck`
Expand Down
1 change: 1 addition & 0 deletions ReactAndroid/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ def getNdkBuildFullPath() {
task buildReactNdkLib(dependsOn: [prepareJSC, prepareBoost, prepareDoubleConversion, prepareFolly, prepareGlog], type: Exec) {
inputs.dir("$projectDir/../ReactCommon")
inputs.dir("src/main/jni")
inputs.dir("src/main/java/com/facebook/react/modules/blob")
outputs.dir("$buildDir/react-ndk/all")
commandLine(getNdkBuildFullPath(),
"NDK_DEBUG=" + (nativeBuildType.equalsIgnoreCase("debug") ? "1" : "0"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ rn_android_library(
],
deps = [
react_native_dep("libraries/fbcore/src/main/java/com/facebook/common/logging:logging"),
react_native_dep("libraries/soloader/java/com/facebook/soloader:soloader"),
react_native_dep("third-party/java/infer-annotations:infer-annotations"),
react_native_dep("third-party/java/jsr-305:jsr-305"),
react_native_dep("third-party/java/okhttp:okhttp3"),
Expand All @@ -24,6 +25,7 @@ rn_android_library(
react_native_target("java/com/facebook/react/bridge:bridge"),
react_native_target("java/com/facebook/react/common:common"),
react_native_target("java/com/facebook/react/module/annotations:annotations"),
react_native_target("java/com/facebook/react/modules/blob/jni:jni"),
react_native_target("java/com/facebook/react/modules/network:network"),
react_native_target("java/com/facebook/react/modules/websocket:websocket"),
],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package com.facebook.react.modules.blob;

import com.facebook.react.bridge.JavaScriptContextHolder;
import com.facebook.react.bridge.ReactContext;
import com.facebook.soloader.SoLoader;

/* package */ class BlobCollector {
static {
SoLoader.loadLibrary("reactnativeblob");
}

static void install(final ReactContext reactContext, final BlobModule blobModule) {
reactContext.runOnJSQueueThread(
new Runnable() {
@Override
public void run() {
JavaScriptContextHolder jsContext = reactContext.getJavaScriptContextHolder();
synchronized (jsContext) {
nativeInstall(blobModule, jsContext.get());
}
}
});
}

private static native void nativeInstall(Object blobModule, long jsContext);
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import android.provider.MediaStore;
import android.webkit.MimeTypeMap;
import androidx.annotation.Nullable;
import com.facebook.proguard.annotations.DoNotStrip;
import com.facebook.react.bridge.Arguments;
import com.facebook.react.bridge.ReactApplicationContext;
import com.facebook.react.bridge.ReactContextBaseJavaModule;
Expand Down Expand Up @@ -143,6 +144,11 @@ public BlobModule(ReactApplicationContext reactContext) {
super(reactContext);
}

@Override
public void initialize() {
BlobCollector.install(getReactApplicationContext(), this);
}

@Override
public String getName() {
return NAME;
Expand Down Expand Up @@ -170,11 +176,16 @@ public String store(byte[] data) {
}

public void store(byte[] data, String blobId) {
mBlobs.put(blobId, data);
synchronized (mBlobs) {
mBlobs.put(blobId, data);
}
}

@DoNotStrip
public void remove(String blobId) {
mBlobs.remove(blobId);
synchronized (mBlobs) {
mBlobs.remove(blobId);
}
}

public @Nullable byte[] resolve(Uri uri) {
Expand All @@ -193,17 +204,19 @@ public void remove(String blobId) {
}

public @Nullable byte[] resolve(String blobId, int offset, int size) {
byte[] data = mBlobs.get(blobId);
if (data == null) {
return null;
}
if (size == -1) {
size = data.length - offset;
}
if (offset > 0 || size != data.length) {
data = Arrays.copyOfRange(data, offset, offset + size);
synchronized (mBlobs) {
byte[] data = mBlobs.get(blobId);
if (data == null) {
return null;
}
if (size == -1) {
size = data.length - offset;
}
if (offset > 0 || size != data.length) {
data = Arrays.copyOfRange(data, offset, offset + size);
}
return data;
}
return data;
}

public @Nullable byte[] resolve(ReadableMap blob) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Copyright (c) Facebook, Inc. and its affiliates.
#
# This source code is licensed under the MIT license found in the
# LICENSE file in the root directory of this source tree.

LOCAL_PATH := $(call my-dir)

include $(CLEAR_VARS)

LOCAL_MODULE := reactnativeblob

LOCAL_SRC_FILES := $(wildcard $(LOCAL_PATH)/*.cpp)

LOCAL_C_INCLUDES := $(LOCAL_PATH)

LOCAL_CFLAGS += -fvisibility=hidden -fexceptions -frtti

LOCAL_STATIC_LIBRARIES := libjsi libjsireact jscruntime
LOCAL_SHARED_LIBRARIES := libfolly_json libfb libreactnativejni

include $(BUILD_SHARED_LIBRARY)
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
load("//tools/build_defs/oss:rn_defs.bzl", "ANDROID", "FBJNI_TARGET", "react_native_target", "react_native_xplat_target", "rn_xplat_cxx_library")

rn_xplat_cxx_library(
name = "jni",
srcs = glob(["*.cpp"]),
headers = glob(["*.h"]),
header_namespace = "",
compiler_flags = ["-fexceptions"],
fbandroid_allow_jni_merging = True,
platforms = ANDROID,
soname = "libreactnativeblob.$(ext)",
visibility = [
"PUBLIC",
],
deps = [
"fbsource//xplat/folly:molly",
FBJNI_TARGET,
react_native_target("jni/react/jni:jni"),
react_native_xplat_target("jsi:JSCRuntime"),
react_native_xplat_target("jsiexecutor:jsiexecutor"),
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Copyright 2004-present Facebook. All Rights Reserved.
// This source code is licensed under the MIT license found in the
// LICENSE file in the root directory of this source tree.

#include "BlobCollector.h"

#include <fb/fbjni.h>
#include <memory>
#include <mutex>

using namespace facebook;

namespace facebook {
namespace react {

static constexpr auto kBlobModuleJavaDescriptor =
"com/facebook/react/modules/blob/BlobModule";

BlobCollector::BlobCollector(
jni::global_ref<jobject> blobModule,
const std::string &blobId)
: blobModule_(blobModule), blobId_(blobId) {}

BlobCollector::~BlobCollector() {
jni::ThreadScope::WithClassLoader([&] {
static auto removeMethod = jni::findClassStatic(kBlobModuleJavaDescriptor)
->getMethod<void(jstring)>("remove");
removeMethod(blobModule_, jni::make_jstring(blobId_).get());
});
}

void BlobCollector::nativeInstall(
jni::alias_ref<jhybridobject> jThis,
jni::alias_ref<jobject> blobModule,
jlong jsContextNativePointer) {
auto &runtime = *((jsi::Runtime *) jsContextNativePointer);
auto blobModuleRef = jni::make_global(blobModule);
runtime.global().setProperty(
runtime,
"__blobCollectorProvider",
jsi::Function::createFromHostFunction(
runtime,
jsi::PropNameID::forAscii(runtime, "__blobCollectorProvider"),
1,
[blobModuleRef](
jsi::Runtime &rt,
const jsi::Value &thisVal,
const jsi::Value *args,
size_t count) {
auto blobId = args[0].asString(rt).utf8(rt);
auto blobCollector =
std::make_shared<BlobCollector>(blobModuleRef, blobId);
return jsi::Object::createFromHostObject(rt, blobCollector);
}));
}

void BlobCollector::registerNatives() {
registerHybrid(
{makeNativeMethod("nativeInstall", BlobCollector::nativeInstall)});
}

} // namespace react
} // namespace facebook
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright 2004-present Facebook. All Rights Reserved.
// This source code is licensed under the MIT license found in the
// LICENSE file in the root directory of this source tree.

#pragma once

#include <fb/fbjni.h>
#include <jsi/jsi.h>

namespace facebook {
namespace react {

class BlobCollector : public jni::HybridClass<BlobCollector>,
public jsi::HostObject {
public:
BlobCollector(
jni::global_ref<jobject> blobManager,
const std::string &blobId);
~BlobCollector();

static constexpr auto kJavaDescriptor =
"Lcom/facebook/react/modules/blob/BlobCollector;";

static void nativeInstall(
jni::alias_ref<jhybridobject> jThis,
jni::alias_ref<jobject> blobModule,
jlong jsContextNativePointer);

static void registerNatives();

private:
friend HybridBase;

jni::global_ref<jobject> blobModule_;
const std::string blobId_;
};

} // namespace react
} // namespace facebook
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright 2004-present Facebook. All Rights Reserved.

// This source code is licensed under the MIT license found in the
// LICENSE file in the root directory of this source tree.

#include <fb/fbjni.h>

#include "BlobCollector.h"

JNIEXPORT jint JNICALL JNI_OnLoad(JavaVM *vm, void *reserved) {
return facebook::jni::initialize(
vm, [] { facebook::react::BlobCollector::registerNatives(); });
}
2 changes: 2 additions & 0 deletions ReactAndroid/src/main/jni/react/jni/Android.mk
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,5 @@ include $(REACT_SRC_DIR)/turbomodule/core/jni/Android.mk
# $(call import-module,jscexecutor)

include $(REACT_SRC_DIR)/jscexecutor/Android.mk

include $(REACT_SRC_DIR)/modules/blob/jni/Android.mk

3 comments on commit 88e18b6

@janicduplessis
Copy link
Contributor

Choose a reason for hiding this comment

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

@makovkastar Thanks for your help landing this!

@atticoos
Copy link
Contributor

@atticoos atticoos commented on 88e18b6 Aug 20, 2019

Choose a reason for hiding this comment

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

Oh my.. that... that allows the native side to perform side effects when JS objects are garbage collected...

Mind. Absolutely. Blown.

@talha-apps
Copy link

Choose a reason for hiding this comment

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

@janicduplessis thanks for creating this. I am not sure about the process here. Is there anything preventing this from getting merged?

Please sign in to comment.