Skip to content

fix: Ensure Isolate is alive before using it (#63) #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 17, 2020
Merged

fix: Ensure Isolate is alive before using it (#63) #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions NativeScript/runtime/ArgConverter.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "ObjectManager.h"
#include "Interop.h"
#include "Helpers.h"
#include "Runtime.h"

using namespace v8;
using namespace std;
Expand Down Expand Up @@ -91,6 +92,12 @@
MethodCallbackWrapper* data = static_cast<MethodCallbackWrapper*>(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);
Expand Down
73 changes: 39 additions & 34 deletions NativeScript/runtime/ClassBuilder.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "Helpers.h"
#include "Caches.h"
#include "Interop.h"
#include "Runtime.h"

using namespace v8;

Expand Down Expand Up @@ -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> value = it->second->Get(isolate);
BaseDataWrapper* wrapper = tns::GetValue(isolate, value);
if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) {
ObjCDataWrapper* objcWrapper = static_cast<ObjCDataWrapper*>(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> value = it->second->Get(isolate);
BaseDataWrapper* wrapper = tns::GetValue(isolate, value);
if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) {
ObjCDataWrapper* objcWrapper = static_cast<ObjCDataWrapper*>(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> value = it->second->Get(isolate);
BaseDataWrapper* wrapper = tns::GetValue(isolate, value);
if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) {
ObjCDataWrapper* objcWrapper = static_cast<ObjCDataWrapper*>(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> value = it->second->Get(isolate);
BaseDataWrapper* wrapper = tns::GetValue(isolate, value);
if (wrapper != nullptr && wrapper->Type() == WrapperType::ObjCObject) {
ObjCDataWrapper* objcWrapper = static_cast<ObjCDataWrapper*>(wrapper);
objcWrapper->GcUnprotect();
}
}
}
}

release(self, @selector(release));
});
class_addMethod(extendedClass, @selector(release), newRelease, "v@:");

Expand Down
3 changes: 3 additions & 0 deletions NativeScript/runtime/Runtime.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<v8::Platform> platform_;
static std::vector<v8::Isolate*> isolates_;
static bool mainThreadInitialized_;

void DefineGlobalObject(v8::Local<v8::Context> context);
Expand Down
9 changes: 9 additions & 0 deletions NativeScript/runtime/Runtime.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -71,6 +73,8 @@
create_params.array_buffer_allocator = &allocator_;
Isolate* isolate = Isolate::New(create_params);

Runtime::isolates_.emplace_back(isolate);

return isolate;
}

Expand Down Expand Up @@ -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<Platform> Runtime::platform_;
std::vector<Isolate*> Runtime::isolates_;
bool Runtime::mainThreadInitialized_ = false;
thread_local Runtime* Runtime::currentRuntime_ = nullptr;

Expand Down
38 changes: 38 additions & 0 deletions TestRunner/app/tests/shared/Workers/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down