Android memory leak due to JSC

[ben-manes]

I apologize for not providing this report sooner or with detailed charts. We have been using a patched version of JSC for a few months now and believe RN should drive its resolution. I did not investigate or resolve it, but the author prefers to remain anonymous and uninvolved. The provided patch had a significant impact for a long running application on a low end device.

Environment

This occurs on all versions of Android and RN. It requires patching JavaScript Core and using that version.

Description

A full GC is performed only when Heap#overCriticalMemoryThreshold returns true or if the default is changed to disable the generational GC. Otherwise only the young generation is collected. Unfortunately, overCriticalMemoryThreshold is only implemented for iOS and always returns false for other platforms. This means that any objects promoted from young to old are not GC'd and the app will eventually crash due to an out of memory error.

Please note that running RN through the Chrome debugger does not use JSC. This will use Chrome's JS engine and promptly reclaim memory. We confirmed this bug and fix by reducing the maximum memory for JSC, observing the leak, and then healthy behavior when resolved.

Patch

The least invasive change was to force a full collection and we are satisfied with the resulting performance. In our application, we did not experience long pause times that would result in a negative experience. We changed overCriticalMemoryThreshold to return true on non-iOS platforms and believe that is the correct default behavior. A more advanced solution would be to add Android support to calculate if the threshold was crossed.

iff --git a/webkit/Source/JavaScriptCore/heap/Heap.cpp b/webkit/Source/JavaScriptCore/heap/Heap.cpp
index ffeac67d..9dcf4113 100644
--- a/webkit/Source/JavaScriptCore/heap/Heap.cpp
+++ b/webkit/Source/JavaScriptCore/heap/Heap.cpp
@@ -498,7 +498,7 @@ bool Heap::overCriticalMemoryThreshold(MemoryThresholdCallType memoryThresholdCa
     return m_overCriticalMemoryThreshold;
 #else
     UNUSED_PARAM(memoryThresholdCallType);
-    return false;
+    return true;
 #endif
 }

Resolution

We would not be the appropriate party to coordinate a long term fix. If we went to JSC directly, RN would still have to upgrade its dependency (4yrs old, iirc). Please drive this issue to a happy conclusion.

[hramos]

The JSC dependency was updated back in December (f3e5cce), and is scheduled to be part of the 0.59 release. Can you verify if the issue is present in the updated JSC?

[ben-manes]

Yes, the linked code is for the latest version of JSC. When investigating this issue, we tried upgrading to the latest when discovering it was a JS GC issue, but not knowing the cause. Our custom version of JSC was built (& up to date) on Dec 18, 2018 when we moved to bintray rather than relying on locally built copies for the deployment process.

[akshetpandey]

@hramos: Can we remove the Old Version tag. This impacts the version of JSC bundled in master and is likely a blocker for 0.59 release.

Here is the JSC code as compiled on master:

https://trac.webkit.org/browser/webkit/releases/WebKitGTK/webkit-2.22.2/Source/JavaScriptCore/heap/Heap.cpp#L516

Latest stable release also doesn't have it implemented:

https://trac.webkit.org/browser/webkit/releases/WebKitGTK/webkit-2.23.3/Source/JavaScriptCore/heap/Heap.cpp#L516

[akshetpandey]

As far as I can tell, the version of JS bundled with react native before the JSC upgrade doesn't have this problem, but all versions of npm/jsc-android after 224109.0.0 does have this problem. Which would imply expo also has this issue.

[akshetpandey]

@ben-manes : Can you also create an issues on https://github.com/react-native-community/jsc-android-buildscripts

[ben-manes]

@akshetpandey why would the bundled JS not have this problem? Does 0.59 replace JSC with a different implementation or by chance disable generational GC?

|    +--- com.facebook.react:react-native:+ -> 0.58.3
|    ...
|    \--- org.webkit:android-jsc:r174650 -> com.withvector:android-jsc:r225067

I'll create the issue over there and cc you.

[akshetpandey]

Sorry, I meant the version before JSC was upgraded.

[ben-manes]

We found this problem in 0.53 and are currently upgrading to 0.58.3. We were using the stock JSC and tried upgrading via jsc-android-buildscripts when debugging. It appeared to us to be a long standing issue from the webkit commit history.

[guhungry]

@ben-manes According to react-native-community/jsc-android-buildscripts#82 (comment). I think this issue can be closed?

[ben-manes]

Okay