diff --git a/Source/Noesis.Javascript/JavaScript.Net.vcxproj b/Source/Noesis.Javascript/JavaScript.Net.vcxproj index bd80fb9..c3f5c01 100644 --- a/Source/Noesis.Javascript/JavaScript.Net.vcxproj +++ b/Source/Noesis.Javascript/JavaScript.Net.vcxproj @@ -153,6 +153,7 @@ + diff --git a/Source/Noesis.Javascript/JavascriptContext.cpp b/Source/Noesis.Javascript/JavascriptContext.cpp index 667f052..6565a69 100644 --- a/Source/Noesis.Javascript/JavascriptContext.cpp +++ b/Source/Noesis.Javascript/JavascriptContext.cpp @@ -163,7 +163,7 @@ JavascriptContext::JavascriptContext() isolate->SetFatalErrorHandler(FatalErrorCallback); mExternals = gcnew System::Collections::Generic::Dictionary(); - mFunctions = gcnew System::Collections::Generic::List(); + mFunctions = gcnew System::Collections::Generic::List(); HandleScope scope(isolate); mContext = new Persistent(isolate, Context::New(isolate)); terminateRuns = false; @@ -178,8 +178,11 @@ JavascriptContext::~JavascriptContext() v8::Isolate::Scope isolate_scope(isolate); for each (WrappedJavascriptExternal wrapped in mExternals->Values) delete wrapped.Pointer; - for each (JavascriptFunction^ f in mFunctions) - delete f; + for each (System::WeakReference^ f in mFunctions) { + JavascriptFunction ^function = safe_cast(f->Target); + if (function != nullptr) + delete function; + } delete mContext; delete mExternals; delete mFunctions; @@ -202,7 +205,7 @@ void JavascriptContext::SetFatalErrorHandler(FatalErrorHandler^ handler) void JavascriptContext::SetFlags(System::String^ flags) { std::string convertedFlags = msclr::interop::marshal_as(flags); - v8::V8::SetFlagsFromString(convertedFlags.c_str(), convertedFlags.length()); + v8::V8::SetFlagsFromString(convertedFlags.c_str(), (int)convertedFlags.length()); } //////////////////////////////////////////////////////////////////////////////////////////////////// @@ -499,7 +502,9 @@ JavascriptContext::GetObjectWrapperTemplate() void JavascriptContext::RegisterFunction(System::Object^ f) { - mFunctions->Add(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)); } //////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/Source/Noesis.Javascript/JavascriptContext.h b/Source/Noesis.Javascript/JavascriptContext.h index 2247997..18b7823 100644 --- a/Source/Noesis.Javascript/JavascriptContext.h +++ b/Source/Noesis.Javascript/JavascriptContext.h @@ -220,7 +220,7 @@ public ref class JavascriptContext: public System::IDisposable // Stores every JavascriptFunction we create. Ensures we dispose of them // all. - System::Collections::Generic::List ^mFunctions; + System::Collections::Generic::List ^mFunctions; // See comment for TerminateExecution(). bool terminateRuns; diff --git a/Source/Noesis.Javascript/JavascriptFunction.cpp b/Source/Noesis.Javascript/JavascriptFunction.cpp index da83903..8e73361 100644 --- a/Source/Noesis.Javascript/JavascriptFunction.cpp +++ b/Source/Noesis.Javascript/JavascriptFunction.cpp @@ -31,7 +31,7 @@ JavascriptFunction::~JavascriptFunction() { if (mContext) { - JavascriptScope scope(mContext); + JavascriptScope scope(mContext); mFuncHandle->Reset(); } delete mFuncHandle; @@ -39,9 +39,19 @@ JavascriptFunction::~JavascriptFunction() } } +JavascriptFunction::!JavascriptFunction() +{ + delete this; +} + System::Object^ JavascriptFunction::Call(... cli::array^ args) { - JavascriptScope scope(mContext); + if (mFuncHandle == nullptr) + 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(); HandleScope handleScope(isolate); @@ -65,9 +75,17 @@ System::Object^ JavascriptFunction::Call(... cli::array^ args) bool JavascriptFunction::operator==(JavascriptFunction^ func1, JavascriptFunction^ func2) { - if(ReferenceEquals(func2, nullptr)) { + bool func1_null = func1 == nullptr, + func2_null = func2 == nullptr; + if (func1_null != func2_null) return false; - } + if (func1_null && func2_null) + return true; + if (func1->mFuncHandle == nullptr) + throw gcnew JavascriptException(L"'func1's owning JavascriptContext has been disposed"); + if (func2->mFuncHandle == nullptr) + throw gcnew JavascriptException(L"'func2's owning JavascriptContext has been disposed"); + Handle jsFuncPtr1 = func1->mFuncHandle->Get(func1->mContext->GetCurrentIsolate()); Handle jsFuncPtr2 = func2->mFuncHandle->Get(func2->mContext->GetCurrentIsolate()); @@ -81,7 +99,12 @@ bool JavascriptFunction::Equals(JavascriptFunction^ other) bool JavascriptFunction::Equals(Object^ other) { - JavascriptFunction^ otherFunc = dynamic_cast(other); + if (mFuncHandle == nullptr) + throw gcnew JavascriptException(L"This function's owning JavascriptContext has been disposed"); + JavascriptFunction^ otherFunc = dynamic_cast(other); + if (otherFunc != nullptr && otherFunc->mFuncHandle == nullptr) + throw gcnew JavascriptException(L"This function's owning JavascriptContext has been disposed"); + return (otherFunc && this->Equals(otherFunc)); } diff --git a/Source/Noesis.Javascript/JavascriptFunction.h b/Source/Noesis.Javascript/JavascriptFunction.h index 1c2150e..b71f128 100644 --- a/Source/Noesis.Javascript/JavascriptFunction.h +++ b/Source/Noesis.Javascript/JavascriptFunction.h @@ -17,13 +17,16 @@ namespace Noesis { namespace Javascript { ////////////////////////////////////////////////////////////////////////// // JavascriptFunction // -// Wraps around JS function object and allow calling it in later time +// Wraps around a JS function when passed back to C#, allowing it to be +// called from C#. Callers must not dispose of their JavascriptContext +// while they still have references to JavascriptFunctions. ////////////////////////////////////////////////////////////////////////// public ref class JavascriptFunction { public: JavascriptFunction(v8::Handle iFunction, JavascriptContext^ context); ~JavascriptFunction(); + !JavascriptFunction(); System::Object^ Call(... cli::array^ args); diff --git a/Tests/Noesis.Javascript.Tests/JavascriptFunctionTests.cs b/Tests/Noesis.Javascript.Tests/JavascriptFunctionTests.cs index 0b39caf..d0b1132 100644 --- a/Tests/Noesis.Javascript.Tests/JavascriptFunctionTests.cs +++ b/Tests/Noesis.Javascript.Tests/JavascriptFunctionTests.cs @@ -27,7 +27,7 @@ public void TearDown() public void GetFunctionExpressionFromJsContext() { _context.Run("a = function(a, b) { return a + b; }"); - + JavascriptFunction funcObj = _context.GetParameter("a") as JavascriptFunction; funcObj.Should().NotBeNull(); funcObj.Call(1, 2).Should().BeOfType().Which.Should().Be(3); @@ -37,7 +37,7 @@ public void GetFunctionExpressionFromJsContext() public void GetNamedFunctionFromJsContext() { _context.Run("function test(a, b) { return a + b; }"); - + JavascriptFunction funcObj = _context.GetParameter("test") as JavascriptFunction; funcObj.Should().NotBeNull(); funcObj.Call(1, 2).Should().BeOfType().Which.Should().Be(3); @@ -47,7 +47,7 @@ public void GetNamedFunctionFromJsContext() public void GetArrowFunctionExpressionFromJsContext() { _context.Run("a = (a, b) => a + b"); - + JavascriptFunction funcObj = _context.GetParameter("a") as JavascriptFunction; funcObj.Should().NotBeNull(); funcObj.Call(1, 2).Should().BeOfType().Which.Should().Be(3); @@ -57,7 +57,7 @@ public void GetArrowFunctionExpressionFromJsContext() public void PassFunctionToMethodInManagedObjectAndUseItToFilterAList() { _context.SetParameter("collection", new CollectionWrapper()); - + var result = _context.Run("collection.Filter(x => x % 2 === 0)") as IEnumerable; result.Should().NotBeNull(); result.Should().BeEquivalentTo(2, 4); @@ -73,6 +73,30 @@ public void ExceptionsAreHandledAndWrappedInAJavascriptExceptionObject() } } + [TestClass] + public class JavascriptFunctionTestsWithoutAutomaticContext + { + [TestMethod] + public void CannotUseAFunctionWhenItsContextIsDisposed() + { + JavascriptFunction function; + using (var context = new JavascriptContext()) { + function = context.Run("() => { throw new Error('test'); }") as JavascriptFunction; + } + Action action = () => function.Call(); + action.ShouldThrowExactly().WithMessage("This function's owning JavascriptContext has been disposed"); + } + + [TestMethod] + public void DisposingAFunction() + { + using (var context = new JavascriptContext()) { + var function = context.Run("() => { throw new Error('test'); }") as JavascriptFunction; + function.Dispose(); + } + } + } + class CollectionWrapper { private IEnumerable numbers = new List { 1, 2, 3, 4, 5 };