Review Callback classes and implement the simplest possible fix until a long term solution for #2349 is devised

amaitland · amaitland · commit 11cfb7d43477 · 2018-05-13T13:07:13.000+10:00
- Add OnComplete check to all the TaskCompletionSource based callbacks that could potentially have executed TrySetResultAsync twice, which could on rare occasions execute in a non deterministic fashion (The Disposed execution of TrySetResultAsync called before the OnComplete one). See #2349 for a more detailed proposal and one potential long term solution (upgrading to .Net 4.6 is the best option, though not practical now, will update the example projects shortly). - Update callbacks for consistency, all now implement IDisposable and have an IsDisposed
diff --git a/CefSharp.Core/Internals/CefResolveCallbackAdapter.h b/CefSharp.Core/Internals/CefResolveCallbackAdapter.h
@@ -22,7 +22,8 @@ namespace CefSharp
 
         ~CefResolveCallbackAdapter()
         {
-            _handler = nullptr;
+			delete _handler;
+			_handler = nullptr;
         }
 
         void OnResolveCompleted(cef_errorcode_t result,	const std::vector<CefString>& resolvedIps) OVERRIDE
diff --git a/CefSharp/Callback/TaskCompletionCallback.cs b/CefSharp/Callback/TaskCompletionCallback.cs
@@ -10,13 +10,18 @@ namespace CefSharp
 {
     public class TaskCompletionCallback : ICompletionCallback
     {
-        private readonly TaskCompletionSource<bool> taskCompletionSource = new TaskCompletionSource<bool>();
+        private readonly TaskCompletionSource<bool> taskCompletionSource;
         private volatile bool isDisposed;
-        private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety
+        private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety
+
+        public TaskCompletionCallback()
+        {
+            taskCompletionSource = new TaskCompletionSource<bool>();
+        }
 
         void ICompletionCallback.OnComplete()
         {
-            complete = true;
+            onComplete = true;
 
             taskCompletionSource.TrySetResultAsync(true);
         }
@@ -35,9 +40,10 @@ void IDisposable.Dispose()
         {
             var task = taskCompletionSource.Task;
 
-            //If the Task hasn't completed and this is being disposed then
-            //set the TCS to false
-            if(complete == false && task.IsCompleted == false)
+            //If onComplete is false then ICompletionCallback.OnComplete was never called,
+            //so we'll set the result to false. Calling TrySetResultAsync multiple times 
+            //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349
+            if (onComplete == false && task.IsCompleted == false)
             {
                 taskCompletionSource.TrySetResultAsync(false);
             }
diff --git a/CefSharp/Callback/TaskDeleteCookiesCallback.cs b/CefSharp/Callback/TaskDeleteCookiesCallback.cs
@@ -15,13 +15,18 @@ public class TaskDeleteCookiesCallback : IDeleteCookiesCallback
     {
         public const int InvalidNoOfCookiesDeleted = -1;
 
-        private readonly TaskCompletionSource<int> taskCompletionSource = new TaskCompletionSource<int>();
+        private readonly TaskCompletionSource<int> taskCompletionSource;
         private volatile bool isDisposed;
-        private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety
+        private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety
+
+        public TaskDeleteCookiesCallback()
+        {
+            taskCompletionSource = new TaskCompletionSource<int>();
+        }
 
         void IDeleteCookiesCallback.OnComplete(int numDeleted)
         {
-            complete = true;
+            onComplete = true;
 
             taskCompletionSource.TrySetResultAsync(numDeleted);
         }
@@ -35,14 +40,14 @@ bool IDeleteCookiesCallback.IsDisposed
         {
             get { return isDisposed; }
         }
-
         void IDisposable.Dispose()
         {
             var task = taskCompletionSource.Task;
 
-            //If the Task hasn't completed and this is being disposed then
-            //set the TCS to InvalidNoOfCookiesDeleted
-            if (complete == false && task.IsCompleted == false)
+            //If onComplete is false then IDeleteCookiesCallback.OnComplete was never called,
+            //so we'll set the result to false. Calling TrySetResultAsync multiple times 
+            //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349
+            if (onComplete == false && task.IsCompleted == false)
             {
                 taskCompletionSource.TrySetResultAsync(InvalidNoOfCookiesDeleted);
             }
diff --git a/CefSharp/Callback/TaskPrintToPdfCallback.cs b/CefSharp/Callback/TaskPrintToPdfCallback.cs
@@ -12,7 +12,7 @@ public sealed class TaskPrintToPdfCallback : IPrintToPdfCallback
     {
         private readonly TaskCompletionSource<bool> taskCompletionSource = new TaskCompletionSource<bool>();
         private volatile bool isDisposed;
-        private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety
+        private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety
 
         public Task<bool> Task
         {
@@ -21,7 +21,7 @@ public Task<bool> Task
 
         void IPrintToPdfCallback.OnPdfPrintFinished(string path, bool ok)
         {
-            complete = true;
+            onComplete = true;
 
             taskCompletionSource.TrySetResultAsync(ok);
         }
@@ -35,9 +35,10 @@ void IDisposable.Dispose()
         {
             var task = taskCompletionSource.Task;
 
-            //If the Task hasn't completed and this is being disposed then
-            //set the TCS to false
-            if (complete == false && task.IsCompleted == false)
+            //If onComplete is false then IPrintToPdfCallback.OnPdfPrintFinished was never called,
+            //so we'll set the result to false. Calling TrySetResultAsync multiple times 
+            //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349
+            if (onComplete == false && task.IsCompleted == false)
             {
                 taskCompletionSource.TrySetResultAsync(false);
             }
diff --git a/CefSharp/Callback/TaskRegisterCdmCallback.cs b/CefSharp/Callback/TaskRegisterCdmCallback.cs
@@ -13,13 +13,18 @@ namespace CefSharp
     /// </summary>
     public class TaskRegisterCdmCallback : IRegisterCdmCallback
     {
-        private readonly TaskCompletionSource<CdmRegistration> taskCompletionSource = new TaskCompletionSource<CdmRegistration>();
+        private readonly TaskCompletionSource<CdmRegistration> taskCompletionSource;
         private volatile bool isDisposed;
-        private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety
+        private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety
+
+        public TaskRegisterCdmCallback()
+        {
+            taskCompletionSource = new TaskCompletionSource<CdmRegistration>();
+        }
 
         void IRegisterCdmCallback.OnRegistrationComplete(CdmRegistration registration)
         {
-            complete = true;
+            onComplete = true;
 
             taskCompletionSource.TrySetResultAsync(registration);
         }
@@ -38,9 +43,10 @@ void IDisposable.Dispose()
         {
             var task = taskCompletionSource.Task;
 
-            //If the Task hasn't completed and this is being disposed then
-            //set the TCS to null
-            if (complete == false && task.IsCompleted == false)
+            //If onComplete is false then IRegisterCdmCallback.OnRegistrationComplete was never called,
+            //so we'll set the result to false. Calling TrySetResultAsync multiple times 
+            //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349
+            if (onComplete == false && task.IsCompleted == false)
             {
                 taskCompletionSource.TrySetResultAsync(null);
             }
diff --git a/CefSharp/Callback/TaskResolveCallback.cs b/CefSharp/Callback/TaskResolveCallback.cs
@@ -11,39 +11,45 @@ namespace CefSharp
 {
     public class TaskResolveCallback : IResolveCallback
     {
-        private readonly TaskCompletionSource<ResolveCallbackResult> taskCompletionSource = new TaskCompletionSource<ResolveCallbackResult>();
+        private readonly TaskCompletionSource<ResolveCallbackResult> taskCompletionSource;
         private volatile bool isDisposed;
-        private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety
+        private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety
+
+        public TaskResolveCallback()
+        {
+            taskCompletionSource = new TaskCompletionSource<ResolveCallbackResult>();
+        }
 
         void IResolveCallback.OnResolveCompleted(CefErrorCode result, IList<string> resolvedIpAddresses)
         {
-            complete = true;
+            onComplete = true;
 
             taskCompletionSource.TrySetResultAsync(new ResolveCallbackResult(result, resolvedIpAddresses));
         }
 
-        void IDisposable.Dispose()
+        bool IResolveCallback.IsDisposed
         {
-            var task = taskCompletionSource.Task;
-
-            //If the Task hasn't completed and this is being disposed then
-            //set the TCS to null
-            if (complete == false && task.IsCompleted == false)
-            {
-                taskCompletionSource.TrySetResultAsync(new ResolveCallbackResult(CefErrorCode.Unexpected, null));
-            }
-
-            isDisposed = true;
+            get { return isDisposed; }
         }
 
         public Task<ResolveCallbackResult> Task
         {
             get { return taskCompletionSource.Task; }
         }
 
-        bool IResolveCallback.IsDisposed
+        void IDisposable.Dispose()
         {
-            get { return isDisposed; }
+            var task = taskCompletionSource.Task;
+
+            //If onComplete is false then IResolveCallback.OnResolveCompleted was never called,
+            //so we'll set the result to false. Calling TrySetResultAsync multiple times 
+            //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349
+            if (onComplete == false && task.IsCompleted == false)
+            {
+                taskCompletionSource.TrySetResultAsync(new ResolveCallbackResult(CefErrorCode.Unexpected, null));
+            }
+
+            isDisposed = true;
         }
     }
 }
diff --git a/CefSharp/Callback/TaskSetCookieCallback.cs b/CefSharp/Callback/TaskSetCookieCallback.cs
@@ -13,13 +13,18 @@ namespace CefSharp
     /// </summary>
     public class TaskSetCookieCallback : ISetCookieCallback
     {
-        private readonly TaskCompletionSource<bool> taskCompletionSource = new TaskCompletionSource<bool>();
+        private readonly TaskCompletionSource<bool> taskCompletionSource;
         private volatile bool isDisposed;
-        private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety
+        private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety
+
+        public TaskSetCookieCallback()
+        {
+            taskCompletionSource = new TaskCompletionSource<bool>();
+        }
 
         void ISetCookieCallback.OnComplete(bool success)
         {
-            complete = true;
+            onComplete = true;
 
             taskCompletionSource.TrySetResultAsync(success);
         }
@@ -38,9 +43,10 @@ void IDisposable.Dispose()
         {
             var task = taskCompletionSource.Task;
 
-            //If the Task hasn't completed and this is being disposed then
-            //set the TCS to false
-            if (complete == false && task.IsCompleted == false)
+            //If onComplete is false then ISetCookieCallback.OnComplete was never called,
+            //so we'll set the result to false. Calling TrySetResultAsync multiple times 
+            //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349
+            if (onComplete == false && task.IsCompleted == false)
             {
                 taskCompletionSource.TrySetResultAsync(false);
             }

Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,8 @@ namespace CefSharp`
`22`	`22`
`23`	`23`	`~CefResolveCallbackAdapter()`
`24`	`24`	`{`
`25`		`- _handler = nullptr;`
	`25`	`+ delete _handler;`
	`26`	`+ _handler = nullptr;`
`26`	`27`	`}`
`27`	`28`
`28`	`29`	`void OnResolveCompleted(cef_errorcode_t result, const std::vector<CefString>& resolvedIps) OVERRIDE`
Original file line number	Diff line number	Diff line change
`@@ -10,13 +10,18 @@ namespace CefSharp`
`10`	`10`	`{`
`11`	`11`	`public class TaskCompletionCallback : ICompletionCallback`
`12`	`12`	`{`
`13`		`- private readonly TaskCompletionSource<bool> taskCompletionSource = new TaskCompletionSource<bool>();`
	`13`	`+ private readonly TaskCompletionSource<bool> taskCompletionSource;`
`14`	`14`	`private volatile bool isDisposed;`
`15`		`- private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety`
	`15`	`+ private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety`
	`16`	`+`
	`17`	`+ public TaskCompletionCallback()`
	`18`	`+ {`
	`19`	`+ taskCompletionSource = new TaskCompletionSource<bool>();`
	`20`	`+ }`
`16`	`21`
`17`	`22`	`void ICompletionCallback.OnComplete()`
`18`	`23`	`{`
`19`		`- complete = true;`
	`24`	`+ onComplete = true;`
`20`	`25`
`21`	`26`	`taskCompletionSource.TrySetResultAsync(true);`
`22`	`27`	`}`
`@@ -35,9 +40,10 @@ void IDisposable.Dispose()`
`35`	`40`	`{`
`36`	`41`	`var task = taskCompletionSource.Task;`
`37`	`42`
`38`		`- //If the Task hasn't completed and this is being disposed then`
`39`		`- //set the TCS to false`
`40`		`- if(complete == false && task.IsCompleted == false)`
	`43`	`+ //If onComplete is false then ICompletionCallback.OnComplete was never called,`
	`44`	`+ //so we'll set the result to false. Calling TrySetResultAsync multiple times`
	`45`	`+ //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349`
	`46`	`+ if (onComplete == false && task.IsCompleted == false)`
`41`	`47`	`{`
`42`	`48`	`taskCompletionSource.TrySetResultAsync(false);`
`43`	`49`	`}`
Original file line number	Diff line number	Diff line change
`@@ -15,13 +15,18 @@ public class TaskDeleteCookiesCallback : IDeleteCookiesCallback`
`15`	`15`	`{`
`16`	`16`	`public const int InvalidNoOfCookiesDeleted = -1;`
`17`	`17`
`18`		`- private readonly TaskCompletionSource<int> taskCompletionSource = new TaskCompletionSource<int>();`
	`18`	`+ private readonly TaskCompletionSource<int> taskCompletionSource;`
`19`	`19`	`private volatile bool isDisposed;`
`20`		`- private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety`
	`20`	`+ private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety`
	`21`	`+`
	`22`	`+ public TaskDeleteCookiesCallback()`
	`23`	`+ {`
	`24`	`+ taskCompletionSource = new TaskCompletionSource<int>();`
	`25`	`+ }`
`21`	`26`
`22`	`27`	`void IDeleteCookiesCallback.OnComplete(int numDeleted)`
`23`	`28`	`{`
`24`		`- complete = true;`
	`29`	`+ onComplete = true;`
`25`	`30`
`26`	`31`	`taskCompletionSource.TrySetResultAsync(numDeleted);`
`27`	`32`	`}`
`@@ -35,14 +40,14 @@ bool IDeleteCookiesCallback.IsDisposed`
`35`	`40`	`{`
`36`	`41`	`get { return isDisposed; }`
`37`	`42`	`}`
`38`		`-`
`39`	`43`	`void IDisposable.Dispose()`
`40`	`44`	`{`
`41`	`45`	`var task = taskCompletionSource.Task;`
`42`	`46`
`43`		`- //If the Task hasn't completed and this is being disposed then`
`44`		`- //set the TCS to InvalidNoOfCookiesDeleted`
`45`		`- if (complete == false && task.IsCompleted == false)`
	`47`	`+ //If onComplete is false then IDeleteCookiesCallback.OnComplete was never called,`
	`48`	`+ //so we'll set the result to false. Calling TrySetResultAsync multiple times`
	`49`	`+ //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349`
	`50`	`+ if (onComplete == false && task.IsCompleted == false)`
`46`	`51`	`{`
`47`	`52`	`taskCompletionSource.TrySetResultAsync(InvalidNoOfCookiesDeleted);`
`48`	`53`	`}`
Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@ public sealed class TaskPrintToPdfCallback : IPrintToPdfCallback`
`12`	`12`	`{`
`13`	`13`	`private readonly TaskCompletionSource<bool> taskCompletionSource = new TaskCompletionSource<bool>();`
`14`	`14`	`private volatile bool isDisposed;`
`15`		`- private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety`
	`15`	`+ private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety`
`16`	`16`
`17`	`17`	`public Task<bool> Task`
`18`	`18`	`{`
`@@ -21,7 +21,7 @@ public Task<bool> Task`
`21`	`21`
`22`	`22`	`void IPrintToPdfCallback.OnPdfPrintFinished(string path, bool ok)`
`23`	`23`	`{`
`24`		`- complete = true;`
	`24`	`+ onComplete = true;`
`25`	`25`
`26`	`26`	`taskCompletionSource.TrySetResultAsync(ok);`
`27`	`27`	`}`
`@@ -35,9 +35,10 @@ void IDisposable.Dispose()`
`35`	`35`	`{`
`36`	`36`	`var task = taskCompletionSource.Task;`
`37`	`37`
`38`		`- //If the Task hasn't completed and this is being disposed then`
`39`		`- //set the TCS to false`
`40`		`- if (complete == false && task.IsCompleted == false)`
	`38`	`+ //If onComplete is false then IPrintToPdfCallback.OnPdfPrintFinished was never called,`
	`39`	`+ //so we'll set the result to false. Calling TrySetResultAsync multiple times`
	`40`	`+ //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349`
	`41`	`+ if (onComplete == false && task.IsCompleted == false)`
`41`	`42`	`{`
`42`	`43`	`taskCompletionSource.TrySetResultAsync(false);`
`43`	`44`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13,13 +13,18 @@ namespace CefSharp`
`13`	`13`	`/// </summary>`
`14`	`14`	`public class TaskRegisterCdmCallback : IRegisterCdmCallback`
`15`	`15`	`{`
`16`		`- private readonly TaskCompletionSource<CdmRegistration> taskCompletionSource = new TaskCompletionSource<CdmRegistration>();`
	`16`	`+ private readonly TaskCompletionSource<CdmRegistration> taskCompletionSource;`
`17`	`17`	`private volatile bool isDisposed;`
`18`		`- private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety`
	`18`	`+ private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety`
	`19`	`+`
	`20`	`+ public TaskRegisterCdmCallback()`
	`21`	`+ {`
	`22`	`+ taskCompletionSource = new TaskCompletionSource<CdmRegistration>();`
	`23`	`+ }`
`19`	`24`
`20`	`25`	`void IRegisterCdmCallback.OnRegistrationComplete(CdmRegistration registration)`
`21`	`26`	`{`
`22`		`- complete = true;`
	`27`	`+ onComplete = true;`
`23`	`28`
`24`	`29`	`taskCompletionSource.TrySetResultAsync(registration);`
`25`	`30`	`}`
`@@ -38,9 +43,10 @@ void IDisposable.Dispose()`
`38`	`43`	`{`
`39`	`44`	`var task = taskCompletionSource.Task;`
`40`	`45`
`41`		`- //If the Task hasn't completed and this is being disposed then`
`42`		`- //set the TCS to null`
`43`		`- if (complete == false && task.IsCompleted == false)`
	`46`	`+ //If onComplete is false then IRegisterCdmCallback.OnRegistrationComplete was never called,`
	`47`	`+ //so we'll set the result to false. Calling TrySetResultAsync multiple times`
	`48`	`+ //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349`
	`49`	`+ if (onComplete == false && task.IsCompleted == false)`
`44`	`50`	`{`
`45`	`51`	`taskCompletionSource.TrySetResultAsync(null);`
`46`	`52`	`}`
Original file line number	Diff line number	Diff line change
`@@ -11,39 +11,45 @@ namespace CefSharp`
`11`	`11`	`{`
`12`	`12`	`public class TaskResolveCallback : IResolveCallback`
`13`	`13`	`{`
`14`		`- private readonly TaskCompletionSource<ResolveCallbackResult> taskCompletionSource = new TaskCompletionSource<ResolveCallbackResult>();`
	`14`	`+ private readonly TaskCompletionSource<ResolveCallbackResult> taskCompletionSource;`
`15`	`15`	`private volatile bool isDisposed;`
`16`		`- private bool complete; //Only ever accessed on the same CEF thread, so no need for thread safety`
	`16`	`+ private bool onComplete; //Only ever accessed on the same CEF thread, so no need for thread safety`
	`17`	`+`
	`18`	`+ public TaskResolveCallback()`
	`19`	`+ {`
	`20`	`+ taskCompletionSource = new TaskCompletionSource<ResolveCallbackResult>();`
	`21`	`+ }`
`17`	`22`
`18`	`23`	`void IResolveCallback.OnResolveCompleted(CefErrorCode result, IList<string> resolvedIpAddresses)`
`19`	`24`	`{`
`20`		`- complete = true;`
	`25`	`+ onComplete = true;`
`21`	`26`
`22`	`27`	`taskCompletionSource.TrySetResultAsync(new ResolveCallbackResult(result, resolvedIpAddresses));`
`23`	`28`	`}`
`24`	`29`
`25`		`- void IDisposable.Dispose()`
	`30`	`+ bool IResolveCallback.IsDisposed`
`26`	`31`	`{`
`27`		`- var task = taskCompletionSource.Task;`
`28`		`-`
`29`		`- //If the Task hasn't completed and this is being disposed then`
`30`		`- //set the TCS to null`
`31`		`- if (complete == false && task.IsCompleted == false)`
`32`		`- {`
`33`		`- taskCompletionSource.TrySetResultAsync(new ResolveCallbackResult(CefErrorCode.Unexpected, null));`
`34`		`- }`
`35`		`-`
`36`		`- isDisposed = true;`
	`32`	`+ get { return isDisposed; }`
`37`	`33`	`}`
`38`	`34`
`39`	`35`	`public Task<ResolveCallbackResult> Task`
`40`	`36`	`{`
`41`	`37`	`get { return taskCompletionSource.Task; }`
`42`	`38`	`}`
`43`	`39`
`44`		`- bool IResolveCallback.IsDisposed`
	`40`	`+ void IDisposable.Dispose()`
`45`	`41`	`{`
`46`		`- get { return isDisposed; }`
	`42`	`+ var task = taskCompletionSource.Task;`
	`43`	`+`
	`44`	`+ //If onComplete is false then IResolveCallback.OnResolveCompleted was never called,`
	`45`	`+ //so we'll set the result to false. Calling TrySetResultAsync multiple times`
	`46`	`+ //can result in the issue outlined in https://github.com/cefsharp/CefSharp/pull/2349`
	`47`	`+ if (onComplete == false && task.IsCompleted == false)`
	`48`	`+ {`
	`49`	`+ taskCompletionSource.TrySetResultAsync(new ResolveCallbackResult(CefErrorCode.Unexpected, null));`
	`50`	`+ }`
	`51`	`+`
	`52`	`+ isDisposed = true;`
`47`	`53`	`}`
`48`	`54`	`}`
`49`	`55`	`}`