Prevent native blob resource from being de-allocated prematurely by awinograd · Pull Request #31392 · react/react-native

awinograd · 2021-04-20T16:40:37Z

Summary

This PR prevents blob data from being prematurely de-allocated in native code when using slice to create views into an existing blob. Currently, whenever a new blob is created via createFromOptions, BlobManager.js creates a new blobCollector object since options.__collector is never provided.

https://github.com/facebook/react-native/blob/dc80b2dcb52fadec6a573a9dd1824393f8c29fdc/Libraries/Blob/BlobManager.js#L115-L123

When the reference to a blobCollector is garbage collected, the corresponding native memory for the blob data is de-allocated.

https://github.com/facebook/react-native/blob/27651720b40cab564a0cbd41be56a02584e0c73a/Libraries/Blob/RCTBlobCollector.mm#L19-L25

Since, blob.slice() is supposed to create a new view onto the same binary data as the original blob, we need to re-use the same collector object when slicing so that it is not GC'd until the last reference to the binary data is no longer reachable. Currently, since each blob slice gets a new blobCollector object, the memory is de-allocated when the first blob is GC'd.

Fixes#29970
Fixes#27857

Changelog

[iOS] [Fixed] - Blob data is no longer prematurely deallocated when using blob.slice

Test Plan

I could use help coming up with a test plan here. I could add a referential equality check for the blob.data.__collector in Blob-test but it doesn't seem quite right to be testing the implementation detail there.

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

analysis-bot · 2021-04-20T17:24:15Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,104,346	+41
android	hermes	armeabi-v7a	6,473,107	+40
android	hermes	x86	7,522,621	+40
android	hermes	x86_64	7,381,045	+37
android	jsc	arm64-v8a	8,971,785	+7
android	jsc	armeabi-v7a	7,703,213	+5
android	jsc	x86	9,034,393	+6
android	jsc	x86_64	9,511,776	-3

Base commit: e509007
Branch: main

analysis-bot · 2021-04-20T17:32:44Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 322a796
Branch: main

lachenmayer · 2022-08-23T19:02:39Z

Hi there, we have been facing this issue for quite a while. We are using a resumable/chunked upload client called tus which uses Blob#slice() under the hood (see tus/tus-js-client#326 for a detailed bug report).

We have been applying this patch (using patch-package) for several months now, and we can confirm that it works. Also a bit unclear how you would test something like this other than saying it has massively reduced our crash rate :)

Without the patch we consistently see crashes with a *** -[__NSPlaceholderDictionary initWithObjects:forKeys:count:]: attempt to insert nil object from objects[0] exception.

One of my team members implemented a repro here if anyone is interested: samal-rasmussen/tus-js-client-slice-bug@112843b

awinograd · 2022-12-02T23:15:14Z

@kelset sorry to bother you with a ping, but would be great to get some 👁️ on this PR. Is there anything I can do to entice a maintainer to take a look?

Since `blob.slice()` creates a new view onto the same binary data as the original blob, we should re-use the same collector object so that the underlying resource gets deallocated when the last view into the data is released, not the first.

facebook-github-bot · 2022-12-05T11:09:09Z

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pull-bot · 2022-12-05T12:01:14Z

PR build artifact for d064dfe is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

pull-bot · 2022-12-05T12:04:06Z

PR build artifact for d064dfe is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

github-actions · 2022-12-05T15:25:44Z

This pull request was successfully merged by @awinograd in 36cc71a.

^{When will my fix make it into a release? | Upcoming Releases}

awinograd · 2022-12-05T15:47:06Z

Thank you @kelset @cortinico for pushing this PR forward and getting it merged. Much appreciated!

) Summary: This PR prevents blob data from being prematurely de-allocated in native code when using slice to create views into an existing blob. Currently, whenever a new blob is created via createFromOptions, BlobManager.js creates a new blobCollector object since options.__collector is never provided. https://github.com/facebook/react-native/blob/dc80b2dcb52fadec6a573a9dd1824393f8c29fdc/Libraries/Blob/BlobManager.js#L115-L123 When the reference to a blobCollector is garbage collected, the corresponding native memory for the blob data is de-allocated. https://github.com/facebook/react-native/blob/27651720b40cab564a0cbd41be56a02584e0c73a/Libraries/Blob/RCTBlobCollector.mm#L19-L25 Since, `blob.slice()` is supposed to create a new view onto the same binary data as the original blob, we need to re-use the same collector object when slicing so that it is not GC'd until the last reference to the binary data is no longer reachable. Currently, since each blob slice gets a new blobCollector object, the memory is de-allocated when the first blob is GC'd. Fixes#29970Fixes#27857 ## Changelog  [iOS] [Fixed] - Blob data is no longer prematurely deallocated when using blob.slice Pull Request resolved: #31392 Test Plan: I could use help coming up with a test plan here. I could add a referential equality check for the blob.data.__collector in `Blob-test` but it doesn't seem quite right to be testing the implementation detail there. Reviewed By: javache Differential Revision: D41730782 Pulled By: cortinico fbshipit-source-id: 5671ae2c69908f4c9acb5d203ba198b41b421294

Summary: `FileReader.readAsText` / `readAsDataURL` / `readAsArrayBuffer` pass only the plain `blob.data` descriptor to the native module and retain no reference to the `Blob` instance itself. If the caller also drops its reference, the Blob — and the `BlobCollector` attached to `blob.data.__collector` — becomes unreachable while the native read is still in flight. When GC runs in that window, the collector's finalizer unconditionally removes the bytes from the native blob store (`BlobCollector.cpp` calls `BlobModule.remove()` on Android; `RCTBlobCollector.mm` calls `[RCTBlobManager remove:]` on iOS), and the pending read rejects with **"The specified blob is invalid"** (Android) / **"Unable to resolve data for blob"** (iOS). This is not an exotic case: React Native's fetch polyfill (whatwg-fetch) reads blob bodies exactly this way — `readBlobAsText` creates a `FileReader`, calls `reader.readAsText(blob)`, and keeps a reference only to the reader. So a plain `fetch(url).then(r => r.json())`, where the `Response` is not otherwise retained, is subject to this race. This matches the symptom profile of #56884: intermittent failures under many concurrent fetches (GC pressure plus native-module thread-hop latency), affecting both platforms, and disappearing when the same flow is rewritten with `async`/`await` — the suspended frame keeps the `Response` (and therefore the Blob and its collector) reachable, which is exactly the reference this fix restores. The fix retains the Blob on the FileReader instance until the native read settles, completing the reference chain: pending native promise → callbacks → reader → `_blob` → Blob → collector. The reference is cleared when the current read settles (after the existing read-id staleness check, so a read abandoned by `abort()` cannot drop a newer read's reference) and in `abort()` itself, before the abort event is dispatched, so a read started from an abort handler is retained correctly. Memory impact is negligible: the native bytes must live until the read completes anyway — this change only guarantees they do. The root cause is in the shared JS layer, so both Android and iOS are fixed. Fixes#56884 Related prior art: #31392 fixed a different premature-deallocation path in the same subsystem (`blob.slice()` creating a second collector for the same blobId). ## Changelog: [GENERAL] [FIXED] - Retain Blob reference in FileReader during pending native reads to prevent premature deallocation by BlobCollector Pull Request resolved: #57796 Test Plan: - `yarn jest packages/react-native/Libraries/Blob/__tests__/FileReader-test.js` — 19 passed, including 4 new tests: the blob is retained while a read is pending, released on resolve / reject / `abort()`, and a stale read settling after abort does not drop a newer read's blob. - `yarn flow check` — no errors. `eslint` on both changed files — clean. - The GC race itself cannot be reproduced deterministically under Jest (it requires a real engine GC collecting the Blob between dispatch and native execution), so the unit tests assert the reference-retention behavior instead. A deterministic on-device repro is in the issue comment below / #56884. Reviewed By: javache Differential Revision: D114576384 Pulled By: fabriziocucci fbshipit-source-id: ed3f5b51f2d246e041c2b178e3eadd58aeb70038

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 20, 2021

analysis-bot suggested changes Apr 20, 2021
View reviewed changes

Comment threadLibraries/Blob/Blob.js Outdated
Comment threadLibraries/Blob/Blob.js Outdated

react-native-bot added Bug Platform: iOS iOS applications. labels Oct 1, 2021

This was referenced Mar 10, 2022
iOS Native Crash when uploading blobs using aws-sdk #27857
Closed
BUG: Exception when using chunking with react-native on ios tus/tus-js-client#326
Closed

kelset added the Needs: React Native Team Attention label Mar 28, 2022

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Mar 28, 2022

awinograd added 2 commits December 5, 2022 11:56

fix trailing spacing
d064dfe

facebook-github-bot closed this in 36cc71aDec 5, 2022

github-actionsBot added the Merged This PR has been merged. label Dec 5, 2022

awinograd deleted the patch-1 branch December 5, 2022 15:47

abdulmomin703 mentioned this pull request Apr 5, 2023
Upload failed if app is running in the background in React Native tus/tus-js-client#179
Open

snyk-bot mentioned this pull request Apr 19, 2023
[Snyk] Upgrade react-native from 0.63.3 to 0.71.5 NOUIY/react-native-macos#1034
Open

NOUIY mentioned this pull request Apr 25, 2023
[Snyk] Upgrade react-native from 0.63.3 to 0.71.6 NOUIY/react-native-macos#1039
Open

cipolleschi mentioned this pull request Aug 16, 2023
cipolleschi/add maven polling #39039
Merged

cipolleschi mentioned this pull request Sep 21, 2023
[LOCAL] Copy hermes ruby files to hermes folder in local E2E test #39577
Merged

cipolleschi mentioned this pull request Oct 11, 2023
cipolleschi/fix CI #40793
Merged

This was referenced Nov 1, 2023
[RN][CI]Update node installation on debian (0.71) #41277
Merged
[RN][CI]Update node installation on debian (0.70) #41278
Closed

cipolleschi mentioned this pull request Dec 19, 2023
[Local] Bump hermes-engine #41991
Merged

This was referenced Jan 8, 2024
[0.71][RN][General] Migrate boost download url away from JFrog #42177
Merged
[0.70][RN][General] Migrate boost download url away from JFrog #42178
Merged
[RN][iOS] Fix symbol not found _jump_fcontext with use_frameworks! #42229
Merged

cipolleschi mentioned this pull request Mar 6, 2024
[RN][iOS][0.71] Fix flipper for Xcode 15.3 #43345
Merged

cipolleschi mentioned this pull request Apr 5, 2024
cipolleschi/fix yoga archive 0.71 #43895
Closed

cipolleschi mentioned this pull request Apr 26, 2024
[iOS][Privacy Manifest] Set up privacy manifest for React Native 0.71 #44281
Merged

heecheolman mentioned this pull request Aug 3, 2026
Fix premature blob deallocation during FileReader reads #57796
Closed

Navigation Menu

Uh oh!

Prevent native blob resource from being de-allocated prematurely - #31392

Prevent native blob resource from being de-allocated prematurely#31392
awinograd wants to merge 2 commits into
react:mainfrom
awinograd:patch-1

awinograd commented Apr 20, 2021

Uh oh!

analysis-bot left a comment

Uh oh!

Uh oh!

Uh oh!

analysis-bot commented Apr 20, 2021•
edited
Loading

Uh oh!

analysis-bot commented Apr 20, 2021•
edited
Loading

Uh oh!

lachenmayer commented Aug 23, 2022

Uh oh!

awinograd commented Dec 2, 2022

Uh oh!

facebook-github-bot commented Dec 5, 2022

Uh oh!

pull-bot commented Dec 5, 2022

Uh oh!

pull-bot commented Dec 5, 2022

Uh oh!

github-actionsBot commented Dec 5, 2022

Uh oh!

awinograd commented Dec 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

awinograd commented Apr 20, 2021

Summary

Changelog

Test Plan

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

analysis-bot commented Apr 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot commented Apr 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lachenmayer commented Aug 23, 2022

Uh oh!

awinograd commented Dec 2, 2022

Uh oh!

facebook-github-bot commented Dec 5, 2022

Uh oh!

pull-bot commented Dec 5, 2022

Uh oh!

pull-bot commented Dec 5, 2022

Uh oh!

github-actionsBot commented Dec 5, 2022

Uh oh!

awinograd commented Dec 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

analysis-bot commented Apr 20, 2021•
edited
Loading

analysis-bot commented Apr 20, 2021•
edited
Loading