Vendor fetch polyfill, remove default blob response type

[janicduplessis]

While investigating an issue about blobs (#18223), I noticed that the fetch polyfill (https://github.com/github/fetch) uses blobs as the response type by default if the module is available (https://github.com/github/fetch/blob/master/fetch.js#L454). This surfaced some issue with the blob implementation on iOS that has since been fixed.

However after further review of the fetch polyfill and the way Blobs work in RN, I noticed a major issue that causes blobs created by fetch to leak memory. This is because RN blobs are not deallocated automatically like in the browser (see comment https://github.com/facebook/react-native/blob/master/Libraries/Blob/Blob.js#L108) and the fetch polyfill does not deallocate them explicitly using the close method.

Ideally we should implement automatic blob cleanup when the instance is garbage collected but implementing that is probably somewhat complex as it requires integrating with JSC. For now I suggest disabling the default handling of requests as blobs in the fetch polyfill. This will mitigate the issue for people not using Blobs directly. I'm not sure how well documented the Blob module is but we should make it clear that they currently require explicit deallocation with the close method for people using them directly.

Test Plan

Run a simple http request using fetch and make sure it does not use the Blob module anymore.

Release Notes

[GENERAL] [BUGFIX] [fetch] - Do not use blobs to handle responses in the fetch polyfill, fixes potential memory leak.

[janicduplessis]

cc @satya164 @hramos

[janicduplessis]

Not sure of the best way to include the license here, it is also MIT so it shouldn't be a problem.

[hramos]

I'm checking with our open source team on how to proceed with licensing here.

[pull-bot]

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS

[rozele]

@janicduplessis - maybe it will be easier to just fix the issue to begin with? I was thinking about how we would implement this in react-native-windows, and I came across the JsObjectBeforeCollectCallback in ChakraCore, which would allow us to attach finalizers to JS object:
https://github.com/Microsoft/ChakraCore/wiki/JsObjectBeforeCollectCallback

My thought is, in the BlobModule for react-native, we add a new native callback for react-native, e.g., nativeFinalizer that takes an object ref and a function ref. Internally, this would maintain some native finalizer mapping, wire up the call to JsObjectBeforeCollectCallback (or in the case of JSC, JsObjectFinalizeCallback), and invoke the JavaScript function just before garbage collection. This would allow us to wire up a finalizer on Blob objects that would make sure native blobs are cleaned up. Consequently, this could also help support an interesting ErrorModule for promises that would allow us to hold on to native exception objects while a rejected Promise is still being handled, so the user could query the ErrorModule for additional native exception details.

[janicduplessis]

@rozele Makes sense, I don't have time to work on implementing this at the moment though, I suggest we still land this to fix the issue ASAP and we can work on implementing finalizers after. I think I'd rather not use blobs when using fetch anyway since RN already has optimized native methods for loading the different content types.

[facebook-github-bot]

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

[janicduplessis]

Ping @hramos

[facebook-github-bot]

@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[hramos]

Not sure why this PR remains stuck. It's not even making it into our Phabricator instance.

[shyaniv7]

This PR never made it into 0.55 or 0.56 and this is still an issue.

Can anyone merge it?
Thanks in advance

[hramos]

@shyaniv7 this fix is clearly in 0.56, as you can see in the list of tags in the commit that closed the PR: 122b379.

[jamesmoss]

This change has broken fetch with blobs in 0.57 on iOS.

If we read a jpeg from disk via fetch the Blob we're getting back is now roughly 2x the file size and the contents can't be read by any image editor:

const response = await fetch('/path/to/some/file/on/disk');
const blob = await response.blob();

console.log(blob.size); // incorrect value returned

After some investigation, this block being removed from the fetch polyfill when it was copied across has caused this:

if ('responseType' in xhr && support.blob) {
  xhr.responseType = 'blob'
}

I'm putting together a proper bug report with example code but leaving this here for now in case anybody else is hitting this issue.

[sjchmiela]

Hey, @jamesmoss, dunno if you're still suffering from this issue, but I found a workaround how to get the blob and be able to send it somewhere, like Firebase! I've described it in length here: expo/expo#2402 (comment). I hope it helps you or anyone solve the problem!