Skip to content

Commit

Permalink
Vendor fetch polyfill, remove default blob response type
Browse files Browse the repository at this point in the history
Summary:
While investigating an issue about blobs (facebook#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.

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

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

Differential Revision: D8125463

Pulled By: hramos

fbshipit-source-id: 8f4602190dfc2643606606886c698e8e9b1d91d1
  • Loading branch information
janicduplessis authored and ide committed Jul 11, 2018
1 parent 0b4c6d1 commit 35ac3e7
Show file tree
Hide file tree
Showing 2 changed files with 520 additions and 1 deletion.
Loading

0 comments on commit 35ac3e7

Please sign in to comment.