react-native-v8 icon indicating copy to clipboard operation
react-native-v8 copied to clipboard

fetch() BlobModule.java memory leak fix not working if enable v8 js engine

Open jgreen210 opened this issue 5 years ago • 5 comments

We've been seeing memory leaks from the mBlobs HashMap in BlobModule.java for some time (with JavaScriptCore). This is one of the tickets for this:

facebook/react-native#20352

We were expecting this to be fixed when we upgraded react native, since 0.61.3 added the following fix:

facebook/react-native@766f470

I.e. the js blob HostObject destructor is supposed to call remove() in BlobModule.java.

We found that, while this is fixed for JavaScriptCore, if we enabled the v8 engine (to avoid various native crashes we've been seeing in JavaScriptCore's libjsc.so), we were still getting java-heap out of memory errors. I.e. the fetch() memory leak is fixed by react native now, but only if use JavaScriptCore.

I created a small app that demonstrates this problem:

https://github.com/jgreen210/ReactNativeFetchBlobLeak/tree/v8

Setting breakpoints in BlobModule.java in android studio is a useful way to debug this:

https://github.com/facebook/react-native/blob/v0.61.5/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobModule.java#L172 https://github.com/facebook/react-native/blob/v0.61.5/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobModule.java#L184

I've not tried this for v8, although have used it for the hermes js engine, where there is a similar problem (https://github.com/facebook/hermes/issues/164). For hermes, BlobModule.java's remove() method is not getting called.

react native 0.61.5

jgreen210 avatar Jan 08 '20 12:01 jgreen210

@jgreen210 V8 manages its GC lifecycle internally. From my past experience, it will not GC very frequently. Since Blob is a JSI hosted object and owns an underlying native resources. JS engine has no memory usage for these native resources. If V8 does not do GC timely, the native resources may exceed the limit.

JavaScript, unfortunately, do not have RAII support. In the meantime, please try if Blob.close() helps for your problem. From your sample code:

    fetch(url)
      .then(response => response.blob())
      .then(blob => {
        const size = blob.size;
        blob.close();
        return size;
      })
      .then(bytes => {
        setTotalBytes(totalBytes + bytes);
      });

Or maybe you try async/await + try and finally.

Kudo avatar Jan 10 '20 12:01 Kudo

It didn't check whether this was just caused by less frequent garbage collections in v8 than JavaScriptCore. I did file https://github.com/facebook/react-native/issues/27532 though, which might help in this situation.

If close() helps (I've not tried it or looked at what it does), would have to get react native to fix its json() implementation - it uses blob() internally.

jgreen210 avatar Jan 10 '20 13:01 jgreen210

I can confirm that adding blob.close() can fix your OOM crash problem from your demo code.

As mentioned earlier, JS VM has no idea about how much memory cost for underlying JSI native resources, so it may not do GC timely and cause OOM crash. JavaScriptCore AFAIK, will check the ratio of used memory and available memory frequently and do GC. V8 seems not have similar design.

I would highly suggest applications to call blob.close(), free unused resources timely and not rely on JS GC mechanism.

BTW I have an experiment to patch fetch that will automatically call blob.close() at the end of promise chain. That is totally for fun, with some limitations and not practical in production code. You may take a look at this commit. https://github.com/Kudo/react-native/commit/86ac876e50bba71091f08660fdbc08cc239421a0

Kudo avatar Mar 05 '20 03:03 Kudo

The objects are alive in the Hermes heap so more frequent GCs will not help. Manually freeing the blobs is a good workaround, but the associated XMLHttpRequests are still accumulating. I'm still trying to work out the specific mechanics of this.

willholen avatar Mar 06 '20 18:03 willholen

@willholen - did you mean hermes in your above comment? (Hermes and v8 have the same symptoms, perhaps with the same cause, but this is an issue in the react-native-v8 repo.)

jgreen210 avatar Mar 10 '20 11:03 jgreen210