Prefer a weak reference to the JavascriptContext in JavascriptFunction to a list of JavascriptFunction handles in JavascriptContext

- makes it safe to call Dispose (~JavascriptFunction) explicitly and to let the GC remove references of undisposed references (!JavascriptFunction)
- simplifies code in JavascriptContext which now does not need to know about JavascriptFunction handles that the GC takes care of anyway
- the Reset method on the Persistent<Function> handle is irrelevant after the v8::Context is disposed because the underlying memory has been freed by V8

Author: spahnke

Source/Noesis.Javascript/JavascriptContext.cpp

@@ -161,7 +161,6 @@ JavascriptContext::JavascriptContext()
 	mExternals = gcnew System::Collections::Generic::Dictionary<System::Object ^, WrappedJavascriptExternal>();
 	mMethods = gcnew System::Collections::Generic::Dictionary<System::String ^, WrappedMethod>();
     mTypeToConstructorMapping = gcnew System::Collections::Generic::Dictionary<System::Type ^, System::IntPtr>();
-	mFunctions = gcnew System::Collections::Generic::List<System::WeakReference ^>();
 	HandleScope scope(isolate);
 	mContext = new Persistent<Context>(isolate, Context::New(isolate));
     terminateRuns = false;
@@ -181,19 +180,14 @@ JavascriptContext::~JavascriptContext()
             wrapped.Pointer->Reset();
             delete wrapped.Pointer;
         }
-        for each (System::WeakReference^ f in mFunctions) {
-            JavascriptFunction ^function = safe_cast<JavascriptFunction ^>(f->Target);
-            if (function != nullptr)
-                delete function;
-        }
         for each (System::IntPtr p in mTypeToConstructorMapping->Values) {
             delete (void *)p;
         }
 		delete mContext;
+        mContext = nullptr;
 		delete mExternals;
         delete mMethods;
         delete mTypeToConstructorMapping;
-		delete mFunctions;
 	}
 	if (isolate != NULL)
 		isolate->Dispose();
@@ -532,16 +526,6 @@ JavascriptContext::GetObjectWrapperConstructorTemplate(System::Type ^type)
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
 
-void
-JavascriptContext::RegisterFunction(System::Object^ f)
-{
-    // Note that while we do store WeakReferences, we never clean up this hashtable,
-    // so it will just grow and grow.
-	mFunctions->Add(gcnew System::WeakReference(f));
-}
-
-////////////////////////////////////////////////////////////////////////////////////////////////////
-
 System::String^ JavascriptContext::V8Version::get()
 {
 	return gcnew System::String(v8::V8::GetVersion());

Source/Noesis.Javascript/JavascriptContext.h

@@ -196,10 +196,10 @@ public ref class JavascriptContext: public System::IDisposable
 
 	Local<FunctionTemplate> GetObjectWrapperConstructorTemplate(System::Type ^type);
 
-	void RegisterFunction(System::Object^ f);
-
 	static void FatalErrorCallbackMember(const char* location, const char* message);
 
+    inline bool IsDisposed() { return mContext == nullptr; }
+
 	////////////////////////////////////////////////////////////
 	// Data members
 	////////////////////////////////////////////////////////////
@@ -224,10 +224,6 @@ public ref class JavascriptContext: public System::IDisposable
     // The `IntPtr` points to a `Persistent<FunctionTemplate>`.
     System::Collections::Generic::Dictionary<System::Type ^, System::IntPtr> ^mTypeToConstructorMapping;
 
-	// Stores every JavascriptFunction we create.  Ensures we dispose of them
-	// all.
-	System::Collections::Generic::List<System::WeakReference ^> ^mFunctions;
-
 	// See comment for TerminateExecution().
 	bool terminateRuns;
 

Source/Noesis.Javascript/JavascriptFunction.cpp

@@ -20,18 +20,16 @@ JavascriptFunction::JavascriptFunction(v8::Local<v8::Object> iFunction, Javascri
 		throw gcnew System::ArgumentException("Must provide a JavascriptContext");
 
 	mFuncHandle = new Persistent<Function>(context->GetCurrentIsolate(), Local<Function>::Cast(iFunction));
-	mContext = context;
-
-	mContext->RegisterFunction(this);
+    mContextHandle = gcnew System::WeakReference(context);
 }
 
 JavascriptFunction::~JavascriptFunction()
 {
 	if(mFuncHandle) 
 	{
-		if (mContext)
+		if (IsAlive())
 		{
-            JavascriptScope scope(mContext);
+            JavascriptScope scope(GetContext());
 			mFuncHandle->Reset();
 		}
 		delete mFuncHandle;
@@ -46,16 +44,17 @@ JavascriptFunction::!JavascriptFunction()
 
 System::Object^ JavascriptFunction::Call(... cli::array<System::Object^>^ args)
 {
-    if (mFuncHandle == nullptr)
+    if (!IsAlive())
         throw gcnew JavascriptException(L"This function's owning JavascriptContext has been disposed");
     if (!args)
         throw gcnew System::ArgumentNullException("args");
 
-    JavascriptScope scope(mContext);
-	v8::Isolate* isolate = mContext->GetCurrentIsolate();
+    auto context = GetContext();
+    JavascriptScope scope(context);
+	v8::Isolate* isolate = context->GetCurrentIsolate();
 	HandleScope handleScope(isolate);
 
-	Local<v8::Object> global = mContext->GetGlobal();
+	Local<v8::Object> global = context->GetGlobal();
 
 	int argc = args->Length;
 	Local<v8::Value> *argv = new Local<v8::Value>[argc];
@@ -81,13 +80,13 @@ bool JavascriptFunction::operator==(JavascriptFunction^ func1, JavascriptFunctio
 		return false;
     if (func1_null && func2_null)
         return true;
-    if (func1->mFuncHandle == nullptr)
+    if (!func1->IsAlive())
         throw gcnew JavascriptException(L"'func1's owning JavascriptContext has been disposed");
-    if (func2->mFuncHandle == nullptr)
+    if (!func2->IsAlive())
         throw gcnew JavascriptException(L"'func2's owning JavascriptContext has been disposed");
 
-	Local<Function> jsFuncPtr1 = func1->mFuncHandle->Get(func1->mContext->GetCurrentIsolate());
-	Local<Function> jsFuncPtr2 = func2->mFuncHandle->Get(func2->mContext->GetCurrentIsolate());
+	Local<Function> jsFuncPtr1 = func1->mFuncHandle->Get(func1->GetContext()->GetCurrentIsolate());
+	Local<Function> jsFuncPtr2 = func2->mFuncHandle->Get(func2->GetContext()->GetCurrentIsolate());
 
 	return jsFuncPtr1->Equals(JavascriptContext::GetCurrentIsolate()->GetCurrentContext(), jsFuncPtr2).ToChecked();
 }
@@ -99,22 +98,23 @@ bool JavascriptFunction::Equals(JavascriptFunction^ other)
 
 bool JavascriptFunction::Equals(Object^ other)
 {
-    if (mFuncHandle == nullptr)
+    if (!IsAlive())
         throw gcnew JavascriptException(L"This function's owning JavascriptContext has been disposed");
     JavascriptFunction^ otherFunc = dynamic_cast<JavascriptFunction^>(other);
-    if (otherFunc != nullptr && otherFunc->mFuncHandle == nullptr)
-        throw gcnew JavascriptException(L"This function's owning JavascriptContext has been disposed");
+    if (otherFunc != nullptr && !otherFunc->IsAlive())
+        throw gcnew JavascriptException(L"The other function's owning JavascriptContext has been disposed");
 
 	return (otherFunc && this->Equals(otherFunc));
 }
 
 System::String^ JavascriptFunction::ToString()
 {
-    if (mFuncHandle == nullptr)
+    if (!IsAlive())
         throw gcnew JavascriptException(L"This function's owning JavascriptContext has been disposed");
 
-    JavascriptScope scope(mContext);
-    auto isolate = mContext->GetCurrentIsolate();
+    auto context = GetContext();
+    JavascriptScope scope(context);
+    auto isolate = context->GetCurrentIsolate();
     HandleScope handleScope(isolate);
     auto asString = mFuncHandle->Get(isolate)->ToString(isolate->GetCurrentContext());
     return safe_cast<System::String^>(JavascriptInterop::ConvertFromV8(asString.ToLocalChecked()));

Source/Noesis.Javascript/JavascriptFunction.h

@@ -35,10 +35,12 @@ public ref class JavascriptFunction
 	virtual bool Equals(Object^ other) override;
 
     virtual System::String^ ToString() override;
-
+internal:
+    v8::Persistent<v8::Function>* mFuncHandle;
 private:
-	v8::Persistent<v8::Function>* mFuncHandle;
-	JavascriptContext^ mContext;
+    System::WeakReference^ mContextHandle;
+    inline JavascriptContext^ GetContext() { return mContextHandle->IsAlive ? safe_cast<JavascriptContext^>(mContextHandle->Target) : nullptr; }
+    inline bool IsAlive() { auto context = GetContext(); return context != nullptr && !context->IsDisposed() && mFuncHandle != nullptr; }
 };
 
 //////////////////////////////////////////////////////////////////////////