From c647950e5e78f1bcd110111067b21daa44e4d464 Mon Sep 17 00:00:00 2001 From: "huzhanbo.luc" Date: Thu, 9 May 2024 19:55:43 -0700 Subject: [PATCH] fix fetch memory leak (#44336) Summary: Root cause of the fetch memory leak: The fetch requests store its result inside Blob which memory is managed by BlobCollector. On the JS engine side, the Blob is represented by an ID as JS string, and the GC don't know the size of the blob. So GC won't have interests to release the Blob. Fix: On iOS and Android, use `setExternalMemoryPressure` to acknowledge JS engine the size of Blob it holds. ## Changelog: [GENERAL] [FIXED] - fix fetch memory leak Pull Request resolved: https://github.com/facebook/react-native/pull/44336 Test Plan: `RepeatedlyFetch` inside `XHR` example Reviewed By: javache Differential Revision: D57145270 Pulled By: NickGerleman fbshipit-source-id: afa53540e8563db4f9c6657f2dbbdff7bdfa66c0 --- .../Libraries/Blob/RCTBlobCollector.mm | 5 ++- .../Libraries/Blob/RCTBlobManager.h | 2 ++ .../Libraries/Blob/RCTBlobManager.mm | 6 ++++ .../ReactAndroid/api/ReactAndroid.api | 1 + .../react/modules/blob/BlobModule.java | 8 +++++ .../react/reactnativeblob/BlobCollector.cpp | 14 +++++++- .../jni/react/reactnativeblob/BlobCollector.h | 2 ++ .../js/examples/XHR/XHRExampleFetch.js | 32 ++++++++++++++++++- 8 files changed, 67 insertions(+), 3 deletions(-) diff --git a/packages/react-native/Libraries/Blob/RCTBlobCollector.mm b/packages/react-native/Libraries/Blob/RCTBlobCollector.mm index 9b2ca8cb3a0..0043c5ae44c 100644 --- a/packages/react-native/Libraries/Blob/RCTBlobCollector.mm +++ b/packages/react-native/Libraries/Blob/RCTBlobCollector.mm @@ -45,7 +45,10 @@ void RCTBlobCollector::install(RCTBlobManager *blobManager) [blobManager](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(blobManager, blobId); - return jsi::Object::createFromHostObject(rt, blobCollector); + auto blobCollectorJsObject = jsi::Object::createFromHostObject(rt, blobCollector); + blobCollectorJsObject.setExternalMemoryPressure( + rt, [blobManager lengthOfBlobWithId:[NSString stringWithUTF8String:blobId.c_str()]]); + return blobCollectorJsObject; })); } queue:RCTJSThread]; diff --git a/packages/react-native/Libraries/Blob/RCTBlobManager.h b/packages/react-native/Libraries/Blob/RCTBlobManager.h index bceb8eb5d81..38a7238380b 100755 --- a/packages/react-native/Libraries/Blob/RCTBlobManager.h +++ b/packages/react-native/Libraries/Blob/RCTBlobManager.h @@ -18,6 +18,8 @@ RCT_EXTERN void RCTEnableBlobManagerProcessingQueue(BOOL enabled); - (void)store:(NSData *)data withId:(NSString *)blobId; +- (NSUInteger)lengthOfBlobWithId:(NSString *)blobId; + - (NSData *)resolve:(NSDictionary *)blob; - (NSData *)resolve:(NSString *)blobId offset:(NSInteger)offset size:(NSInteger)size; diff --git a/packages/react-native/Libraries/Blob/RCTBlobManager.mm b/packages/react-native/Libraries/Blob/RCTBlobManager.mm index 286710d6910..9d4cde7c457 100755 --- a/packages/react-native/Libraries/Blob/RCTBlobManager.mm +++ b/packages/react-native/Libraries/Blob/RCTBlobManager.mm @@ -98,6 +98,12 @@ RCT_EXPORT_MODULE(BlobModule) _blobs[blobId] = data; } +- (NSUInteger)lengthOfBlobWithId:(NSString *)blobId +{ + std::lock_guard lock(_blobsMutex); + return _blobs[blobId].length; +} + - (NSData *)resolve:(NSDictionary *)blob { NSString *blobId = [RCTConvert NSString:blob[@"blobId"]]; diff --git a/packages/react-native/ReactAndroid/api/ReactAndroid.api b/packages/react-native/ReactAndroid/api/ReactAndroid.api index 1043824e561..a3a849e0704 100644 --- a/packages/react-native/ReactAndroid/api/ReactAndroid.api +++ b/packages/react-native/ReactAndroid/api/ReactAndroid.api @@ -3015,6 +3015,7 @@ public class com/facebook/react/modules/blob/BlobModule : com/facebook/fbreact/s public fun addNetworkingHandler ()V public fun addWebSocketHandler (D)V public fun createFromParts (Lcom/facebook/react/bridge/ReadableArray;Ljava/lang/String;)V + public fun getLengthOfBlob (Ljava/lang/String;)J public fun getTypedExportedConstants ()Ljava/util/Map; public fun initialize ()V public fun release (Ljava/lang/String;)V diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobModule.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobModule.java index d86962793ed..01b37af9268 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobModule.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobModule.java @@ -174,6 +174,14 @@ public class BlobModule extends NativeBlobModuleSpec { } } + @DoNotStrip + public long getLengthOfBlob(String blobId) { + synchronized (mBlobs) { + byte[] data = mBlobs.get(blobId); + return data != null ? data.length : 0; + } + } + @DoNotStrip public void remove(String blobId) { synchronized (mBlobs) { diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/reactnativeblob/BlobCollector.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/reactnativeblob/BlobCollector.cpp index d16ba17aa4c..9b8f80cb9eb 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/reactnativeblob/BlobCollector.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/reactnativeblob/BlobCollector.cpp @@ -32,6 +32,14 @@ BlobCollector::~BlobCollector() { }); } +size_t BlobCollector::getBlobLength() { + static auto getLengthMethod = + jni::findClassStatic(kBlobModuleJavaDescriptor) + ->getMethod("getLengthOfBlob"); + auto length = getLengthMethod(blobModule_, jni::make_jstring(blobId_).get()); + return static_cast(length); +} + void BlobCollector::nativeInstall( jni::alias_ref, jni::alias_ref blobModule, @@ -53,7 +61,11 @@ void BlobCollector::nativeInstall( auto blobId = args[0].asString(rt).utf8(rt); auto blobCollector = std::make_shared(blobModuleRef, blobId); - return jsi::Object::createFromHostObject(rt, blobCollector); + auto blobCollectorJsObject = + jsi::Object::createFromHostObject(rt, blobCollector); + blobCollectorJsObject.setExternalMemoryPressure( + rt, blobCollector->getBlobLength()); + return blobCollectorJsObject; })); } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/reactnativeblob/BlobCollector.h b/packages/react-native/ReactAndroid/src/main/jni/react/reactnativeblob/BlobCollector.h index f72e9be5ffb..17046b494f2 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/reactnativeblob/BlobCollector.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/reactnativeblob/BlobCollector.h @@ -18,6 +18,8 @@ class BlobCollector : public jni::HybridClass, BlobCollector(jni::global_ref blobModule, const std::string& blobId); ~BlobCollector(); + size_t getBlobLength(); + static constexpr auto kJavaDescriptor = "Lcom/facebook/react/modules/blob/BlobCollector;"; diff --git a/packages/rn-tester/js/examples/XHR/XHRExampleFetch.js b/packages/rn-tester/js/examples/XHR/XHRExampleFetch.js index 5d386d5ba58..3f49af76b48 100644 --- a/packages/rn-tester/js/examples/XHR/XHRExampleFetch.js +++ b/packages/rn-tester/js/examples/XHR/XHRExampleFetch.js @@ -11,7 +11,14 @@ 'use strict'; const React = require('react'); -const {Platform, StyleSheet, Text, TextInput, View} = require('react-native'); +const { + Button, + Platform, + StyleSheet, + Text, + TextInput, + View, +} = require('react-native'); class XHRExampleFetch extends React.Component { responseURL: ?string; @@ -59,6 +66,25 @@ class XHRExampleFetch extends React.Component { return responseHeaders; } + startRepeatedlyFetch() { + const doRequest = () => { + const url = + 'https://microsoftedge.github.io/Demos/json-dummy-data/5MB-min.json'; + fetch(url, { + method: 'GET', + headers: { + Accept: 'application/json', + 'Content-Type': 'application/json', + }, + }) + .then(() => { + console.log('fetch one time'); + }) + .catch(error => console.error(error)); + }; + setInterval(doRequest, 500); + } + render(): React.Node { const responseURL = this.responseURL ? ( @@ -88,6 +114,10 @@ class XHRExampleFetch extends React.Component { return ( +