From 6a533ce58b22b163d888c51f203be2ef3aa98347 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Fri, 22 Sep 2023 16:34:50 -0300 Subject: [PATCH] fix: resolve __postFrameCallback crash on re-scheduling Also adds tests --- test-app/app/src/main/assets/app/mainpage.js | 1 + .../assets/app/tests/testPostFrameCallback.js | 142 ++++++++++++++++++ .../runtime/src/main/cpp/CallbackHandlers.cpp | 24 ++- .../runtime/src/main/cpp/CallbackHandlers.h | 39 ++++- 4 files changed, 197 insertions(+), 9 deletions(-) create mode 100644 test-app/app/src/main/assets/app/tests/testPostFrameCallback.js diff --git a/test-app/app/src/main/assets/app/mainpage.js b/test-app/app/src/main/assets/app/mainpage.js index 3b6b4f6f3..287bf2cb8 100644 --- a/test-app/app/src/main/assets/app/mainpage.js +++ b/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"); \ No newline at end of file diff --git a/test-app/app/src/main/assets/app/tests/testPostFrameCallback.js b/test-app/app/src/main/assets/app/tests/testPostFrameCallback.js new file mode 100644 index 000000000..59ead004d --- /dev/null +++ b/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); + }); +}); diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp index 65e408e24..d86348e94 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp @@ -1609,7 +1609,6 @@ void CallbackHandlers::PostCallback(const FunctionCallbackInfo &args, void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo &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 & 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(); auto idKey = ArgConverter::ConvertToV8String(isolate, "_postFrameCallbackId"); @@ -1629,8 +1633,13 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo & 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 & 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 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(); auto idKey = ArgConverter::ConvertToV8String(isolate, "_postFrameCallbackId"); @@ -1673,7 +1685,7 @@ void CallbackHandlers::RemoveFrameCallback(const FunctionCallbackInfo auto id = pId->IntegerValue(context).FromMaybe(0); auto cb = frameCallbackCache_.find(id); if (cb != frameCallbackCache_.end()) { - cb->second.removed = true; + cb->second.markRemoved(); } } diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.h b/test-app/runtime/src/main/cpp/CallbackHandlers.h index 8c7f819c2..0ca90e7ac 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.h +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.h @@ -328,9 +328,21 @@ namespace tns { v8::Isolate *isolate_; v8::Global 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(data); - if (entry->removed) { + if (entry->shouldRemoveBeforeCall()) { frameCallbackCache_.erase(entry->id); // invalidates *entry return; } @@ -354,6 +366,8 @@ namespace tns { v8::Local cb = entry->callback_.Get(isolate); v8::Local 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 args[1] = {v8::Number::New(isolate, ts)}; @@ -361,7 +375,11 @@ namespace tns { 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); @@ -369,6 +387,21 @@ namespace tns { } } + private: + bool removed = false; + bool scheduled = false; + void markUnscheduled() { + scheduled = false; + removed = true; + } + + bool shouldRemoveBeforeCall() { + return removed; + } + + bool shouldRemoveAfterCall() { + return !scheduled && removed; + } };