fix: resolve __postFrameCallback crash on re-scheduling

Also adds tests

test-app/app/src/main/assets/app/mainpage.js

@@ -68,4 +68,5 @@ require("./tests/kotlin/access/testInternalLanguageFeaturesSupport");
 require("./tests/testPackagePrivate");
 require("./tests/kotlin/properties/testPropertiesSupport.js");
 require('./tests/testNativeTimers');
+require("./tests/testPostFrameCallback");
 require("./tests/console/logTests.js");

test-app/app/src/main/assets/app/tests/testPostFrameCallback.js

@@ -0,0 +1,142 @@
+describe("test PostFrameCallback", function () {
+  const defaultWaitTime = 300;
+  it("__postFrameCallback exists", () => {
+    expect(global.__postFrameCallback).toBeDefined();
+  });
+
+  it("__removeFrameCallback exists", () => {
+    expect(global.__removeFrameCallback).toBeDefined();
+  });
+
+  it("should throw when providing wrong arguments", () => {
+    expect(() => global.__postFrameCallback(null)).toThrow();
+    expect(() => global.__removeFrameCallback(null)).toThrow();
+    expect(() => global.__postFrameCallback("")).toThrow();
+    expect(() => global.__removeFrameCallback("")).toThrow();
+    expect(() => global.__postFrameCallback()).toThrow();
+    expect(() => global.__removeFrameCallback()).toThrow();
+  });
+
+  it("should call the callback once", (done) => {
+    let callCount = 0;
+    const callback = () => {
+      callCount++;
+    };
+    global.__postFrameCallback(callback);
+    setTimeout(() => {
+      expect(callCount).toBe(1);
+      done();
+    }, defaultWaitTime);
+  });
+
+  it("should call the callback once even if scheduled multiple times", (done) => {
+    let callCount = 0;
+    const callback = () => {
+      callCount++;
+    };
+    global.__postFrameCallback(callback);
+    global.__postFrameCallback(callback);
+    setTimeout(() => {
+      expect(callCount).toBe(1);
+      done();
+    }, defaultWaitTime);
+  });
+
+  it("should not trigger the callback if it was canceled", (done) => {
+    let callCount = 0;
+    const callback = () => {
+      callCount++;
+    };
+    global.__postFrameCallback(callback);
+    global.__removeFrameCallback(callback);
+    setTimeout(() => {
+      expect(callCount).toBe(0);
+      done();
+    }, defaultWaitTime);
+  });
+
+  it("should trigger the callback if it was canceled then re-scheduled", (done) => {
+    let callCount = 0;
+    const callback = () => {
+      callCount++;
+    };
+    global.__postFrameCallback(callback);
+    global.__removeFrameCallback(callback);
+    global.__postFrameCallback(callback);
+    setTimeout(() => {
+      expect(callCount).toBe(1);
+      done();
+    }, defaultWaitTime);
+  });
+
+  it("should trigger the callback if it was re-scheduled by itself", (done) => {
+    let callCount = 0;
+    const callback = () => {
+      callCount++;
+      if (callCount === 1) {
+        global.__postFrameCallback(callback);
+      }
+    };
+    global.__postFrameCallback(callback);
+    setTimeout(() => {
+      expect(callCount).toBe(2);
+      done();
+    }, defaultWaitTime);
+  });
+
+  it("should release the callback after being done", (done) => {
+    let callCount = 0;
+    let callback = () => {
+      callCount++;
+    };
+    global.__postFrameCallback(callback);
+    const weakCallback = new WeakRef(callback);
+    callback = null;
+    gc();
+    setTimeout(() => {
+      gc();
+      expect(callCount).toBe(1);
+      expect(!!weakCallback.deref()).toBe(false);
+      done();
+    }, defaultWaitTime);
+  });
+
+  it("should release the callback removal", (done) => {
+    let callCount = 0;
+    let callback = () => {
+      callCount++;
+    };
+    global.__postFrameCallback(callback);
+    global.__removeFrameCallback(callback);
+    const weakCallback = new WeakRef(callback);
+    callback = null;
+    gc();
+    setTimeout(() => {
+      gc();
+      expect(callCount).toBe(0);
+      expect(!!weakCallback.deref()).toBe(false);
+      done();
+    }, defaultWaitTime);
+  });
+
+  it("should retain callback until called", (done) => {
+    let callCount = 0;
+    let callback = () => {
+      callCount++;
+      gc();
+      expect(!!weakCallback.deref()).toBe(true);
+    };
+    global.__postFrameCallback(callback);
+    global.__removeFrameCallback(callback);
+    global.__postFrameCallback(callback);
+    const weakCallback = new WeakRef(callback);
+    callback = null;
+    gc();
+    setTimeout(() => {
+      gc();
+      expect(callCount).toBe(1);
+      expect(!!weakCallback.deref()).toBe(false);
+      done();
+    }, defaultWaitTime);
+  });
+});

test-app/runtime/src/main/cpp/CallbackHandlers.cpp

@@ -1609,7 +1609,6 @@ void CallbackHandlers::PostCallback(const FunctionCallbackInfo<v8::Value> &args,
 void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &args) {
     if (android_get_device_api_level() >= 24) {
         InitChoreographer();
-        assert(args[0]->IsFunction());
         Isolate *isolate = args.GetIsolate();
 
         v8::Locker locker(isolate);
@@ -1618,6 +1617,11 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &
         auto context = isolate->GetCurrentContext();
         Context::Scope context_scope(context);
 
+        if (args.Length() < 1 || !args[0]->IsFunction()) {
+            isolate->ThrowException(v8::Exception::TypeError(v8::String::NewFromUtf8Literal(isolate, "Frame callback argument is not a function")));
+            return;
+        }
+
         auto func = args[0].As<Function>();
 
         auto idKey = ArgConverter::ConvertToV8String(isolate, "_postFrameCallbackId");
@@ -1629,8 +1633,13 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &
             auto id = pId->IntegerValue(context).FromMaybe(0);
             auto cb = frameCallbackCache_.find(id);
             if (cb != frameCallbackCache_.end()) {
-                cb->second.removed = false;
-                PostCallback(args, &cb->second, context);
+                // check if it's already scheduled first, we don't want to schedule it twice
+                bool shouldReschedule = !cb->second.isScheduled();
+                // always mark as scheduled, as that will also mark it as not removed anymore
+                cb->second.markScheduled();
+                if (shouldReschedule) {
+                    PostCallback(args, &cb->second, context);
+                }
                 return;
             }
         }
@@ -1645,6 +1654,7 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &
         std::tie(val, inserted) = frameCallbackCache_.try_emplace(key, isolate, callback, key);
         assert(inserted && "Frame callback ID should not be duplicated");
 
+        val->second.markScheduled();
         PostCallback(args, &val->second, context);
     }
 }
@@ -1653,15 +1663,17 @@ void CallbackHandlers::RemoveFrameCallback(const FunctionCallbackInfo<v8::Value>
 
     if (android_get_device_api_level() >= 24) {
         InitChoreographer();
-        assert(args[0]->IsFunction());
         Isolate *isolate = args.GetIsolate();
-
         v8::Locker locker(isolate);
         Isolate::Scope isolate_scope(isolate);
         HandleScope handle_scope(isolate);
         auto context = isolate->GetCurrentContext();
         Context::Scope context_scope(context);
 
+        if (args.Length() < 1 || !args[0]->IsFunction()) {
+            isolate->ThrowException(v8::Exception::TypeError(v8::String::NewFromUtf8Literal(isolate, "Frame callback argument is not a function")));
+            return;
+        }
         auto func = args[0].As<Function>();
 
         auto idKey = ArgConverter::ConvertToV8String(isolate, "_postFrameCallbackId");
@@ -1673,7 +1685,7 @@ void CallbackHandlers::RemoveFrameCallback(const FunctionCallbackInfo<v8::Value>
             auto id = pId->IntegerValue(context).FromMaybe(0);
             auto cb = frameCallbackCache_.find(id);
             if (cb != frameCallbackCache_.end()) {
-                cb->second.removed = true;
+                cb->second.markRemoved();
             }
         }
 

test-app/runtime/src/main/cpp/CallbackHandlers.h

@@ -328,9 +328,21 @@ namespace tns {
 
             v8::Isolate *isolate_;
             v8::Global<v8::Function> callback_;
-            bool removed = false;
             uint64_t id;
 
+            bool isScheduled() {
+                return scheduled;
+            }
+
+            void markScheduled() {
+                scheduled = true;
+                removed = false;
+            }
+            void markRemoved() {
+                // we can never unschedule a callback, so we just mark it as removed
+                removed = true;
+            }
+
             AChoreographer_frameCallback frameCallback_ = [](long ts, void *data) {
                 execute((double)ts, data);
             };
@@ -342,7 +354,7 @@ namespace tns {
             static void execute(double ts, void *data){
                 if (data != nullptr) {
                     auto entry = static_cast<FrameCallbackCacheEntry *>(data);
-                    if (entry->removed) {
+                    if (entry->shouldRemoveBeforeCall()) {
                         frameCallbackCache_.erase(entry->id);  // invalidates *entry
                         return;
                     }
@@ -354,21 +366,42 @@ namespace tns {
                     v8::Local<v8::Function> cb = entry->callback_.Get(isolate);
                     v8::Local<v8::Context> context = cb->GetCreationContextChecked();
                     v8::Context::Scope context_scope(context);
+                    // we're running the callback now, so it's not scheduled anymore
+                    entry->markUnscheduled();
 
                     v8::Local<v8::Value> args[1] = {v8::Number::New(isolate, ts)};
 
                     v8::TryCatch tc(isolate);
 
                     cb->Call(context, context->Global(), 1, args);  // ignore JS return value
 
-                    frameCallbackCache_.erase(entry->id);  // invalidates *entry
+                    // check if we should remove it (it should be both unscheduled and removed)
+                    if (entry->shouldRemoveAfterCall()) {
+                        frameCallbackCache_.erase(entry->id);  // invalidates *entry
+                    }
+
 
                     if(tc.HasCaught()){
                         throw NativeScriptException(tc);
                     }
 
                 }
             }
+            private:
+            bool removed = false;
+            bool scheduled = false;
+            void markUnscheduled() {
+                scheduled = false;
+                removed = true;
+            }
+
+            bool shouldRemoveBeforeCall() {
+                return removed;
+            }
+
+            bool shouldRemoveAfterCall() {
+                return !scheduled && removed;
+            }
 
         };