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 (#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 #19333

Differential Revision: D8125463

Pulled By: hramos

fbshipit-source-id: 8f4602190dfc2643606606886c698e8e9b1d91d1
  • Loading branch information
janicduplessis authored and facebook-github-bot committed May 29, 2018
1 parent 2bf4755 commit 122b379
Show file tree
Hide file tree
Showing 2 changed files with 520 additions and 1 deletion.
Loading

0 comments on commit 122b379

Please sign in to comment.