Merge pull request #666 from amaitland/fix/jsb-memoryleak

amaitland · amaitland · commit 349edc7448fe · 2014-12-04T14:38:01.000+10:00
Fix JavaScript Binding Memoryleak
diff --git a/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp b/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.cpp
@@ -5,9 +5,9 @@
 
 #include "Stdafx.h"
 
-#include "CefAppWrapper.h"
 #include "CefBrowserWrapper.h"
 #include "CefAppUnmanagedWrapper.h"
+#include "JavascriptRootObjectWrapper.h"
 
 using namespace System;
 using namespace System::Diagnostics;
@@ -50,15 +50,22 @@ namespace CefSharp
         {
             auto window = context->GetGlobal();
 
-            auto jsRootWrapper = gcnew JavascriptRootObjectWrapper(wrapper->JavascriptRootObject, wrapper->BrowserProcess);
+            wrapper->JavascriptRootObjectWrapper = gcnew JavascriptRootObjectWrapper(wrapper->JavascriptRootObject, wrapper->BrowserProcess);
 
-            jsRootWrapper->V8Value = window;
-            jsRootWrapper->Bind();
+            wrapper->JavascriptRootObjectWrapper->V8Value = window;
+            wrapper->JavascriptRootObjectWrapper->Bind();
         }
     };
 
     void CefAppUnmanagedWrapper::OnContextReleased(CefRefPtr<CefBrowser> browser, CefRefPtr<CefFrame> frame, CefRefPtr<CefV8Context> context)
-    {        
+    { 
+        auto wrapper = FindBrowserWrapper(browser, true);
+
+        if (wrapper->JavascriptRootObjectWrapper != nullptr)
+        {
+            delete wrapper->JavascriptRootObjectWrapper;
+            wrapper->JavascriptRootObjectWrapper = nullptr;
+        }
     };
 
     CefBrowserWrapper^ CefAppUnmanagedWrapper::FindBrowserWrapper(CefRefPtr<CefBrowser> browser, bool mustExist)
diff --git a/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h b/CefSharp.BrowserSubprocess.Core/CefAppUnmanagedWrapper.h
@@ -10,7 +10,6 @@
 #include "include/cef_base.h"
 
 #include "CefBrowserWrapper.h"
-#include "CefAppWrapper.h"
 
 using namespace System::Collections::Generic;
 
diff --git a/CefSharp.BrowserSubprocess.Core/CefAppWrapper.h b/CefSharp.BrowserSubprocess.Core/CefAppWrapper.h
@@ -8,7 +8,6 @@
 #include "include/cef_app.h"
 #include "include/cef_base.h"
 
-#include "JavascriptRootObjectWrapper.h"
 #include "CefBrowserWrapper.h"
 #include "CefAppUnmanagedWrapper.h"
 
diff --git a/CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h b/CefSharp.BrowserSubprocess.Core/CefBrowserWrapper.h
@@ -10,6 +10,7 @@
 
 #include "TypeUtils.h"
 #include "Stdafx.h"
+#include "JavascriptRootObjectWrapper.h"
 
 using namespace CefSharp::Internals;
 using namespace System;
@@ -43,6 +44,7 @@ namespace CefSharp
         property bool IsPopup;
         property DuplexChannelFactory<IBrowserProcess^>^ ChannelFactory;
         property JavascriptRootObject^ JavascriptRootObject;
+        property JavascriptRootObjectWrapper^ JavascriptRootObjectWrapper;
         property IBrowserProcess^ BrowserProcess;
 
         JavascriptResponse^ EvaluateScriptInContext(CefRefPtr<CefV8Context> context, CefString script)
diff --git a/CefSharp.BrowserSubprocess.Core/CefTaskScheduler.cpp b/CefSharp.BrowserSubprocess.Core/CefTaskScheduler.cpp
@@ -2,6 +2,8 @@
 //
 // Use of this source code is governed by a BSD-style license that can be found in the LICENSE file.
 
+#pragma once
+
 #include "Stdafx.h"
 #include "CefTaskScheduler.h"
 
diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptMethodWrapper.cpp b/CefSharp.BrowserSubprocess.Core/JavascriptMethodWrapper.cpp
@@ -6,7 +6,6 @@
 #include "Stdafx.h"
 
 #include "JavascriptMethodWrapper.h"
-#include "CefAppWrapper.h"
 
 using namespace System;
 
diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptObjectWrapper.cpp b/CefSharp.BrowserSubprocess.Core/JavascriptObjectWrapper.cpp
@@ -6,7 +6,6 @@
 #include "Stdafx.h"
 
 #include "JavascriptObjectWrapper.h"
-#include "CefAppWrapper.h"
 
 using namespace System;
 using namespace CefSharp::Internals;
@@ -16,13 +15,13 @@ namespace CefSharp
 	void JavascriptObjectWrapper::Bind()
 	{
 		//Create property handler for get and set of Properties of this object
-		JsPropertyHandler = new JavascriptPropertyHandler(
+		_jsPropertyHandler = new JavascriptPropertyHandler(
 			gcnew Func<String^, BrowserProcessResponse^>(this, &JavascriptObjectWrapper::GetProperty),
 			gcnew Func<String^, Object^, BrowserProcessResponse^>(this, &JavascriptObjectWrapper::SetProperty)
 			);
 
 		//V8Value that represents this javascript object - only one per complex type
-		auto javascriptObject = V8Value->CreateObject(JsPropertyHandler.get());
+		auto javascriptObject = V8Value->CreateObject(_jsPropertyHandler.get());
 		auto objectName = StringUtils::ToNative(_object->JavascriptName);
 		V8Value->SetValue(objectName, javascriptObject, V8_PROPERTY_ATTRIBUTE_NONE);
 
diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptObjectWrapper.h b/CefSharp.BrowserSubprocess.Core/JavascriptObjectWrapper.h
@@ -9,6 +9,7 @@
 
 #include "JavascriptMethodWrapper.h"
 #include "JavascriptPropertyWrapper.h"
+#include "JavascriptPropertyHandler.h"
 
 using namespace System::Runtime::Serialization;
 using namespace System::Linq;
@@ -25,10 +26,11 @@ namespace CefSharp
         List<JavascriptMethodWrapper^>^ _wrappedMethods;
         List<JavascriptPropertyWrapper^>^ _wrappedProperties;
         IBrowserProcess^ _browserProcess;
+        MCefRefPtr<JavascriptPropertyHandler> _jsPropertyHandler;
 
     internal:
         MCefRefPtr<CefV8Value> V8Value;
-        MCefRefPtr<JavascriptPropertyHandler> JsPropertyHandler;
+        
 
     public:
         JavascriptObjectWrapper(JavascriptObject^ object, IBrowserProcess^ browserProcess)
@@ -43,7 +45,17 @@ namespace CefSharp
         ~JavascriptObjectWrapper()
         {
             V8Value = nullptr;
-            JsPropertyHandler = nullptr;
+            _jsPropertyHandler->Cleanup();
+            _jsPropertyHandler = nullptr;
+
+            for each (JavascriptMethodWrapper^ var in _wrappedMethods)
+            {
+                delete var;
+            }
+            for each (JavascriptPropertyWrapper^ var in _wrappedProperties)
+            {
+                delete var;
+            }
         }
 
         void Bind();
diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptPropertyHandler.h b/CefSharp.BrowserSubprocess.Core/JavascriptPropertyHandler.h
@@ -31,6 +31,12 @@ namespace CefSharp
             delete _setter;
         }
 
+        void Cleanup()
+        {
+            delete _getter;
+            delete _setter;
+        }
+
         virtual bool Get(const CefString& name, const CefRefPtr<CefV8Value> object, CefRefPtr<CefV8Value>& retval,
             CefString& exception) override
         {
diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptPropertyWrapper.cpp b/CefSharp.BrowserSubprocess.Core/JavascriptPropertyWrapper.cpp
@@ -19,9 +19,11 @@ namespace CefSharp
 
         if (_javascriptProperty->IsComplexType)
         {
-            auto wrapperObject = gcnew JavascriptObjectWrapper(_javascriptProperty->JsObject, _browserProcess);
-            wrapperObject->V8Value = V8Value.get();
-            wrapperObject->Bind();
+            auto javascriptObjectWrapper = gcnew JavascriptObjectWrapper(_javascriptProperty->JsObject, _browserProcess);
+            javascriptObjectWrapper->V8Value = V8Value.get();
+            javascriptObjectWrapper->Bind();
+
+            _javascriptObjectWrapper = javascriptObjectWrapper;
         }
         else
         {
diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptPropertyWrapper.h b/CefSharp.BrowserSubprocess.Core/JavascriptPropertyWrapper.h
@@ -7,8 +7,6 @@
 #include "Stdafx.h"
 #include "include/cef_v8.h"
 
-#include "JavascriptPropertyHandler.h"
-
 using namespace CefSharp::Internals;
 
 namespace CefSharp
@@ -19,7 +17,8 @@ namespace CefSharp
         JavascriptProperty^ _javascriptProperty;
         int64 _ownerId;
         IBrowserProcess^ _browserProcess;
-
+        //TODO: Strongly type this variable - currently trying to include JavascriptObjectWrapper.h creates a circular reference, so won't compile
+        Object^ _javascriptObjectWrapper;
     internal:
         MCefRefPtr<CefV8Value> V8Value;
 
@@ -34,6 +33,12 @@ namespace CefSharp
         ~JavascriptPropertyWrapper()
         {
             V8Value = nullptr;
+
+            if (_javascriptObjectWrapper != nullptr)
+            {
+                delete _javascriptObjectWrapper;
+                _javascriptObjectWrapper = nullptr;
+            }
         }
 
         void Bind();
diff --git a/CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h b/CefSharp.BrowserSubprocess.Core/JavascriptRootObjectWrapper.h
@@ -7,8 +7,6 @@
 #include "Stdafx.h"
 #include "include/cef_v8.h"
 
-#include "JavascriptMethodWrapper.h"
-#include "JavascriptPropertyWrapper.h"
 #include "JavascriptObjectWrapper.h"
 
 using namespace System::Runtime::Serialization;
@@ -40,6 +38,10 @@ namespace CefSharp
         ~JavascriptRootObjectWrapper()
         {
             V8Value = nullptr;
+            for each (JavascriptObjectWrapper^ var in _wrappedObjects)
+            {
+                delete var;
+            }
         }
 
         void Bind()

Original file line number	Diff line number	Diff line change
`@@ -31,6 +31,12 @@ namespace CefSharp`
`31`	`31`	`delete _setter;`
`32`	`32`	`}`
`33`	`33`
	`34`	`+ void Cleanup()`
	`35`	`+ {`
	`36`	`+ delete _getter;`
	`37`	`+ delete _setter;`
	`38`	`+ }`
	`39`	`+`
`34`	`40`	`virtual bool Get(const CefString& name, const CefRefPtr<CefV8Value> object, CefRefPtr<CefV8Value>& retval,`
`35`	`41`	`CefString& exception) override`
`36`	`42`	`{`
Original file line number	Diff line number	Diff line change
`@@ -19,9 +19,11 @@ namespace CefSharp`
`19`	`19`
`20`	`20`	`if (_javascriptProperty->IsComplexType)`
`21`	`21`	`{`
`22`		`- auto wrapperObject = gcnew JavascriptObjectWrapper(_javascriptProperty->JsObject, _browserProcess);`
`23`		`- wrapperObject->V8Value = V8Value.get();`
`24`		`- wrapperObject->Bind();`
	`22`	`+ auto javascriptObjectWrapper = gcnew JavascriptObjectWrapper(_javascriptProperty->JsObject, _browserProcess);`
	`23`	`+ javascriptObjectWrapper->V8Value = V8Value.get();`
	`24`	`+ javascriptObjectWrapper->Bind();`
	`25`	`+`
	`26`	`+ _javascriptObjectWrapper = javascriptObjectWrapper;`
`25`	`27`	`}`
`26`	`28`	`else`
`27`	`29`	`{`