Remove weak callback from __postFrameCallback cache (#1755)

* feat: Remove NDK profiler

BREAKING CHANGE: __startNDKProfiler() and __stopNDKProfiler() global
bindings no longer available.

The NDK profiler was not functional, since nothing in the build process
defined the NDK_PROFILER_ENABLED preprocessor symbol. The start and stop
functions were already no-ops.

* chore: Remove outdated comment

RunMicrotasks no longer exists, it's already been replaced in the code
with PerformMicrotaskCheckpoint.

* chore: Use unique_ptr for NewBackingStore in byte buffers

V8 doc: https://docs.google.com/document/d/1sTc_jRL87Fu175Holm5SV0kajkseGl2r8ifGY76G35k/edit

The V8 usage examples show unique_ptr here; it probably doesn't matter
much, but we don't need the backing store after creating the ArrayBuffer,
and I believe shared_ptr is slightly more overhead than unique_ptr.

For convenience, replace the manual empty deleter for direct buffers with
v8::BackingStore::EmptyDeleter.

* chore: Remove weak finalizer callback from __postFrameCallback()

Weak finalizer callbacks are going away in V8 11.x, so we have to remove
this one. Luckily, a finalizer callback is not necessary - it's never
needed to prevent the frame callback from being collected.

However, a weak callback is not necessary in the first place. We can just
clean up the cache entry after the callback is executed, since it is only
executed once.

Note that the call to erase() destructs the FrameCallbackCacheEntry
instance, making `entry` a dangling pointer, so don't use it after the
erase(). There's probably a safer way to do this, although the way that
first occurred to me (pass the key to the callback instead of the entry,
and then use std::unordered_map::extract()) is not available on
robin_hood::unordered_map.

* fix: Make sure frame callbacks are not ignored

There was a bug where __postFrameCallback() would not always cause the
callback to be called. Without initializing `removed`, sometimes it would
have a nonzero value, so the callback would be ignored.

* chore: Clear callback caches' persistent handles in destructor

Clearing the persistent handles in the destructor makes it a bit easier to
deal with the cache entry's lifetime: they are cleared whenever the entry
is removed from the cache.

We do this for both the main thread callback cache and the frame callback
cache.

Adding a destructor makes the cache entries non-movable. But the only
place where they were moved was when inserting them into the cache anyway.
We can use C++17's try_emplace() method to insert them without having to
move them.

* chore: Construct FrameCallbackCacheEntry with ID

This avoids the situation of forgetting to add an ID to the cache entry.

* chore: Improve usage of unordered_map APIs in CallbackHandlers

This fixes a few places where we can avoid double lookups:

- In RunOnMainThreadFdCallback, we already have a valid iterator, so no
  need to look up the same key again in RemoveKey (this is the only usage
  of RemoveKey, so we can remove it.) (Additionally, we probably want to
  remove the cache entry before throwing a NativeScript exception.)

- In PostFrameCallback and RemoveFrameCallback, we should not do
  contains() immediately followed by find().

* chore: Fix runtime typo

* chore: Ignore main thread and frame callback return values

We don't do anything with the return value from these callbacks, so it's
OK to ignore them and not convert them to a local handle.

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

@@ -89,13 +89,13 @@ void ArrayBufferHelper::CreateFromCallbackImpl(const FunctionCallbackInfo<Value>
         auto data = env.GetDirectBufferAddress(obj);
         auto size = env.GetDirectBufferCapacity(obj);
 
-        std::shared_ptr<v8::BackingStore> store = ArrayBuffer::NewBackingStore(
+        std::unique_ptr<v8::BackingStore> store = ArrayBuffer::NewBackingStore(
                 data,
                 size,
-                [](void* data, size_t length, void* deleter_data) {},
-                data);
+                BackingStore::EmptyDeleter,
+                nullptr);
 
-        arrayBuffer = ArrayBuffer::New(isolate, store);
+        arrayBuffer = ArrayBuffer::New(isolate, std::move(store));
     } else {
         if (m_remainingMethodID == nullptr) {
             m_remainingMethodID = env.GetMethodID(m_ByteBufferClass, "remaining", "()I");
@@ -117,15 +117,15 @@ void ArrayBufferHelper::CreateFromCallbackImpl(const FunctionCallbackInfo<Value>
         jbyte* data = new jbyte[bufferRemainingSize];
         memcpy(data, byteArrayElements, bufferRemainingSize);
 
-        std::shared_ptr<v8::BackingStore> store = ArrayBuffer::NewBackingStore(
+        std::unique_ptr<v8::BackingStore> store = ArrayBuffer::NewBackingStore(
                 data,
                 bufferRemainingSize,
                 [](void *data, size_t length, void *deleter_data) {
                     delete[] ((jbyte*)data);
                 },
                 nullptr);
 
-        arrayBuffer = ArrayBuffer::New(isolate, store);
+        arrayBuffer = ArrayBuffer::New(isolate, std::move(store));
         env.ReleaseByteArrayElements(byteArray, byteArrayElements, 0);
     }
 

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

@@ -671,8 +671,11 @@ void CallbackHandlers::RunOnMainThreadCallback(const FunctionCallbackInfo<v8::Va
 
     uint64_t key = ++count_;
     Local<v8::Function> callback = args[0].As<v8::Function>();
-    CacheEntry entry(isolate, callback, context);
-    cache_.emplace(key, std::move(entry));
+
+    bool inserted;
+    std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback, context);
+    assert(inserted && "Main thread callback ID should not be duplicated");
+
     auto value = Callback(key);
     auto size = sizeof(Callback);
     auto wrote = write(Runtime::GetWriter(),&value , size);
@@ -697,32 +700,20 @@ int CallbackHandlers::RunOnMainThreadFdCallback(int fd, int events, void *data)
     auto context = it->second.context_.Get(isolate);
     Context::Scope context_scope(context);
     Local<v8::Function> cb = it->second.callback_.Get(isolate);
-    Local<Value> result;
 
     v8::TryCatch tc(isolate);
 
-    if (!cb->Call(context, context->Global(), 0, nullptr).ToLocal(&result)) {}
+    cb->Call(context, context->Global(), 0, nullptr);  // ignore JS return value
+
+    cache_.erase(it);
 
     if(tc.HasCaught()){
         throw NativeScriptException(tc);
     }
 
-    RemoveKey(key);
-
     return 1;
 }
 
-void CallbackHandlers::RemoveKey(const uint64_t key) {
-    auto it = cache_.find(key);
-    if (it == cache_.end()) {
-        return;
-    }
-
-    it->second.callback_.Reset();
-    it->second.context_.Reset();
-    cache_.erase(it);
-}
-
 void CallbackHandlers::LogMethodCallback(const v8::FunctionCallbackInfo<v8::Value> &args) {
     try {
         if ((args.Length() > 0) && args[0]->IsString()) {
@@ -834,10 +825,10 @@ void CallbackHandlers::EnableVerboseLoggingMethodCallback(
         const v8::FunctionCallbackInfo<v8::Value> &args) {
     try {
         auto isolate = args.GetIsolate();
-        auto runtume = Runtime::GetRuntime(isolate);
+        auto runtime = Runtime::GetRuntime(isolate);
         tns::LogEnabled = true;
         JEnv env;
-        env.CallVoidMethod(runtume->GetJavaRuntime(), ENABLE_VERBOSE_LOGGING_METHOD_ID);
+        env.CallVoidMethod(runtime->GetJavaRuntime(), ENABLE_VERBOSE_LOGGING_METHOD_ID);
     } catch (NativeScriptException &e) {
         e.ReThrowToV8();
     } catch (std::exception e) {
@@ -855,10 +846,10 @@ void CallbackHandlers::DisableVerboseLoggingMethodCallback(
         const v8::FunctionCallbackInfo<v8::Value> &args) {
     try {
         auto isolate = args.GetIsolate();
-        auto runtume = Runtime::GetRuntime(isolate);
+        auto runtime = Runtime::GetRuntime(isolate);
         tns::LogEnabled = false;
         JEnv env;
-        env.CallVoidMethod(runtume->GetJavaRuntime(), DISABLE_VERBOSE_LOGGING_METHOD_ID);
+        env.CallVoidMethod(runtime->GetJavaRuntime(), DISABLE_VERBOSE_LOGGING_METHOD_ID);
     } catch (NativeScriptException &e) {
         e.ReThrowToV8();
     } catch (std::exception e) {
@@ -1589,16 +1580,12 @@ void CallbackHandlers::TerminateWorkerThread(Isolate *isolate) {
 void CallbackHandlers::RemoveIsolateEntries(v8::Isolate *isolate) {
     for (auto &item: cache_) {
         if (item.second.isolate_ == isolate) {
-            item.second.callback_.Reset();
-            item.second.context_.Reset();
             cache_.erase(item.first);
         }
     }
 
     for (auto &item: frameCallbackCache_) {
         if (item.second.isolate_ == isolate) {
-            item.second.callback_.Reset();
-            item.second.context_.Reset();
             frameCallbackCache_.erase(item.first);
         }
     }
@@ -1653,12 +1640,10 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &
 
         if (success && pId->IsNumber()){
             auto id = pId->IntegerValue(context).FromMaybe(0);
-            if(frameCallbackCache_.contains(id)){
-                auto cb = frameCallbackCache_.find(id);
-                if(cb != frameCallbackCache_.end()){
-                    cb->second.removed = false;
-                    PostCallback(args, &cb->second, context);
-                }
+            auto cb = frameCallbackCache_.find(id);
+            if (cb != frameCallbackCache_.end()) {
+                cb->second.removed = false;
+                PostCallback(args, &cb->second, context);
                 return;
             }
         }
@@ -1668,26 +1653,10 @@ void CallbackHandlers::PostFrameCallback(const FunctionCallbackInfo<v8::Value> &
 
         V8SetPrivateValue(isolate, func, idKey, v8::Number::New(isolate, (double) key));
 
-        FrameCallbackCacheEntry entry (isolate, callback, context);
-        entry.id = key;
-
-        auto finalCallback = [](const v8::WeakCallbackInfo<FrameCallbackCacheEntry> &data) {
-            auto value = data.GetParameter();
-            for (auto &item: frameCallbackCache_) {
-                if (item.second.id == value->id) {
-                    item.second.context_.Reset();
-                    item.second.callback_.Reset();
-                    frameCallbackCache_.erase(item.first);
-                    break;
-                }
-            }
-        };
-
-        entry.callback_.SetWeak(&entry, finalCallback, v8::WeakCallbackType::kFinalizer);
-
-        frameCallbackCache_.emplace(key, std::move(entry));
-
-        auto val = frameCallbackCache_.find(key);
+        robin_hood::unordered_map<uint64_t, FrameCallbackCacheEntry>::iterator val;
+        bool inserted;
+        std::tie(val, inserted) = frameCallbackCache_.try_emplace(key, isolate, callback, context, key);
+        assert(inserted && "Frame callback ID should not be duplicated");
 
         PostCallback(args, &val->second, context);
     }
@@ -1715,8 +1684,8 @@ void CallbackHandlers::RemoveFrameCallback(const FunctionCallbackInfo<v8::Value>
 
         if (success && pId->IsNumber()){
             auto id = pId->IntegerValue(context).FromMaybe(0);
-            if(frameCallbackCache_.contains(id)){
-                auto cb = frameCallbackCache_.find(id);
+            auto cb = frameCallbackCache_.find(id);
+            if (cb != frameCallbackCache_.end()) {
                 cb->second.removed = true;
             }
         }

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

@@ -284,8 +284,6 @@ namespace tns {
             jobject _runtime;
         };
 
-        static void RemoveKey(const uint64_t key);
-
         static std::atomic_int64_t count_;
 
 
@@ -305,6 +303,11 @@ namespace tns {
                       context_(isolate, context){
             }
 
+            ~CacheEntry() {
+                context_.Reset();
+                callback_.Reset();
+            }
+
             v8::Isolate* isolate_;
             v8::Global<v8::Function> callback_;
             v8::Global<v8::Context> context_;
@@ -316,17 +319,23 @@ namespace tns {
         static std::atomic_uint64_t frameCallbackCount_;
 
         struct FrameCallbackCacheEntry {
-            FrameCallbackCacheEntry(v8::Isolate *isolate, v8::Local<v8::Function> callback, v8::Local<v8::Context> context)
-                    : isolate_(isolate),
-                      callback_(isolate,callback),
-                      context_(isolate, context)
-                      {
+            FrameCallbackCacheEntry(v8::Isolate *isolate, v8::Local<v8::Function> callback,
+                                    v8::Local<v8::Context> context, uint64_t aId)
+                : isolate_(isolate),
+                  callback_(isolate, callback),
+                  context_(isolate, context),
+                  id(aId) {
+            }
+
+            ~FrameCallbackCacheEntry() {
+                context_.Reset();
+                callback_.Reset();
             }
 
             v8::Isolate *isolate_;
             v8::Global<v8::Function> callback_;
             v8::Global<v8::Context> context_;
-            bool removed;
+            bool removed = false;
             uint64_t id;
 
             AChoreographer_frameCallback frameCallback_ = [](long ts, void *data) {
@@ -341,6 +350,7 @@ namespace tns {
                 if (data != nullptr) {
                     auto entry = static_cast<FrameCallbackCacheEntry *>(data);
                     if (entry->removed) {
+                        frameCallbackCache_.erase(entry->id);  // invalidates *entry
                         return;
                     }
                     v8::Isolate *isolate = entry->isolate_;
@@ -352,15 +362,13 @@ namespace tns {
                     v8::Context::Scope context_scope(context);
 
                     v8::Local<v8::Function> cb = entry->callback_.Get(isolate);
-                    v8::Local<v8::Value> result;
-
                     v8::Local<v8::Value> args[1] = {v8::Number::New(isolate, ts)};
 
                     v8::TryCatch tc(isolate);
 
-                    if (!cb->Call(context, context->Global(), 1, args).ToLocal(&result)) {
-                        // TODO
-                    }
+                    cb->Call(context, context->Global(), 1, args);  // ignore JS return value
+
+                    frameCallbackCache_.erase(entry->id);  // invalidates *entry
 
                     if(tc.HasCaught()){
                         throw NativeScriptException(tc);

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

@@ -129,7 +129,6 @@ int MessageLoopTimer::PumpMessageLoopCallback(int fd, int events, void* data) {
     v8::HandleScope handleScope(isolate);
 
     while (v8::platform::PumpMessageLoop(Runtime::platform, isolate)) {
-//        isolate->RunMicrotasks();
         isolate->PerformMicrotaskCheckpoint();
     }
 

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

@@ -18,8 +18,6 @@ void Profiler::Init(Isolate* isolate, const Local<Object>& globalObj, const stri
     globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__startCPUProfiler"), FunctionTemplate::New(isolate, Profiler::StartCPUProfilerCallback, extData)->GetFunction(context).ToLocalChecked());
     globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__stopCPUProfiler"), FunctionTemplate::New(isolate, Profiler::StopCPUProfilerCallback, extData)->GetFunction(context).ToLocalChecked());
     globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__heapSnapshot"), FunctionTemplate::New(isolate, Profiler::HeapSnapshotMethodCallback, extData)->GetFunction(context).ToLocalChecked());
-    globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__startNDKProfiler"), FunctionTemplate::New(isolate, Profiler::StartNDKProfilerCallback, extData)->GetFunction(context).ToLocalChecked());
-    globalObj->Set(context, ArgConverter::ConvertToV8String(isolate, "__stopNDKProfiler"), FunctionTemplate::New(isolate, Profiler::StopNDKProfilerCallback, extData)->GetFunction(context).ToLocalChecked());
 }
 
 void Profiler::StartCPUProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args) {
@@ -195,56 +193,6 @@ bool Profiler::Write(CpuProfile* cpuProfile) {
     return true;
 }
 
-void Profiler::StartNDKProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args) {
-    try {
-        auto isolate = args.GetIsolate();
-        auto extData = args.Data().As<External>();
-        auto thiz = static_cast<Profiler*>(extData->Value());
-        thiz->StartNDKProfiler();
-    } catch (NativeScriptException& e) {
-        e.ReThrowToV8();
-    } catch (std::exception e) {
-        stringstream ss;
-        ss << "Error: c++ exception: " << e.what() << endl;
-        NativeScriptException nsEx(ss.str());
-        nsEx.ReThrowToV8();
-    } catch (...) {
-        NativeScriptException nsEx(std::string("Error: c++ exception!"));
-        nsEx.ReThrowToV8();
-    }
-}
-
-void Profiler::StopNDKProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args) {
-    try {
-        auto isolate = args.GetIsolate();
-        auto extData = args.Data().As<External>();
-        auto thiz = static_cast<Profiler*>(extData->Value());
-        thiz->StopNDKProfiler();
-    } catch (NativeScriptException& e) {
-        e.ReThrowToV8();
-    } catch (std::exception e) {
-        stringstream ss;
-        ss << "Error: c++ exception: " << e.what() << endl;
-        NativeScriptException nsEx(ss.str());
-        nsEx.ReThrowToV8();
-    } catch (...) {
-        NativeScriptException nsEx(std::string("Error: c++ exception!"));
-        nsEx.ReThrowToV8();
-    }
-}
-
-void Profiler::StartNDKProfiler() {
-#ifdef NDK_PROFILER_ENABLED
-    monstartup("libNativeScript.so");
-#endif
-}
-
-void Profiler::StopNDKProfiler() {
-#ifdef NDK_PROFILER_ENABLED
-    moncleanup();
-#endif
-}
-
 class FileOutputStream: public OutputStream {
     public:
         FileOutputStream(FILE* stream) :

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

@@ -17,10 +17,6 @@ class Profiler {
 
         static void StopCPUProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args);
 
-        static void StartNDKProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args);
-
-        static void StopNDKProfilerCallback(const v8::FunctionCallbackInfo<v8::Value>& args);
-
         static void HeapSnapshotMethodCallback(const v8::FunctionCallbackInfo<v8::Value>& args);
 
         void StartCPUProfilerCallbackImpl(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -33,10 +29,6 @@ class Profiler {
 
         bool StopCPUProfiler(v8::Isolate* isolate, const v8::Local<v8::String>& name);
 
-        void StartNDKProfiler();
-
-        void StopNDKProfiler();
-
         bool Write(v8::CpuProfile* cpuProfile);
 
         std::string m_appName;