Skip to content

Commit 32a4c86

Browse files
mrousavymrousavy
authored
fix: Fix Kotlin HybridObject jni::global_ref memory leak by separating CxxPart with weak_ptr (#1238)
Separates out the C++ reference we hold in Kotlin to a separate `CxxPart` nested class that holds a `weak_ptr` to C++. That way we break the cyclic reference, and avoid a memory leak which can be reproduced in #1239 - Fixes a memory leak that happened with all Kotlin Hybrid Objects: margelo/react-native-vision-camera-v5#143 🥳 - Makes HybridObject initialization a **tiny bit** faster since we no longer call `initHybrid()` multiple times per subclass; now it's overridden and only called once - clean! :) - `JHybridObject` now only has one implementation for `equals(..)`, `toString()`, and `dispose()`, which is virtual in Java - no need for C++ duplicate implementations
1 parent f9da753 commit 32a4c86

45 files changed

Lines changed: 881 additions & 817 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎packages/nitrogen/src/autolinking/android/createHybridObjectInitializer.ts‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ export function createHybridObjectIntializer(): SourceFile[] {
2626

2727
const cppHybridObjectImports: SourceImport[] = []
2828
const cppRegistrations: string[] = []
29+
const cppDefinitions: string[] = []
2930
for (const hybridObjectName of Object.keys(autolinkedHybridObjects)) {
3031
const config = autolinkedHybridObjects[hybridObjectName]
3132

@@ -40,11 +41,13 @@ export function createHybridObjectIntializer(): SourceFile[] {
4041
}
4142
if (config?.kotlin != null) {
4243
// Autolink a Kotlin HybridObject through JNI/C++!
43-
const { cppCode, requiredImports } = createJNIHybridObjectRegistration({
44-
hybridObjectName: hybridObjectName,
45-
jniClassName: config.kotlin,
46-
})
44+
const { cppCode, cppDefinition, requiredImports } =
45+
createJNIHybridObjectRegistration({
46+
hybridObjectName: hybridObjectName,
47+
jniClassName: config.kotlin,
48+
})
4749
cppHybridObjectImports.push(...requiredImports)
50+
cppDefinitions.push(cppDefinition)
4851
cppRegistrations.push(cppCode)
4952
}
5053
}
@@ -113,6 +116,8 @@ int initialize(JavaVM* vm) {
113116
});
114117
}
115118
119+
${cppDefinitions.join('\n')}
120+
116121
void registerAllNatives() {
117122
using namespace margelo::nitro;
118123
using namespace ${cxxNamespace};

‎packages/nitrogen/src/syntax/kotlin/FbjniHybridObject.ts‎

Lines changed: 54 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createIndentation, indent } from '../../utils.js'
1+
import { indent } from '../../utils.js'
22
import { getForwardDeclaration } from '../c++/getForwardDeclaration.js'
33
import { includeHeader } from '../c++/includeNitroHeader.js'
44
import { getAllTypes } from '../getAllTypes.js'
@@ -35,22 +35,32 @@ export function createFbjniHybridObject(spec: HybridObjectSpec): SourceFile[] {
3535
'c++/jni',
3636
name.HybridTSpec
3737
)
38+
const cxxPartJniClassDescriptor = `${jniClassDescriptor}$CxxPart`
3839
const cxxNamespace = spec.config.getCxxNamespace('c++')
39-
const spaces = createIndentation(name.JHybridTSpec.length)
4040

41-
let cppBase = 'JHybridObject'
41+
let cppBaseClass = 'JHybridObject'
42+
let javaPartBaseClass = 'JHybridObject::JavaPart'
43+
let cxxPartBaseClass = 'JHybridObject::CxxPart'
44+
const constructorCalls = [
45+
`HybridObject(${name.HybridTSpec}::TAG)`,
46+
`JHybridObject(javaPart)`,
47+
]
4248
if (spec.baseTypes.length > 0) {
4349
if (spec.baseTypes.length > 1) {
4450
throw new Error(
4551
`${name.T}: Inheriting from multiple HybridObject bases is not yet supported on Kotlin!`
4652
)
4753
}
4854
const base = spec.baseTypes[0]!
49-
cppBase = getHybridObjectName(base.name).JHybridTSpec
55+
let baseTypename = getHybridObjectName(base.name).JHybridTSpec
5056
if (base.config.isExternalConfig) {
5157
// It's an external type we inherit from - we have to prefix the namespace
52-
cppBase = base.config.getCxxNamespace('c++', cppBase)
58+
baseTypename = base.config.getCxxNamespace('c++', baseTypename)
5359
}
60+
cppBaseClass = baseTypename
61+
javaPartBaseClass = `${baseTypename}::JavaPart`
62+
cxxPartBaseClass = `${baseTypename}::CxxPart`
63+
constructorCalls.push(`${baseTypename}(javaPart)`)
5464
}
5565
const cppImports: SourceImport[] = []
5666
for (const base of spec.baseTypes) {
@@ -90,34 +100,32 @@ namespace ${cxxNamespace} {
90100
91101
using namespace facebook;
92102
93-
class ${name.JHybridTSpec}: public jni::HybridClass<${name.JHybridTSpec}, ${cppBase}>,
94-
${spaces} public virtual ${name.HybridTSpec} {
103+
class ${name.JHybridTSpec}: public virtual ${name.HybridTSpec}, public virtual ${cppBaseClass} {
95104
public:
96-
static auto constexpr kJavaDescriptor = "L${jniClassDescriptor};";
97-
static jni::local_ref<jhybriddata> initHybrid(jni::alias_ref<jhybridobject> jThis);
98-
static void registerNatives();
99-
100-
protected:
101-
// C++ constructor (called from Java via \`initHybrid()\`)
102-
explicit ${name.JHybridTSpec}(jni::alias_ref<jhybridobject> jThis) :
103-
HybridObject(${name.HybridTSpec}::TAG),
104-
HybridBase(jThis),
105-
_javaPart(jni::make_global(jThis)) {}
105+
struct JavaPart: public jni::JavaClass<JavaPart, ${javaPartBaseClass}> {
106+
static auto constexpr kJavaDescriptor = "L${jniClassDescriptor};";
107+
std::shared_ptr<${name.JHybridTSpec}> get${name.JHybridTSpec}();
108+
};
109+
struct CxxPart: public jni::HybridClass<CxxPart, ${cxxPartBaseClass}> {
110+
static auto constexpr kJavaDescriptor = "L${cxxPartJniClassDescriptor};";
111+
static jni::local_ref<jhybriddata> initHybrid(jni::alias_ref<jhybridobject> jThis);
112+
static void registerNatives();
113+
using HybridBase::HybridBase;
114+
protected:
115+
std::shared_ptr<JHybridObject> createHybridObject(const jni::local_ref<JHybridObject::JavaPart>& javaPart) override;
116+
};
106117
107118
public:
119+
explicit ${name.JHybridTSpec}(const jni::local_ref<${name.JHybridTSpec}::JavaPart>& javaPart):
120+
${indent(constructorCalls.join(',\n'), ' ')},
121+
_javaPart(jni::make_global(javaPart)) {}
108122
~${name.JHybridTSpec}() override {
109123
// Hermes GC can destroy JS objects on a non-JNI Thread.
110124
jni::ThreadScope::WithClassLoader([&] { _javaPart.reset(); });
111125
}
112126
113127
public:
114-
size_t getExternalMemorySize() noexcept override;
115-
bool equals(const std::shared_ptr<HybridObject>& other) override;
116-
void dispose() noexcept override;
117-
std::string toString() override;
118-
119-
public:
120-
inline const jni::global_ref<${name.JHybridTSpec}::javaobject>& getJavaPart() const noexcept {
128+
inline const jni::global_ref<${name.JHybridTSpec}::JavaPart>& getJavaPart() const noexcept {
121129
return _javaPart;
122130
}
123131
@@ -130,9 +138,7 @@ ${spaces} public virtual ${name.HybridTSpec} {
130138
${indent(methodsDecl, ' ')}
131139
132140
private:
133-
friend HybridBase;
134-
using HybridBase::HybridBase;
135-
jni::global_ref<${name.JHybridTSpec}::javaobject> _javaPart;
141+
jni::global_ref<${name.JHybridTSpec}::JavaPart> _javaPart;
136142
};
137143
138144
} // namespace ${cxxNamespace}
@@ -141,7 +147,7 @@ ${spaces} public virtual ${name.HybridTSpec} {
141147
// Make sure we register all native JNI methods on app startup
142148
addJNINativeRegistration({
143149
namespace: cxxNamespace,
144-
className: `${name.JHybridTSpec}`,
150+
className: `${name.JHybridTSpec}::CxxPart`,
145151
import: {
146152
name: `${name.JHybridTSpec}.hpp`,
147153
space: 'user',
@@ -180,37 +186,31 @@ ${cppIncludes.join('\n')}
180186
181187
namespace ${cxxNamespace} {
182188
183-
jni::local_ref<${name.JHybridTSpec}::jhybriddata> ${name.JHybridTSpec}::initHybrid(jni::alias_ref<jhybridobject> jThis) {
184-
return makeCxxInstance(jThis);
185-
}
186-
187-
void ${name.JHybridTSpec}::registerNatives() {
188-
registerHybrid({
189-
makeNativeMethod("initHybrid", ${name.JHybridTSpec}::initHybrid),
190-
});
189+
std::shared_ptr<${name.JHybridTSpec}> ${name.JHybridTSpec}::JavaPart::get${name.JHybridTSpec}() {
190+
auto hybridObject = JHybridObject::JavaPart::getJHybridObject();
191+
auto castHybridObject = std::dynamic_pointer_cast<${name.JHybridTSpec}>(hybridObject);
192+
if (castHybridObject == nullptr) [[unlikely]] {
193+
throw std::runtime_error("Failed to downcast JHybridObject to ${name.JHybridTSpec}!");
194+
}
195+
return castHybridObject;
191196
}
192197
193-
size_t ${name.JHybridTSpec}::getExternalMemorySize() noexcept {
194-
static const auto method = javaClassStatic()->getMethod<jlong()>("getMemorySize");
195-
return method(_javaPart);
198+
jni::local_ref<${name.JHybridTSpec}::CxxPart::jhybriddata> ${name.JHybridTSpec}::CxxPart::initHybrid(jni::alias_ref<jhybridobject> jThis) {
199+
return makeCxxInstance(jThis);
196200
}
197201
198-
bool ${name.JHybridTSpec}::equals(const std::shared_ptr<HybridObject>& other) {
199-
if (auto otherCast = std::dynamic_pointer_cast<${name.JHybridTSpec}>(other)) {
200-
return _javaPart == otherCast->_javaPart;
202+
std::shared_ptr<JHybridObject> ${name.JHybridTSpec}::CxxPart::createHybridObject(const jni::local_ref<JHybridObject::JavaPart>& javaPart) {
203+
auto castJavaPart = jni::dynamic_ref_cast<${name.JHybridTSpec}::JavaPart>(javaPart);
204+
if (castJavaPart == nullptr) [[unlikely]] {
205+
throw std::runtime_error("Failed to cast JHybridObject::JavaPart to ${name.JHybridTSpec}::JavaPart!");
201206
}
202-
return false;
207+
return std::make_shared<${name.JHybridTSpec}>(castJavaPart);
203208
}
204209
205-
void ${name.JHybridTSpec}::dispose() noexcept {
206-
static const auto method = javaClassStatic()->getMethod<void()>("dispose");
207-
method(_javaPart);
208-
}
209-
210-
std::string ${name.JHybridTSpec}::toString() {
211-
static const auto method = javaClassStatic()->getMethod<jni::JString()>("toString");
212-
auto javaString = method(_javaPart);
213-
return javaString->toStdString();
210+
void ${name.JHybridTSpec}::CxxPart::registerNatives() {
211+
registerHybrid({
212+
makeNativeMethod("initHybrid", ${name.JHybridTSpec}::CxxPart::initHybrid),
213+
});
214214
}
215215
216216
// Properties
@@ -275,14 +275,14 @@ function getFbjniMethodForwardImplementation(
275275
if (returnJNI.hasType) {
276276
// return something - we need to parse it
277277
body = `
278-
static const auto method = javaClassStatic()->getMethod<${cxxSignature}>("${methodName}");
278+
static const auto method = _javaPart->javaClassStatic()->getMethod<${cxxSignature}>("${methodName}");
279279
auto __result = method(${paramsForward.join(', ')});
280280
return ${returnJNI.parse('__result', 'kotlin', 'c++', 'c++')};
281281
`
282282
} else {
283283
// void method. no return
284284
body = `
285-
static const auto method = javaClassStatic()->getMethod<${cxxSignature}>("${methodName}");
285+
static const auto method = _javaPart->javaClassStatic()->getMethod<${cxxSignature}>("${methodName}");
286286
method(${paramsForward.join(', ')});
287287
`
288288
}

‎packages/nitrogen/src/syntax/kotlin/KotlinCxxBridgedType.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ export class KotlinCxxBridgedType implements BridgedType<'kotlin', 'c++'> {
363363
case 'c++':
364364
const hybridObjectType = getTypeAs(this.type, HybridObjectType)
365365
const fullName = this.getFullJHybridObjectName(hybridObjectType)
366-
return `${fullName}::javaobject`
366+
return `${fullName}::JavaPart`
367367
default:
368368
return this.type.getCode(language)
369369
}
@@ -472,7 +472,6 @@ export class KotlinCxxBridgedType implements BridgedType<'kotlin', 'c++'> {
472472
// any jni::HybridClass needs to be dereferenced to jobject with .get()
473473
case 'array-buffer':
474474
case 'function':
475-
case 'hybrid-object':
476475
case 'hybrid-object-base':
477476
case 'map':
478477
case 'promise':
@@ -849,9 +848,11 @@ export class KotlinCxxBridgedType implements BridgedType<'kotlin', 'c++'> {
849848
case 'hybrid-object': {
850849
switch (language) {
851850
case 'c++':
852-
const hybrid = getTypeAs(this.type, HybridObjectType)
853-
const fullName = this.getFullJHybridObjectName(hybrid)
854-
return `${parameterName}->cthis()->shared_cast<${fullName}>()`
851+
const hybridObject = getTypeAs(this.type, HybridObjectType)
852+
const { JHybridTSpec } = getHybridObjectName(
853+
hybridObject.hybridObjectName
854+
)
855+
return `${parameterName}->get${JHybridTSpec}()`
855856
default:
856857
return parameterName
857858
}

‎packages/nitrogen/src/syntax/kotlin/KotlinHybridObject.ts‎

Lines changed: 21 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -26,22 +26,22 @@ export function createKotlinHybridObject(spec: HybridObjectSpec): SourceFile[] {
2626
...spec.baseTypes.flatMap((b) =>
2727
new HybridObjectType(b).getRequiredImports('kotlin')
2828
),
29+
{
30+
name: 'com.margelo.nitro.core.HybridObject',
31+
space: 'system',
32+
language: 'kotlin',
33+
},
2934
]
3035
if (spec.isHybridView) {
3136
extraImports.push({
3237
name: 'com.margelo.nitro.views.HybridView',
3338
space: 'system',
3439
language: 'kotlin',
3540
})
36-
} else {
37-
extraImports.push({
38-
name: 'com.margelo.nitro.core.HybridObject',
39-
space: 'system',
40-
language: 'kotlin',
41-
})
4241
}
4342

4443
let kotlinBase = spec.isHybridView ? 'HybridView' : 'HybridObject'
44+
let cxxPartBase = 'HybridObject.CxxPart'
4545
if (spec.baseTypes.length > 0) {
4646
if (spec.baseTypes.length > 1) {
4747
throw new Error(
@@ -51,6 +51,7 @@ export function createKotlinHybridObject(spec: HybridObjectSpec): SourceFile[] {
5151
const base = spec.baseTypes[0]!
5252
const baseHybrid = new HybridObjectType(base)
5353
kotlinBase = baseHybrid.getCode('kotlin')
54+
cxxPartBase = `${kotlinBase}.CxxPart`
5455
}
5556

5657
const imports = extraImports
@@ -82,30 +83,27 @@ ${imports.join('\n')}
8283
"LocalVariableName", "PropertyName", "PrivatePropertyName", "FunctionName"
8384
)
8485
abstract class ${name.HybridTSpec}: ${kotlinBase}() {
85-
@DoNotStrip
86-
private var mHybridData: HybridData = initHybrid()
87-
88-
init {
89-
super.updateNative(mHybridData)
90-
}
86+
// Properties
87+
${indent(properties, ' ')}
9188
92-
override fun updateNative(hybridData: HybridData) {
93-
mHybridData = hybridData
94-
super.updateNative(hybridData)
95-
}
89+
// Methods
90+
${indent(methods, ' ')}
9691
9792
// Default implementation of \`HybridObject.toString()\`
9893
override fun toString(): String {
9994
return "[HybridObject ${name.T}]"
10095
}
10196
102-
// Properties
103-
${indent(properties, ' ')}
104-
105-
// Methods
106-
${indent(methods, ' ')}
107-
108-
private external fun initHybrid(): HybridData
97+
// C++ backing class
98+
@DoNotStrip
99+
@Keep
100+
protected open class CxxPart(javaPart: ${name.HybridTSpec}): ${cxxPartBase}(javaPart) {
101+
// C++ ${name.JHybridTSpec}::CxxPart::initHybrid(...)
102+
external override fun initHybrid(): HybridData
103+
}
104+
override fun createCxxPart(): CxxPart {
105+
return CxxPart(this)
106+
}
109107
110108
companion object {
111109
protected const val TAG = "${name.HybridTSpec}"

‎packages/nitrogen/src/syntax/kotlin/KotlinHybridObjectRegistration.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ interface Props {
1515

1616
interface JNIHybridObjectRegistration {
1717
cppCode: string
18+
cppDefinition: string
1819
requiredImports: SourceImport[]
1920
}
2021

@@ -37,13 +38,21 @@ export function createJNIHybridObjectRegistration({
3738
space: 'system',
3839
},
3940
],
41+
cppDefinition: `
42+
struct ${JHybridTSpec}Impl: public jni::JavaClass<${JHybridTSpec}Impl, ${JHybridTSpec}::JavaPart> {
43+
static auto constexpr kJavaDescriptor = "L${jniNamespace};";
44+
static std::shared_ptr<${JHybridTSpec}> create() {
45+
static auto constructorFn = javaClassStatic()->getConstructor<${JHybridTSpec}Impl::javaobject()>();
46+
jni::local_ref<${JHybridTSpec}::JavaPart> javaPart = javaClassStatic()->newObject(constructorFn);
47+
return javaPart->get${JHybridTSpec}();
48+
}
49+
};
50+
`.trim(),
4051
cppCode: `
4152
HybridObjectRegistry::registerHybridObjectConstructor(
4253
"${hybridObjectName}",
4354
[]() -> std::shared_ptr<HybridObject> {
44-
static DefaultConstructableObject<${JHybridTSpec}::javaobject> object("${jniNamespace}");
45-
auto instance = object.create();
46-
return instance->cthis()->shared();
55+
return ${JHybridTSpec}Impl::create();
4756
}
4857
);
4958
`.trim(),

0 commit comments

Comments
 (0)