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

Created on 7 Mar 2019 · 22Comments · Source: facebook/react-native

🐛 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

screen shot 2019-03-06 at 9 22 40 pm

Bug iOS Locked 🌐Networking

Source

cjroth

👍11 👀2

Most helpful comment

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 on 10 Mar 2019

👍8

All 22 comments

This has some similarities to other issues:

cjroth on 7 Mar 2019

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

zhongwuzw on 7 Mar 2019

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

cjroth on 7 Mar 2019

@cjroth Hey do you think my issue is also related to this memory leak ?
https://github.com/facebook/react-native/issues/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.

jonathangreco on 7 Mar 2019

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

cjroth on 7 Mar 2019

I was able to reproduce using the iOS simulator and a variant of @cjroth s code with a five mb test file
here
screenshot 2019-03-07 21 12 39

noahpryor on 8 Mar 2019

❤7

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

brentvatne on 9 Mar 2019

👍3

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

cjroth on 9 Mar 2019

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 on 10 Mar 2019

👍8

This seemed to fix the blob type requests:
https://github.com/timsawtell/react-native/commit/236f00a30a6f529f9c69088da3dd6cda1893d4be

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 on 10 Mar 2019

👍3

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.

timsawtell on 10 Mar 2019

👍1

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

On Sun, Mar 10, 2019 at 18:24 Tim notifications@github.com 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
https://github.com/facebook/react-native/issues/23801#issuecomment-471360105,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAjh8jUrAsqZYPn_klVMVo00kX_q6odiks5vVYYPgaJpZM4biYsr
.

>

Chris Roth
cjroth.com http://www.cjroth.com

cjroth on 11 Mar 2019

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
https://github.com/timsawtell/react-native/commit/fe565e0ee555aba7c96fe5123c908eb78f5c0863

timsawtell on 11 Mar 2019

@timsawtell seems to be crashing when I run it - here's a screenshot.
Screen Shot 2019-03-10 at 8 27 32 PM

cjroth on 11 Mar 2019

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 on 11 Mar 2019

👍1

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

janicduplessis on 11 Mar 2019

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? 🤔
https://github.com/zhongwuzw/react-native/commit/a1be05e8eb85510f4e7d8ec10dafd3c6a8648f6a.
And seems it has another leaks besides we save the blob.

zhongwuzw on 12 Mar 2019

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?

timsawtell on 21 Mar 2019

How can We solve this problem...?

coocon on 1 Apr 2019

😕1

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

mtpjr88 on 23 Oct 2019

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.