From eebf95f65fed3035e70da6845037c12a8ae89b0c Mon Sep 17 00:00:00 2001 From: Darin Dimitrov Date: Thu, 15 Oct 2020 13:01:43 +0300 Subject: [PATCH] fix: Ensure Isolate is alive before using it (#63) --- NativeScript/runtime/ArgConverter.mm | 7 ++ NativeScript/runtime/ClassBuilder.mm | 73 +++++++++++--------- NativeScript/runtime/Runtime.h | 3 + NativeScript/runtime/Runtime.mm | 9 +++ TestRunner/app/tests/shared/Workers/index.js | 38 ++++++++++ 5 files changed, 96 insertions(+), 34 deletions(-) diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index 104945a6..cf3f971f 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -6,6 +6,7 @@ #include "ObjectManager.h" #include "Interop.h" #include "Helpers.h" +#include "Runtime.h" using namespace v8; using namespace std; @@ -91,6 +92,12 @@ MethodCallbackWrapper* data = static_cast(userData); Isolate* isolate = data->isolate_; + + if (!Runtime::IsAlive(isolate)) { + memset(retValue, 0, cif->rtype->size); + return; + } + v8::Locker locker(isolate); Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); diff --git a/NativeScript/runtime/ClassBuilder.mm b/NativeScript/runtime/ClassBuilder.mm index 856490a4..f24ef8ab 100644 --- a/NativeScript/runtime/ClassBuilder.mm +++ b/NativeScript/runtime/ClassBuilder.mm @@ -10,6 +10,7 @@ #include "Helpers.h" #include "Caches.h" #include "Interop.h" +#include "Runtime.h" using namespace v8; @@ -254,45 +255,49 @@ /// in order to make both of them destroyable/GC-able. When the JavaScript object is GC-ed we release the native counterpart as well. void (*retain)(id, SEL) = (void (*)(id, SEL))FindNotOverridenMethod(extendedClass, @selector(retain)); IMP newRetain = imp_implementationWithBlock(^(id self) { - if ([self retainCount] == 1) { - auto it = cache->Instances.find(self); - if (it != cache->Instances.end()) { - v8::Locker locker(isolate); - Isolate::Scope isolate_scope(isolate); - HandleScope handle_scope(isolate); - Local value = it->second->Get(isolate); - BaseDataWrapper* wrapper = tns::GetValue(isolate, value); - if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { - ObjCDataWrapper* objcWrapper = static_cast(wrapper); - objcWrapper->GcProtect(); - } - } - } - - return retain(self, @selector(retain)); + if ([self retainCount] == 1) { + auto it = cache->Instances.find(self); + if (it != cache->Instances.end()) { + v8::Locker locker(isolate); + Isolate::Scope isolate_scope(isolate); + HandleScope handle_scope(isolate); + Local value = it->second->Get(isolate); + BaseDataWrapper* wrapper = tns::GetValue(isolate, value); + if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { + ObjCDataWrapper* objcWrapper = static_cast(wrapper); + objcWrapper->GcProtect(); + } + } + } + + return retain(self, @selector(retain)); }); class_addMethod(extendedClass, @selector(retain), newRetain, "@@:"); void (*release)(id, SEL) = (void (*)(id, SEL))FindNotOverridenMethod(extendedClass, @selector(release)); IMP newRelease = imp_implementationWithBlock(^(id self) { - if ([self retainCount] == 2) { - auto it = cache->Instances.find(self); - if (it != cache->Instances.end()) { - v8::Locker locker(isolate); - Isolate::Scope isolate_scope(isolate); - HandleScope handle_scope(isolate); - if (it->second != nullptr) { - Local value = it->second->Get(isolate); - BaseDataWrapper* wrapper = tns::GetValue(isolate, value); - if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { - ObjCDataWrapper* objcWrapper = static_cast(wrapper); - objcWrapper->GcUnprotect(); - } - } - } - } - - release(self, @selector(release)); + if (!Runtime::IsAlive(isolate)) { + return; + } + + if ([self retainCount] == 2) { + auto it = cache->Instances.find(self); + if (it != cache->Instances.end()) { + v8::Locker locker(isolate); + Isolate::Scope isolate_scope(isolate); + HandleScope handle_scope(isolate); + if (it->second != nullptr) { + Local value = it->second->Get(isolate); + BaseDataWrapper* wrapper = tns::GetValue(isolate, value); + if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) { + ObjCDataWrapper* objcWrapper = static_cast(wrapper); + objcWrapper->GcUnprotect(); + } + } + } + } + + release(self, @selector(release)); }); class_addMethod(extendedClass, @selector(release), newRelease, "v@:"); diff --git a/NativeScript/runtime/Runtime.h b/NativeScript/runtime/Runtime.h index d5674ff4..43f7aa68 100644 --- a/NativeScript/runtime/Runtime.h +++ b/NativeScript/runtime/Runtime.h @@ -42,9 +42,12 @@ class Runtime { } static id GetAppConfigValue(std::string key); + + static bool IsAlive(v8::Isolate* isolate); private: static thread_local Runtime* currentRuntime_; static std::shared_ptr platform_; + static std::vector isolates_; static bool mainThreadInitialized_; void DefineGlobalObject(v8::Local context); diff --git a/NativeScript/runtime/Runtime.mm b/NativeScript/runtime/Runtime.mm index 0e70777b..3a5c354f 100644 --- a/NativeScript/runtime/Runtime.mm +++ b/NativeScript/runtime/Runtime.mm @@ -48,6 +48,8 @@ this->isolate_->Exit(); } + Runtime::isolates_.erase(std::remove(Runtime::isolates_.begin(), Runtime::isolates_.end(), this->isolate_), Runtime::isolates_.end()); + this->isolate_->Dispose(); currentRuntime_ = nullptr; @@ -71,6 +73,8 @@ create_params.array_buffer_allocator = &allocator_; Isolate* isolate = Isolate::New(create_params); + Runtime::isolates_.emplace_back(isolate); + return isolate; } @@ -226,7 +230,12 @@ globalTemplate->Set(ToV8String(isolate, "__time"), timeFunctionTemplate); } +bool Runtime::IsAlive(Isolate* isolate) { + return std::find(Runtime::isolates_.begin(), Runtime::isolates_.end(), isolate) != Runtime::isolates_.end(); +} + std::shared_ptr Runtime::platform_; +std::vector Runtime::isolates_; bool Runtime::mainThreadInitialized_ = false; thread_local Runtime* Runtime::currentRuntime_ = nullptr; diff --git a/TestRunner/app/tests/shared/Workers/index.js b/TestRunner/app/tests/shared/Workers/index.js index 2d721133..b4e122e0 100644 --- a/TestRunner/app/tests/shared/Workers/index.js +++ b/TestRunner/app/tests/shared/Workers/index.js @@ -497,6 +497,44 @@ describe("TNS Workers", () => { task.resume(); ` }); }); + + it("Should not crash if the worker registers a notification", done => { + var worker = new Worker("./tests/shared/Workers/EvalWorker.js"); + worker.onmessage = (msg) => { + worker.terminate(); + NSNotificationCenter.defaultCenter.postNotificationNameObject("MyNotification", null); + done(); + }; + + var workerScript = ` + var NotificationObserver = /** @class */ (function (_super) { + __extends(NotificationObserver, _super); + function NotificationObserver() { + return _super !== null && _super.apply(this, arguments) || this; + } + NotificationObserver.initWithCallback = function (onReceiveCallback) { + var observer = _super.new.call(this); + observer._onReceiveCallback = onReceiveCallback; + return observer; + }; + NotificationObserver.prototype.onReceive = function (notification) { + this._onReceiveCallback(notification); + }; + NotificationObserver.ObjCExposedMethods = { + onReceive: { returns: interop.types.void, params: [NSNotification] }, + }; + return NotificationObserver; + }(NSObject)); + + const observer = NotificationObserver.initWithCallback(notification => { }); + + NSNotificationCenter.defaultCenter.addObserverSelectorNameObject(observer, "onReceive", "MyNotification", null); + + postMessage(self === global); + `; + + worker.postMessage({ eval: workerScript }); + }); } function generateRandomString(strLen) {