Promise.resolve and Promise.reject causes memory leak on iOS.

[schnapzz]

Simply referencing another issue posted on the Nativescript repo but it seem to be an issue with the v8 ios runtime so I'll post it here as it seem a very important issue to be fixed.

NativeScript/NativeScript#9151.

[delaneyb]

@darind This part of PromiseProxy seems to be be the culprit: https://github.com/NativeScript/ns-v8ios-runtime/blob/92195e8f7c02aa50a97d686757e0b04070c78ef7/NativeScript/runtime/PromiseProxy.cpp#L18-L26

Replacing with

let promise = new target(origFunc)

or commenting out the script in PromiseProxy entirely fixed the leak for me

Any immediate ideas?

[rigor789]

The reason this code exists is to ensure the promise callbacks are always executed on the same thread where they are constructed. Though I don't quite understand the inner Proxy by just reading the code, will need to run & see what it does.

[delaneyb]

Not exactly clear what situation it becomes useful as long as V8 locking is used properly, and I'm not sure it works fully as intended using try-catch blocks in async functions (.catch() will get invoked on the same thread but I think the mechanics of try catch in terms of execution moving to the catch block are different)

Anyway, I dug further, this triggers the same memory leak:

let count = 0
function cb () {
    ++count
}

setInterval(() => {
    let runloop = CFRunLoopGetCurrent();
    for (var i = 0; i < 100; i++) {
        CFRunLoopPerformBlock(runloop, kCFRunLoopDefaultMode, cb);
        CFRunLoopWakeUp(runloop);
    }
    console.log(count);
}, 50);

It's the cb being passed to CFRunLoopPerformBlock and how it's processed from there I believe... My guess would be that it's a challenge for the runtime to know when CFRunLoopPerformBlock is done invoking cb, and hence when to clean up anything allocated...

Is there a free some where to go with this, which is getting invoked as part of translating the cb so it can be invoked by ObjC: https://github.com/NativeScript/ns-v8ios-runtime/blob/8409f9af84de73e2cd00cbeebce3ba4b53431a6e/NativeScript/runtime/Interop.mm#L70

[delaneyb]

9721e19 resolves a leak which comes from the runtime handling the second arg to CFRunLoopPerformBlock, kCFRunLoopDefaultMode, but that's unrelated to the bulk of the unreleased allocs, which are made in Interop.mm. Instruments points to these lines for the remaining allocs that aren't being released:

Lines 347, 348 and 350 in Interop::WriteValue, called by Interop::SetFFIParams:
https://github.com/NativeScript/ns-v8ios-runtime/blob/8409f9af84de73e2cd00cbeebce3ba4b53431a6e/NativeScript/runtime/Interop.mm#L345-L352

Line 61 in Interop::CreateMethod:
https://github.com/NativeScript/ns-v8ios-runtime/blob/8409f9af84de73e2cd00cbeebce3ba4b53431a6e/NativeScript/runtime/Interop.mm#L61

and (I think this might just be the actual call to new):
NativeScriptstd::__1::__libcpp_allocate(unsigned long, unsigned long) + 27 at new:253:10, address = 0x000000010330e2e3`

So looks like the memory does have to do with wrapping the passed js function so it can be invoked like a regular ObjC block? Not sure where to go from here to solve this but I assume this effects any native function passed a JS function to a parameter that takes a ObjC block.

[darind]

@delaneyb, great job in pinpointing this issue! I have included your suggested fix for the SymbolLoader memory leak in the V8 update PR. After applying it, I was able to run your sample snippet without uncontrolled memory increase and Instruments showing no leaks:

[schnapzz]

@darind Sounds awesome! Hopefully this fix will be released for the runtime soon as it is a critical issue for my companys app to work.
Any idea what time frame for such a PR to become part of the released runtime?

[rigor789]

@schnapzz we'll be releasing a pre-release later today! 🚀

Update: released v7.1.2-rc.0

[schnapzz]

The memory leak seem to be handled, but memory still seem to not be released. Using the same code as in the original issue post

  var dispatch = function() {
    setTimeout(function() {
      for(var i=0; i < 1000; i++) {
        var p = new Promise((resolve, reject) => { resolve() })
      }
      dispatch()
    }, 50)
  }
  dispatch()

instruments doesn't register leaks but memory still keep increasing and is never released.

[rigor789]

let count = 0
function cb () {
    ++count
}

setInterval(() => {
    let runloop = CFRunLoopGetCurrent();
    for (var i = 0; i < 100; i++) {
        CFRunLoopPerformBlock(runloop, kCFRunLoopDefaultMode, cb);
        CFRunLoopWakeUp(runloop);
    }
    console.log(count);
}, 50);

@schnapzz I guess one of the issues has been fixed - in the above scenario. Perhaps the PromiseProxy still needs some work.

Depending on your use - it may be fine to roll with it as-is in the meantime, or perhaps you could use 6.5.4 (the jsc version) until this is fixed.

[schnapzz]

@rigor789 Thanks I'll test if it works with the downgraded version tomorrow when time allows for it. And thanks for all the attention :-)

Update: Downgrading the iOS runtime to 6.5.4 seem to have solved our critical issue with the video download! It's now sent out for testing and it looks promising!

[delaneyb]

The only noticeable memory leak triggered by my original snippet is patched by 9721e19.

Unfortunately afterwards I discovered that when testing that patch on the original issue NativeScript/NativeScript#9151, the patch only fixed a tiny portion of the memory leaked (the rest I identified in #100 (comment)). This snippet accurately reproduces the eventual call made when Promises are resolved/rejected, causing the larger memory leak:

setInterval(() => {
    for (var i = 0; i < 100; i++) {
        CFRunLoopPerformBlock(CFRunLoopGetCurrent(), kCFRunLoopDefaultMode, () => {});
        CFRunLoopWakeUp(CFRunLoopGetCurrent());
    }
}, 50);

The difference being passing () => {} rather than re-using the function cb every time it is invoked. cb basically got cached so that what I explain next only happens once.

The memory being leaked by each call to CFRunLoopPerformBlock here is hard to solve. A special wrapper around the passed () => {} is allocated in memory and passed to CFRunLoopPerformBlock in place of the JS function, which when called by native invokes our JS function - but how do we know when the native function is done calling that wrapper function we passed it so we can free the memory it's using? At the moment nothing is malfunctioning, there is simply no logic in place to handle the disposal at all. I'm experimenting with different block and smart pointer syntax and will try and understand if/how this was done in the JSC/Android builds but I am a bit out of my depth trying to solve this.

It's possible this has always been an issue and PromiseProxy which was introduced in the V8 build has made it obvious because it means memory is leaked every time a promise is resolved or rejected, as opposed to just when a plugin uses a native function that takes a block as an argument. PromiseProxy doesn't seem to be necessary in my app - my plugins all function fine without it's help making sure Promise chains continue executing on the same thread, but it would really be sweeping the problem under the rug to remove that.

[delaneyb]

Together with 9721e19, these totally fix the memory leak for me: master...delaneyb:fix-JSBlock-leak

There were definitely issues with:

1. ffi_closure_free never being called for https://github.com/NativeScript/ns-v8ios-runtime/blob/d3eecf29633a130329fdf6302f86241d245cd1e4/NativeScript/runtime/Interop.mm#L61 which I have fixed up for this one Block scenario.

2. The BlockWrapper object never getting destroyed, even if the kJSBlockDescriptor's dispose callback was executed.

Both of which my changes seem to fix.

However I just noticed #83, which I assume my use of CFAutoRelease would basically revert 😪....

A PR with the fixes for 1. and 2. will do nothing without the CFAutoRelease or another solution which ensures the JSBlock actually gets released and disposed.

[epaterlini]

Hello @delaneyb , I confirm that your commits in relation to this issue totally fix the problem both on Simulator and on real device.
I have put the app under stress test and no leaks occur.
However as mentioned in #83 I did have some violent crashes too...but one time the message was "App closes due to memory issues" so let's hope it was for the memory being leaked (at least in my case).
I will watch closely for the issue mentioned in #83 in case I have more info.
I also tried to dig in v8-runtime but it is quite complicated...I will do my best to help.

In the meantime, a big thank you!

[delaneyb]

Hello @delaneyb , I confirm that your commits in relation to this issue totally fix the problem both on Simulator and on real device.
I have put the app under stress test and no leaks occur.
However as mentioned in #83 I did have some violent crashes too...but one time the message was "App closes due to memory issues" so let's hope it was for the memory being leaked (at least in my case).
I will watch closely for the issue mentioned in #83 in case I have more info.
I also tried to dig in v8-runtime but it is quite complicated...I will do my best to help.

In the meantime, a big thank you!

Ah, hold on a minute - perhaps #83 CAN be reverted when combined with these other 2 fixes, because prior to #83, the 2 other leaks I fixed would have still been occuring, so by taking all of the fixes together, everything including those issues from #83 should be solved! Let me just see what happens if I try to reproduce 83 with and without those 2 other fixes to confirm that it's stable only with all of them together...