Severe memory leak affecting fetch(), iOS Release Mode 0.59.0-rc.3, 0.58.6, 0.57.8

[cjroth]

🐛 Bug Report

Every call to fetch() uses memory and does not release it. The larger the fetch response, the more memory leaked.

• I've confirmed that this is happening in 0.57.8, 0.58.6, and 0.59.0-rc.3.
• I've tried this with both node-fetch and whatwg-fetch and it happens using both libraries.
• It is not isolated to response.blob(). It affects all types of responses.
• It happens with both Release and Debug.
• It happens with raw XMLHttpRequest as well as fetch.
• I can confirm that it's happening on a (real) iPhone X with iOS 12.1.4 and the Simulator for an iPhone 8 with iOS 12.1.

Comparing memory snapshots in Safari found no difference in size even while the Chrome/Xcode memory profiles showed dramatic increases in memory. I'm not great at debugging memory leaks so I could be wrong but I suspect that means it might be on the native side and not on the JS side.

To Reproduce

I've created an example repo to reproduce the error in 0.59.0-rc.3: https://github.com/cjroth/react-native-fetch-memory-leak. You can copy the App.js and see that it happens in previous versions as well.

Expected Behavior

After each fetch() call that is no longer referenced, the memory should return to what it was previously.

Code Example

https://github.com/cjroth/react-native-fetch-memory-leak

Environment

  React Native Environment Info:
    System:
      OS: macOS 10.14.1
      CPU: (4) x64 Intel(R) Core(TM) i7-4650U CPU @ 1.70GHz
      Memory: 16.96 MB / 8.00 GB
      Shell: 5.3 - /bin/zsh
    Binaries:
      Node: 11.10.0 - /usr/local/bin/node
      Yarn: 1.13.0 - /usr/local/bin/yarn
      npm: 6.8.0 - /usr/local/bin/npm
      Watchman: 4.9.0 - /usr/local/bin/watchman
    SDKs:
      iOS SDK:
        Platforms: iOS 12.1, macOS 10.14, tvOS 12.1, watchOS 5.1
    IDEs:
      Xcode: 10.1/10B61 - /usr/bin/xcodebuild
    npmPackages:
      react: 16.8.3 => 16.8.3 
      react-native: 0.59.0-rc.3 => 0.59.0-rc.3 
    npmGlobalPackages:
      react-native-cli: 2.0.1
      react-native-git-upgrade: 0.2.7
      react-native-rename: 2.4.0

[cjroth]

This has some similarities to other issues:

• Fetching specific JSON causes massive memory leak #20352
• fetch() leaks memory on each request due to unreleased Blobs #19248
• Vendor fetch polyfill, remove default blob response type #19333
• TypeError: Network request failed when fetching a file:// uri expo/expo#2402

[zhongwuzw]

@cjroth Hey, did you try Release mode? I remember I tested like #20352 before, it only happened in Debug mode.

[cjroth]

@cjroth Hey, did you try Release mode? I remember I tested like #20352 before, it only happened in Debug mode.

@zhongwuzw yes I mentioned that in both the title and the body of the issue. Thank you for replying though!

[jonathangreco]

@cjroth Hey do you think my issue is also related to this memory leak ?
#23477

When the response is given (and only in IOS) when I ask a permission to store a media into the gallery at almost the same time that the response is handled, my app freeze and need to be restarted.

[cjroth]

@jonathangreco no idea, I'd suggest profiling the memory. If it spikes massively before it crashes then it would make sense - if you are downloading something as large as a film through the fetch API and if this memory leak is not just me doing something silly it's likely.

[noahpryor]

I was able to reproduce using the iOS simulator and a variant of @cjroth s code with a five mb test file
here

[brentvatne]

what is the most recent release where this did not occur for you?

[cjroth]

I haven't had time to go back and trianglulate the release the caused it, not sure.

[timsawtell]

I think I've fixed it. Time to make my first PR into RN.

I found that the RCTNetworkTasks were not de-allocating, and that RCTBlobManager.mm was holding onto it's blobs (NSData) forever after telling the RCTNetworking coordination code about the response.

[timsawtell]

This seemed to fix the blob type requests:
timsawtell@236f00a

However now that I'm trying out just hitting a regular content-type: "application/json" based response the memory usage issue is still there.

I'm currently debugging that use case (which I think is far more common for apps in general) and I'll update my fork with anything I find. Since this is my first deep dive into RN networking internals, anyone with some experience please feel free to point out where I should be looking.

[timsawtell]

OK so this is a bit of a strange one with regards to solution design. The issue as I understand it is that the RCTBlobManager isn't necessarily at fault here as I first thought. I realised that the first attempt at a fix wasn't right - even though it solved the use case in this issue.

The problem is that whatwg-fetch isn't necessarily concerned with what happens to the data from a request after that request is "finished".

So RCTBlobManager keeps hold of all data from your various fetch operations in your app. It references data via a HashMap with a blobId as the key. The "bug" is that this data is not removed by the standard fetch APIs. Now that's not fetch's fault, it's not RCTBlobManager's fault - it just is :)

So I think the approach to resolving this memory consumption issue would be to tell RCTBlobManager to release it's data after it receives a "read" request on that data. By that point it will return the data back over the bridge to fetch in whatever encoding the programmer has asked for it. I am making an assumption here in saying that once a programmer has read the data in JS then they probably won't be referencing that data in RCTBlobManager again, and it would be safe to release it. The APIs aren't even there (as far as I can tell) for the programmer to even go get it again via a blobId - further reason to think this would be the safest approach.

PR coming soon after I run some more tests.

[cjroth]

Do non-blob responses such as JSON also use the blob manager?

On Sun, Mar 10, 2019 at 18:24 Tim ***@***.***> wrote: OK so this is a bit of a strange one with regards to solution design. The issue as I understand it is that the RCTBlobManager isn't necessarily at fault here as I first thought. I realised that the first attempt at a fix wasn't right - even though it solved the use case in this issue. The problem is that whatwg-fetch isn't necessarily concerned with what happens to the data from a request after that request is "finished". So RCTBlobManager keeps hold of all data from your various fetch operations in your app. It references data via a HashMap with a blobId as the key. The "bug" is that this data is not removed by the standard fetch APIs. Now that's not fetch's fault, it's not RCTBlobManager's fault - it just is :) So I think the approach to resolving this memory consumption issue would be to tell RCTBlobManager to release it's data after it receives a "read" request on that data. By that point it will return the data back over the bridge to fetch in whatever encoding the programmer has asked for it. I am making an assumption here in saying that once a programmer has read the data in JS then they probably won't be referencing that data in RCTBlobManager again, and it would be safe to release it. The APIs aren't even there (as far as I can tell) for the programmer to even go get it again via a blobId - further reason to think this would be the safest approach. PR coming soon after I run some more tests. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23801 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjh8jUrAsqZYPn_klVMVo00kX_q6odiks5vVYYPgaJpZM4biYsr> .

-- Chris Roth cjroth.com <http://www.cjroth.com>

[timsawtell]

Do non-blob responses such as JSON also use the blob manager?

Yep. Looks like it. I altered your demo to use my fork of RN, please have a look.
(clone and run)
https://github.com/timsawtell/react-native-fetch-memory-leak/
which uses RN from my fork
timsawtell@fe565e0

[cjroth]

@timsawtell seems to be crashing when I run it - here's a screenshot.

[janicduplessis]

RN is supposed to use a forked version of whatwg-fetch that doesn’t use blobs by default to work around this issue. Maybe this is not working anymore for some reason?

[janicduplessis]

Here’s roughly how the fetch polyfill should work

https://github.com/facebook/react-native/blob/master/Libraries/Core/setUpXHR.js#L23
https://github.com/facebook/react-native/blob/master/Libraries/Network/fetch.js
https://github.com/facebook/react-native/blob/master/Libraries/vendor/core/whatwg-fetch.js

[zhongwuzw]

Seems we never remove blob data from native _blobs dictionary.

@timsawtell Hi, I see your commit, I think it may break the API, could you check my commit? 🤔
zhongwuzw@a1be05e.
And seems it has another leaks besides we save the blob.

[timsawtell]

Sorry I haven’t replied on this in a while - it looks like either solution (deleting from blob manager on read, or adding a new method to do the same job) brings in implied behavior vs explicit behavior.

(zhongwuzw’s solution is the explicit option).

I’m in favor of the explicit behavior even if it adds a new API binding that’s only going to be used by the iOS implementation. Anyone from the core team have any guidance?

[coocon]

How can We solve this problem...?

[mtpjr88]

My Team recently upgraded to "0.57.2" and have noticed a significant memory leak when making Fetch() requests. Has this been resolved?
@janicduplessis @hramos

[mikecsmith]

No @mtpjr88 this was still in place after the PR which closed this issue and appears to be a long standing bug. We had to switch to Swift and build natively.

[ambarc]

Tried out @zhongwuzw's fix as a patch, and it appears to work on iOS @ RN 0.61.5.

Not sure if this matches fetch's spec - but can confirm some of the functionality.

See before/after with the same user behavior. We repeatedly poll and endpoint which generated a lot of unused (but unleaked) memory.

Memory usage before

After