-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Blob] Release underlying resources when JS instance in GC'ed on iOS #24405
Conversation
TODO:
|
Libraries/Blob/RCTBlobCollector.mm
Outdated
} | ||
|
||
void RCTBlobCollector::install(RCTBlobManager *blobManager) { | ||
RCTCxxBridge *cxxBridge = (RCTCxxBridge *)blobManager.bridge; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit hacky to say the least but it was the only way I found to use jsi from a old RCTBridge
module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite hacky because the existing native module system doesn't give safe access to the JS runtime i.e. jsi::Runtime
. The bridge also has its own lifecycle assumption, so asking for runtime
off the bridge is not always safe.
RCTBlobManager *blobManager = blobManager_; | ||
NSString *blobId = [NSString stringWithUTF8String:blobId_.c_str()]; | ||
dispatch_async([blobManager_ methodQueue], ^{ | ||
[blobManager remove:blobId]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing this dance of using blobId as string, then asking nativemodule to clean it up, it's probably better long term to send down direct pointer to a blob (as jsi::Value) to JS, which can get cleaned up automatically by the JS runtime. See Fabric's UIManager method/impl as an example.
@janicduplessis mind addressing @fkgozali's comments? I'd really love for this fix to go in. |
@fkgozali Do you have any recommendations on how to make this safe / less hacky? What about adding a method on native modules a bit like |
TurboModule bypasses the bridge and interacts with Runtime executor directly during initialization. I don't think there's a safe way to get I think it's probably worth holding off on this change until TurboModule is ready? I think it'd be beneficial to refactor this module to take advantage of I'm closing this for now, but let's revisit in the future. |
@fkgozali Do you have any timeframe for when turbomodules will be ready? I definitely agree that this module would be a lot cleaner with the new infra. My only concern is that the current blob implementation causes a significant memory leak (pretty much every response of I assume fb doesn't use the Blob module so it isn't an issue but it is currently included by default in OSS so I think we'd need to fix this ASAP or disable the blob module. |
It'll be some time later this year, Q3/Q4, but if you're just targeting iOS only, we're currently testing in prod. So in theory you could start using it yourself (just need to write some of the "codegen" output by hand), it's just not enabled by default, yet.
We actually use blob module, but just not the full features I think. So the risk of getting random crash due to unsafe runtime access off bridge can affect FB as well.
Do you know how bad the leak is? Is it like MBs/minute, or more like MBs/day? |
The issue affects both iOS and Android so the fix would have to work for both, I just implemented it on iOS initially to get feedback.
I see, then you are probably affected by the leak. Note that the fix is safe, at worst the blob collector won't be created if for some reason the
I haven't measured personally but there are some info in this issue #23801, looks more like MBs / minute depending on network activity. |
I just did an audit and it looks like we use it only for development purpose for now, so if this is causing bad leaks for apps today, maybe it's ok to have this fix for the short term.
If you're fine with this hack for now, I guess you can have this in, but let's just document via comments so that it won't bite us in the future. When TurboModule is ready, we should really refactor this entire module to take advantage of proper |
@fkgozali The hack is more or less documented here https://github.com/facebook/react-native/pull/24405/files#diff-2bf13477704f772255c64e0beb652478R34, I can also create an issue to properly refactor the module when turbomodules are available. I've shipped this to production for a couple weeks and haven't had issues so I'm confident that at least it doesn't causes crashes. I'll look at implementing it for android if you are fine with shipping this as a temporary solution. |
I'll let @cpojer pull this in.
Could you add 1 sentence that "This is a temporary workaround."? |
Libraries/Blob/RCTBlobCollector.mm
Outdated
|
||
void RCTBlobCollector::install(RCTBlobManager *blobManager) { | ||
RCTCxxBridge *cxxBridge = (RCTCxxBridge *)blobManager.bridge; | ||
if (cxxBridge.runtime == nullptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this code again I can probably add a bit more safety: use a __weak
ref for cxxBrige
and move the cxxBridge.runtime == nullptr
inside the dispatched block.
I guess you already hinted it, but let's be explicit with temporary |
@janicduplessis let me know when this is ready to be pulled in. It still has [WIP] in the title and you mentioned Android support. |
@cpojer I guess adding android in a separate PR would be better, just need to cleanup something then this will be ready. |
As a user that is being directly impacted by these leaks, we're very appreciative of the team's quick movement to get a temporary fix in place until turbomodules is ready. |
@cpojer Should be ready for import! |
2214dae
to
f87571c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This pull request was successfully merged by @janicduplessis in c5c79e5. When will my fix make it into a release? | Upcoming Releases |
Seems like this failed with |
The changes from this PR were reverted in 05baf62 due to the issue Christoph mentioned. |
@hramos @cpojer I rebased on master but it still builds properly in OSS. I checked the diff that moved jsi headers but it doesn't seem like it requires updating the way jsi is imported (I checked it is still imported as Do you know if it fails for a specific project or all RN projects internally? Maybe it would be better if someone at fb can debug the build issue since I can't repro in OSS. |
…ces when JS instance in GC'ed on iOS Differential Revision: D15237418 Original commit changeset: 00a94a54b0b1 fbshipit-source-id: bb6c7aa3f5b6ae7f40965b96f1e0fd8eb7512015
Summary: The original reason for vendoring the fetch polyfill was to remove the default blob response type but this was reverted. Here's a little history around the fetch polyfill and the blob issue: - Original commit introducing the vendored polyfill: #19333, the goal was to fix a memory leak because our blob implementation doesn't release resources automatically. Not an ideal fix but since the issue was pretty severe and the infra for a proper fix was not in place. - This introduced an issue when downloading images using `fetch` which was fixed by #22063 which re-added the default blob content type. However that re-introduced the original fetch memory leak. - We have better infra now with jsi and I was able to get blob deallocation working, see #24405 Currently the vendored fetch polyfill is useless since it was changed back to the original version. We can just use the npm version again. I also updated to 3.0 which brings better spec compliance and support for cancellation via `AbortController`, https://github.com/github/fetch/releases/tag/v3.0.0. ## Changelog [General] [Changed] - Remove vendored fetch polyfill, update to whatwg-fetch@3.0 Pull Request resolved: #24418 Differential Revision: D14932683 Pulled By: cpojer fbshipit-source-id: 915e3d25978e8b9d7507ed807e7fba45aa88385a
Summary: The original reason for vendoring the fetch polyfill was to remove the default blob response type but this was reverted. Here's a little history around the fetch polyfill and the blob issue: - Original commit introducing the vendored polyfill: #19333, the goal was to fix a memory leak because our blob implementation doesn't release resources automatically. Not an ideal fix but since the issue was pretty severe and the infra for a proper fix was not in place. - This introduced an issue when downloading images using `fetch` which was fixed by #22063 which re-added the default blob content type. However that re-introduced the original fetch memory leak. - We have better infra now with jsi and I was able to get blob deallocation working, see #24405 Currently the vendored fetch polyfill is useless since it was changed back to the original version. We can just use the npm version again. I also updated to 3.0 which brings better spec compliance and support for cancellation via `AbortController`, https://github.com/github/fetch/releases/tag/v3.0.0. ## Changelog [General] [Changed] - Remove vendored fetch polyfill, update to whatwg-fetch@3.0 Pull Request resolved: #24418 Differential Revision: D14932683 Pulled By: cpojer fbshipit-source-id: 915e3d25978e8b9d7507ed807e7fba45aa88385a
Hello @janicduplessis ! As mentioned on the Android version of this PR (#24767), this fix seems to work on iOS. By profiling with Instruments (Allocations tool), I can see a stable memory usage (around 60 Mb). Without this PR, I observed a progressive increase of the memory usage (Malloc etc. on the Network layer) until the crash of the application. However, I don't understand why but I still receive "Low memory warning" (after 2/3 hours of app usage, even with this stable memory usage) and an app crash (without crash log). With your investigation on this part, do you have any information (a better way to debug memory usage or something else) to help me ? Thank you very much for your help. |
@tlupo Sadly I don't know of other ways, maybe it could be something else on the device that is eating up memory, eventually causing your app to be killed? I only did limited memory profiling since the leak was very obvious. |
…24418) Summary: The original reason for vendoring the fetch polyfill was to remove the default blob response type but this was reverted. Here's a little history around the fetch polyfill and the blob issue: - Original commit introducing the vendored polyfill: facebook#19333, the goal was to fix a memory leak because our blob implementation doesn't release resources automatically. Not an ideal fix but since the issue was pretty severe and the infra for a proper fix was not in place. - This introduced an issue when downloading images using `fetch` which was fixed by facebook#22063 which re-added the default blob content type. However that re-introduced the original fetch memory leak. - We have better infra now with jsi and I was able to get blob deallocation working, see facebook#24405 Currently the vendored fetch polyfill is useless since it was changed back to the original version. We can just use the npm version again. I also updated to 3.0 which brings better spec compliance and support for cancellation via `AbortController`, https://github.com/github/fetch/releases/tag/v3.0.0. ## Changelog [General] [Changed] - Remove vendored fetch polyfill, update to whatwg-fetch@3.0 Pull Request resolved: facebook#24418 Differential Revision: D14932683 Pulled By: cpojer fbshipit-source-id: 915e3d25978e8b9d7507ed807e7fba45aa88385a
Summary: This sync includes the following changes: - **[bd4784c8f](facebook/react@bd4784c8f )**: Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) ([#24434](facebook/react#24434)) //<dan>// - **[6d3b6d0f4](facebook/react@6d3b6d0f4 )**: forwardRef et al shouldn't affect if props reused ([#24421](facebook/react#24421)) //<Andrew Clark>// - **[bd0813766](facebook/react@bd0813766 )**: Fix: useDeferredValue should reuse previous value ([#24413](facebook/react#24413)) //<Andrew Clark>// - **[9ae80d6a2](facebook/react@9ae80d6a2 )**: Suppress hydration warnings when a preceding sibling suspends ([#24404](facebook/react#24404)) //<Josh Story>// - **[0dc4e6663](facebook/react@0dc4e6663 )**: Land enableClientRenderFallbackOnHydrationMismatch ([#24410](facebook/react#24410)) //<Andrew Clark>// - **[354772952](facebook/react@354772952 )**: Land enableSelectiveHydration flag ([#24406](facebook/react#24406)) //<Andrew Clark>// - **[392808a1f](facebook/react@392808a1f )**: Land enableClientRenderFallbackOnTextMismatch flag ([#24405](facebook/react#24405)) //<Andrew Clark>// - **[1e748b452](facebook/react@1e748b452 )**: Land enableLazyElements flag ([#24407](facebook/react#24407)) //<Andrew Clark>// - **[4175f0593](facebook/react@4175f0593 )**: Temporarily feature flag numeric fallback for symbols ([#24401](facebook/react#24401)) //<Ricky>// - **[a6d53f346](facebook/react@a6d53f346 )**: Revert "Clean up Selective Hydration / Event Replay flag ([#24156](facebook/react#24156))" ([#24402](facebook/react#24402)) //<Ricky>// - **[ab9cdd34f](facebook/react@ab9cdd34f )**: Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted ([#24400](facebook/react#24400)) //<Andrew Clark>// - **[168da8d55](facebook/react@168da8d55 )**: Fix typo that happened during rebasing //<Andrew Clark>// - **[8bc527a4c](facebook/react@8bc527a4c )**: Bugfix: Fix race condition between interleaved and non-interleaved updates ([#24353](facebook/react#24353)) //<Andrew Clark>// - **[f7cf077cc](facebook/react@f7cf077cc )**: [Transition Tracing] Add Offscreen Queue ([#24341](facebook/react#24341)) //<Luna Ruan>// - **[4fc394bbe](facebook/react@4fc394bbe )**: Fix suspense fallback throttling ([#24253](facebook/react#24253)) //<sunderls>// - **[80170a068](facebook/react@80170a068 )**: Match bundle.name and match upper case entry points ([#24346](facebook/react#24346)) //<Sebastian Markbåge>// - **[fea6f8da6](facebook/react@fea6f8da6 )**: [Transition Tracing] Add transition to OffscreenState and pendingSuspenseBoundaries to RootState ([#24340](facebook/react#24340)) //<Luna Ruan>// - **[8e2f9b086](facebook/react@8e2f9b086 )**: move passive flag ([#24339](facebook/react#24339)) //<Luna Ruan>// - **[55a21ef7e](facebook/react@55a21ef7e )**: fix pushTransition for transition tracing ([#24338](facebook/react#24338)) //<Luna Ruan>// - **[069d23bb7](facebook/react@069d23bb7 )**: [eslint-plugin-exhaustive-deps] Fix exhaustive deps check for unstable vars ([#24343](facebook/react#24343)) //<Afzal Sayed>// - **[4997515b9](facebook/react@4997515b9 )**: Point useSubscription to useSyncExternalStore shim ([#24289](facebook/react#24289)) //<dan>// - **[01e2bff1d](facebook/react@01e2bff1d )**: Remove unnecessary check ([#24332](facebook/react#24332)) //<zhoulixiang>// - **[d9a0f9e20](facebook/react@d9a0f9e20 )**: Delete create-subscription folder ([#24288](facebook/react#24288)) //<dan>// - **[f993ffc51](facebook/react@f993ffc51 )**: Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue ([#24247](facebook/react#24247)) //<Andrew Clark>// - **[fa5800226](facebook/react@fa5800226 )**: [Fizz] Pipeable Stream Perf ([#24291](facebook/react#24291)) //<Josh Story>// - **[0568c0f8c](facebook/react@0568c0f8c )**: Replace zero with NoLanes for consistency in FiberLane ([#24327](facebook/react#24327)) //<Leo>// - **[e0160d50c](facebook/react@e0160d50c )**: add transition tracing transitions stack ([#24321](facebook/react#24321)) //<Luna Ruan>// - **[b0f13e5d3](facebook/react@b0f13e5d3 )**: add pendingPassiveTransitions ([#24320](facebook/react#24320)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 60e63b9...bd4784c jest_e2e[run_all_tests] Reviewed By: kacieb Differential Revision: D35899012 fbshipit-source-id: 86a885e336fca9f0efa80cd2b8ca040f2cb53853
Summary: issue: #39441 For the following reasons, I have replaced an object used for id management inside BlobRegistry with `Map`. - The polyfill used for `fetch`, [whatwg-fetch](https://github.com/JakeChampion/fetch), returns responses as `Blob` objects. - When a `Blob` is created, it is registered with blobID in the [BlobRegistry](https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Blob/BlobRegistry.js), which is not automatically released. - This issue was previously reported in #19248 and was fixed by modifying whatwg-fetch. However, with the implementation of automatic garbage collection in #24405, the implementation was reverted in commit bccc92d, returning to the original behavior. - Although #24405 enables `Blob` objects to be garbage collected, the Blob IDs registered in the BlobRegistry remain, causing the count to increase each time `fetch` is called. - As a result, the `Property storage exceeds 196607 properties` error occurs To address this issue, I have modified the implementation of `BlobRegistry` to use a `Map` instead of an object. By using a `Map`, there is no limit to the number of entries. ## Changelog: [Internal] - [Fixed] - Fixed a bug that caused a "Property storage exceeds 196607 properties" error when sending a certain number of `fetch` requests. Pull Request resolved: #39528 Test Plan: I've added a new tests in `packages/react-native/Libraries/Blob/__tests__/BlobRegistry-test.js` and confirmed the test pass before and after changes. ``` $ yarn run test ... Test Suites: 1 skipped, 219 passed, 219 of 220 total Tests: 2 skipped, 4017 passed, 4019 total Snapshots: 1154 passed, 1154 total Time: 10.525 s Ran all test suites. ✨ Done in 12.52s. ``` Reviewed By: javache Differential Revision: D49423213 Pulled By: NickGerleman fbshipit-source-id: d5f73d7f5e34d1d2c3969b7dfbc45d3e6196aa30
Summary: issue: facebook#39441 For the following reasons, I have replaced an object used for id management inside BlobRegistry with `Map`. - The polyfill used for `fetch`, [whatwg-fetch](https://github.com/JakeChampion/fetch), returns responses as `Blob` objects. - When a `Blob` is created, it is registered with blobID in the [BlobRegistry](https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Blob/BlobRegistry.js), which is not automatically released. - This issue was previously reported in facebook#19248 and was fixed by modifying whatwg-fetch. However, with the implementation of automatic garbage collection in facebook#24405, the implementation was reverted in commit bccc92d, returning to the original behavior. - Although facebook#24405 enables `Blob` objects to be garbage collected, the Blob IDs registered in the BlobRegistry remain, causing the count to increase each time `fetch` is called. - As a result, the `Property storage exceeds 196607 properties` error occurs To address this issue, I have modified the implementation of `BlobRegistry` to use a `Map` instead of an object. By using a `Map`, there is no limit to the number of entries. ## Changelog: [Internal] - [Fixed] - Fixed a bug that caused a "Property storage exceeds 196607 properties" error when sending a certain number of `fetch` requests. Pull Request resolved: facebook#39528 Test Plan: I've added a new tests in `packages/react-native/Libraries/Blob/__tests__/BlobRegistry-test.js` and confirmed the test pass before and after changes. ``` $ yarn run test ... Test Suites: 1 skipped, 219 passed, 219 of 220 total Tests: 2 skipped, 4017 passed, 4019 total Snapshots: 1154 passed, 1154 total Time: 10.525 s Ran all test suites. ✨ Done in 12.52s. ``` Reviewed By: javache Differential Revision: D49423213 Pulled By: NickGerleman fbshipit-source-id: d5f73d7f5e34d1d2c3969b7dfbc45d3e6196aa30
Summary
Our Blob implementation was very problematic because it didn't release its underlying resource when the JS instance was dealocated. The main issue is that the fetch polyfill uses blobs by default if the module is available, which causes large memory leaks.
This fixes it by using the new jsi infra to attach a
jsi::HostObject
(BlobCollector
) toBlob
instances. This way when theBlob
is collected, theBlobCollector
also gets collected. Using thejsi::HostObject
dtor we can schedule the cleanup of native resources. This is definitely not the ideal solution but otherwise it would require rewriting the whole module using TurboModules + jsi.Fixes #23801, #20352, #21092
Changelog
[General] [Fixed] - [Blob] Release underlying resources when JS instance in GC'ed
Test Plan
new Blob(['HI'])
)