Prevent native blob resource from being de-allocated prematurely

[awinograd]

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.

react-native/Libraries/Blob/BlobManager.js

Lines 115 to 123 in dc80b2d

	// Reuse the collector instance when creating from an existing blob.
	// This will make sure that the underlying resource is only deallocated
	// when all blobs that refer to it are deallocated.
	options.__collector == null
	? {
	...options,
	__collector: createBlobCollector(options.blobId),
	}
	: options,

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

react-native/Libraries/Blob/RCTBlobCollector.mm

Lines 19 to 25 in 2765172

	RCTBlobCollector::~RCTBlobCollector() {
	RCTBlobManager *blobManager = blobManager_;
	NSString *blobId = [NSString stringWithUTF8String:blobId_.c_str()];
	dispatch_async([blobManager_ methodQueue], ^{
	[blobManager remove:blobId];
	});
	}

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]

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]

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

Base commit: 322a796
Branch: main

[lachenmayer]

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]

@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?

[facebook-github-bot]

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

[pull-bot]

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]

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]

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

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

[awinograd]

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