[Blob] Release underlying resources when JS instance in GC'ed #24745
Conversation
facebook-github-bot
added
CLA Signed
…ces when JS instance in GC'ed on iOS Differential Revision: D15237418 Original commit changeset: 00a94a54b0b1 fbshipit-source-id: bb6c7aa3f5b6ae7f40965b96f1e0fd8eb7512015
There was a problem hiding this comment.
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
The error still occurs, but our current understanding is that this is specific to one of our internal Buck tests, and that we'll need to update our Buck definition, something that you won't be able to change as part of this PR. Please hold off on adding any commits to this PR. I'll work on making the necessary changes over the internal version of this PR (where I can update filepaths outside of the React Native GitHub directory) and once the issue is resolved and the internal diff lands, this PR shall be marked as merged. |
|
Awesome! Thanks @hramos |
|
I have a potential fix waiting for review, shouldn't be long. |
|
The fix I sent last night was approved this morning. The conflict did hold the initial attempt to land this back, but I just resolved it internally and kicked off another land attempt. |
|
This pull request was successfully merged by @janicduplessis in 9ef5107. When will my fix make it into a release? | Upcoming Releases |
…cebook#24767) Summary: Android followup for facebook#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 Pull Request resolved: facebook#24767 Differential Revision: D15279651 Pulled By: cpojer fbshipit-source-id: 2bbdc4bbcbeae8945588ac5e3e895c49e6ac9e1a
…cebook#24767) Summary: Android followup for facebook#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 Pull Request resolved: facebook#24767 Differential Revision: D15279651 Pulled By: cpojer fbshipit-source-id: 2bbdc4bbcbeae8945588ac5e3e895c49e6ac9e1a
#24405 try 2
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) toBlobinstances. This way when theBlobis collected, theBlobCollectoralso gets collected. Using thejsi::HostObjectdtor 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']))