From b7df5302641db51a2e0be37c1945617186e06165 Mon Sep 17 00:00:00 2001 From: louis Date: Mon, 9 Apr 2018 13:02:24 +0200 Subject: [PATCH 1/3] Fix non-thread-safety of TrySetResultAsync The extension method TrySetResultAsync has non-deterministic results when called twice on the same instance, even on the same thread: it creates one task for each call, which may not run in their creation order. This commit adds a wrapper around TaskCompletionSource, that ensures that only the first call to TrySetResultAsync is really executed. It also updates the test case that could be violated with the previous behavior (but that was hard to trigger with only one test). --- CefSharp.OffScreen/ChromiumWebBrowser.cs | 2 +- .../Framework/AsyncExtensionFacts.cs | 25 +++++++------ CefSharp/Callback/TaskCompletionCallback.cs | 4 +-- .../Callback/TaskDeleteCookiesCallback.cs | 4 +-- CefSharp/Callback/TaskPrintToPdfCallback.cs | 2 +- CefSharp/Callback/TaskRegisterCdmCallback.cs | 4 +-- CefSharp/Callback/TaskResolveCallback.cs | 4 +-- CefSharp/Callback/TaskSetCookieCallback.cs | 4 +-- CefSharp/CefSharp.csproj | 1 + .../Internals/AsyncTaskCompletionSource.cs | 35 +++++++++++++++++++ CefSharp/Internals/TaskExtensions.cs | 8 ++++- CefSharp/Visitor/TaskCookieVisitor.cs | 4 +-- .../Visitor/TaskNavigationEntryVisitor.cs | 4 +-- CefSharp/Visitor/TaskStringVisitor.cs | 4 +-- CefSharp/Visitor/TaskWebPluginInfoVisitor.cs | 4 +-- 15 files changed, 77 insertions(+), 32 deletions(-) create mode 100644 CefSharp/Internals/AsyncTaskCompletionSource.cs diff --git a/CefSharp.OffScreen/ChromiumWebBrowser.cs b/CefSharp.OffScreen/ChromiumWebBrowser.cs index 8fd4845654..d7550c242d 100644 --- a/CefSharp.OffScreen/ChromiumWebBrowser.cs +++ b/CefSharp.OffScreen/ChromiumWebBrowser.cs @@ -507,7 +507,7 @@ public Task ScreenshotAsync(bool ignoreExistingScreenshot = false, Popup // Try our luck and see if there is already a screenshot, to save us creating a new thread for nothing. var screenshot = ScreenshotOrNull(blend); - var completionSource = new TaskCompletionSource(); + var completionSource = new AsyncTaskCompletionSource(); if (screenshot == null || ignoreExistingScreenshot) { diff --git a/CefSharp.Test/Framework/AsyncExtensionFacts.cs b/CefSharp.Test/Framework/AsyncExtensionFacts.cs index cabba06c61..4a0b907f35 100644 --- a/CefSharp.Test/Framework/AsyncExtensionFacts.cs +++ b/CefSharp.Test/Framework/AsyncExtensionFacts.cs @@ -36,22 +36,25 @@ public async Task DeleteCookiesReturnsTaskWhenReturnsFalse() [Fact] public async Task TaskDeleteCookiesCallbackOnComplete() { - const int numberOfCookiesDeleted = 10; + for (var i = 0; i < 100; i++) + { + const int numberOfCookiesDeleted = 10; - var callback = new TaskDeleteCookiesCallback(); + var callback = new TaskDeleteCookiesCallback(); - //Execute OnComplete on seperate Thread as in practice will be called on the CEF IO Thread. - Task.Delay(100).ContinueWith(x => - { - var c = (IDeleteCookiesCallback)callback; + //Execute OnComplete on seperate Thread as in practice will be called on the CEF IO Thread. + Task.Delay(100).ContinueWith(x => + { + var c = (IDeleteCookiesCallback)callback; - c.OnComplete(numberOfCookiesDeleted); - c.Dispose(); - }, TaskScheduler.Default); + c.OnComplete(numberOfCookiesDeleted); + c.Dispose(); + }, TaskScheduler.Default); - var result = await callback.Task; + var result = await callback.Task; - Assert.Equal(numberOfCookiesDeleted, result); + Assert.Equal(numberOfCookiesDeleted, result); + } } [Fact] diff --git a/CefSharp/Callback/TaskCompletionCallback.cs b/CefSharp/Callback/TaskCompletionCallback.cs index a419a2daaf..4b3272ee27 100644 --- a/CefSharp/Callback/TaskCompletionCallback.cs +++ b/CefSharp/Callback/TaskCompletionCallback.cs @@ -10,11 +10,11 @@ namespace CefSharp { public class TaskCompletionCallback : ICompletionCallback { - private readonly TaskCompletionSource taskCompletionSource; + private readonly AsyncTaskCompletionSource taskCompletionSource; public TaskCompletionCallback() { - taskCompletionSource = new TaskCompletionSource(); + taskCompletionSource = new AsyncTaskCompletionSource(); } void ICompletionCallback.OnComplete() diff --git a/CefSharp/Callback/TaskDeleteCookiesCallback.cs b/CefSharp/Callback/TaskDeleteCookiesCallback.cs index 930beb416f..915bd64d39 100644 --- a/CefSharp/Callback/TaskDeleteCookiesCallback.cs +++ b/CefSharp/Callback/TaskDeleteCookiesCallback.cs @@ -15,12 +15,12 @@ public class TaskDeleteCookiesCallback : IDeleteCookiesCallback { public const int InvalidNoOfCookiesDeleted = -1; - private readonly TaskCompletionSource taskCompletionSource; + private readonly AsyncTaskCompletionSource taskCompletionSource; private volatile bool isDisposed; public TaskDeleteCookiesCallback() { - taskCompletionSource = new TaskCompletionSource(); + taskCompletionSource = new AsyncTaskCompletionSource(); } void IDeleteCookiesCallback.OnComplete(int numDeleted) diff --git a/CefSharp/Callback/TaskPrintToPdfCallback.cs b/CefSharp/Callback/TaskPrintToPdfCallback.cs index ad3ae2c053..6ab62947dc 100644 --- a/CefSharp/Callback/TaskPrintToPdfCallback.cs +++ b/CefSharp/Callback/TaskPrintToPdfCallback.cs @@ -10,7 +10,7 @@ namespace CefSharp { public sealed class TaskPrintToPdfCallback : IPrintToPdfCallback { - private readonly TaskCompletionSource taskCompletionSource = new TaskCompletionSource(); + private readonly AsyncTaskCompletionSource taskCompletionSource = new AsyncTaskCompletionSource(); public Task Task { diff --git a/CefSharp/Callback/TaskRegisterCdmCallback.cs b/CefSharp/Callback/TaskRegisterCdmCallback.cs index d9c00a4579..ad93378125 100644 --- a/CefSharp/Callback/TaskRegisterCdmCallback.cs +++ b/CefSharp/Callback/TaskRegisterCdmCallback.cs @@ -13,11 +13,11 @@ namespace CefSharp /// public class TaskRegisterCdmCallback: IRegisterCdmCallback { - private readonly TaskCompletionSource taskCompletionSource; + private readonly AsyncTaskCompletionSource taskCompletionSource; public TaskRegisterCdmCallback() { - taskCompletionSource = new TaskCompletionSource(); + taskCompletionSource = new AsyncTaskCompletionSource(); } void IRegisterCdmCallback.OnRegistrationComplete(CdmRegistration registration) diff --git a/CefSharp/Callback/TaskResolveCallback.cs b/CefSharp/Callback/TaskResolveCallback.cs index 822be9ada3..48c23f58c5 100644 --- a/CefSharp/Callback/TaskResolveCallback.cs +++ b/CefSharp/Callback/TaskResolveCallback.cs @@ -10,11 +10,11 @@ namespace CefSharp { public class TaskResolveCallback : IResolveCallback { - private readonly TaskCompletionSource taskCompletionSource; + private readonly AsyncTaskCompletionSource taskCompletionSource; public TaskResolveCallback() { - taskCompletionSource = new TaskCompletionSource(); + taskCompletionSource = new AsyncTaskCompletionSource(); } public void OnResolveCompleted(CefErrorCode result, IList resolvedIpAddresses) diff --git a/CefSharp/Callback/TaskSetCookieCallback.cs b/CefSharp/Callback/TaskSetCookieCallback.cs index 1ddb3de4ce..23be8a152c 100644 --- a/CefSharp/Callback/TaskSetCookieCallback.cs +++ b/CefSharp/Callback/TaskSetCookieCallback.cs @@ -13,12 +13,12 @@ namespace CefSharp /// public class TaskSetCookieCallback : ISetCookieCallback { - private readonly TaskCompletionSource taskCompletionSource; + private readonly AsyncTaskCompletionSource taskCompletionSource; private volatile bool isDisposed; public TaskSetCookieCallback() { - taskCompletionSource = new TaskCompletionSource(); + taskCompletionSource = new AsyncTaskCompletionSource(); } void ISetCookieCallback.OnComplete(bool success) diff --git a/CefSharp/CefSharp.csproj b/CefSharp/CefSharp.csproj index 0bb5bf50f8..dde9b882d5 100644 --- a/CefSharp/CefSharp.csproj +++ b/CefSharp/CefSharp.csproj @@ -72,6 +72,7 @@ + diff --git a/CefSharp/Internals/AsyncTaskCompletionSource.cs b/CefSharp/Internals/AsyncTaskCompletionSource.cs new file mode 100644 index 0000000000..31646f4b15 --- /dev/null +++ b/CefSharp/Internals/AsyncTaskCompletionSource.cs @@ -0,0 +1,35 @@ +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace CefSharp +{ + /// + /// Wraps by providing a way to set its result in an async fashion. + /// This prevents the Task Continuation being executed sync on the same thread, which may be required when + /// continuations must not run on CEF UI threads. + /// + /// + public class AsyncTaskCompletionSource + { + private readonly TaskCompletionSource tcs = new TaskCompletionSource(); + private int resultSet = 0; + + /// + /// Gets the created by this + /// + /// + public Task Task => tcs.Task; + + /// + /// Set the TaskCompletionSource in an async fashion. This prevents the Task Continuation being executed sync on the same thread + /// This is required otherwise contintinuations will happen on CEF UI threads + /// + /// result + public void TrySetResultAsync(T result) + { + if (Interlocked.Exchange(ref resultSet, 1) == 0) + System.Threading.Tasks.Task.Factory.StartNew(() => tcs.TrySetResult(result), CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); + } + } +} diff --git a/CefSharp/Internals/TaskExtensions.cs b/CefSharp/Internals/TaskExtensions.cs index 26498462c0..e1912ce793 100644 --- a/CefSharp/Internals/TaskExtensions.cs +++ b/CefSharp/Internals/TaskExtensions.cs @@ -89,8 +89,14 @@ public static TaskCompletionSource WithTimeout(this TaskComple } /// + /// /// Set the TaskCompletionSource in an async fashion. This prevents the Task Continuation being executed sync on the same thread - /// This is required otherwise contintinuations will happen on CEF UI threads + /// This is required otherwise contintinuations will happen on CEF UI threads. + /// + /// + /// USAGE WARNING: if this method is called twice on the same task completion source, the result it not deterministic. + /// Use it with care, or use . + /// /// /// Generic param /// tcs diff --git a/CefSharp/Visitor/TaskCookieVisitor.cs b/CefSharp/Visitor/TaskCookieVisitor.cs index 41e8135eb9..ee35c68333 100644 --- a/CefSharp/Visitor/TaskCookieVisitor.cs +++ b/CefSharp/Visitor/TaskCookieVisitor.cs @@ -15,7 +15,7 @@ namespace CefSharp /// public class TaskCookieVisitor : ICookieVisitor { - private readonly TaskCompletionSource> taskCompletionSource; + private readonly AsyncTaskCompletionSource> taskCompletionSource; private List list; /// @@ -23,7 +23,7 @@ public class TaskCookieVisitor : ICookieVisitor /// public TaskCookieVisitor() { - taskCompletionSource = new TaskCompletionSource>(); + taskCompletionSource = new AsyncTaskCompletionSource>(); list = new List(); } diff --git a/CefSharp/Visitor/TaskNavigationEntryVisitor.cs b/CefSharp/Visitor/TaskNavigationEntryVisitor.cs index 9c30f2069a..515db67f26 100644 --- a/CefSharp/Visitor/TaskNavigationEntryVisitor.cs +++ b/CefSharp/Visitor/TaskNavigationEntryVisitor.cs @@ -15,7 +15,7 @@ namespace CefSharp /// public class TaskNavigationEntryVisitor : INavigationEntryVisitor { - private TaskCompletionSource> taskCompletionSource; + private AsyncTaskCompletionSource> taskCompletionSource; private List list; /// @@ -23,7 +23,7 @@ public class TaskNavigationEntryVisitor : INavigationEntryVisitor /// public TaskNavigationEntryVisitor() { - taskCompletionSource = new TaskCompletionSource>(); + taskCompletionSource = new AsyncTaskCompletionSource>(); list = new List(); } diff --git a/CefSharp/Visitor/TaskStringVisitor.cs b/CefSharp/Visitor/TaskStringVisitor.cs index 2dde476a8f..a3ef18fd78 100644 --- a/CefSharp/Visitor/TaskStringVisitor.cs +++ b/CefSharp/Visitor/TaskStringVisitor.cs @@ -14,14 +14,14 @@ namespace CefSharp /// public class TaskStringVisitor : IStringVisitor { - private readonly TaskCompletionSource taskCompletionSource; + private readonly AsyncTaskCompletionSource taskCompletionSource; /// /// Default constructor /// public TaskStringVisitor() { - taskCompletionSource = new TaskCompletionSource(); + taskCompletionSource = new AsyncTaskCompletionSource(); } /// diff --git a/CefSharp/Visitor/TaskWebPluginInfoVisitor.cs b/CefSharp/Visitor/TaskWebPluginInfoVisitor.cs index 8521fcf19a..7412e69cf3 100644 --- a/CefSharp/Visitor/TaskWebPluginInfoVisitor.cs +++ b/CefSharp/Visitor/TaskWebPluginInfoVisitor.cs @@ -11,12 +11,12 @@ namespace CefSharp { public class TaskWebPluginInfoVisitor : IWebPluginInfoVisitor { - private TaskCompletionSource> taskCompletionSource; + private AsyncTaskCompletionSource> taskCompletionSource; private List list; public TaskWebPluginInfoVisitor() { - taskCompletionSource = new TaskCompletionSource>(); + taskCompletionSource = new AsyncTaskCompletionSource>(); list = new List(); } From 00c27e5afd5bf1ace6d5b6ac7451e7081c7cd256 Mon Sep 17 00:00:00 2001 From: louis Date: Mon, 9 Apr 2018 13:09:37 +0200 Subject: [PATCH 2/3] code style fix code style --- CefSharp/Internals/AsyncTaskCompletionSource.cs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/CefSharp/Internals/AsyncTaskCompletionSource.cs b/CefSharp/Internals/AsyncTaskCompletionSource.cs index 31646f4b15..b558bf3c24 100644 --- a/CefSharp/Internals/AsyncTaskCompletionSource.cs +++ b/CefSharp/Internals/AsyncTaskCompletionSource.cs @@ -1,4 +1,8 @@ -using System; +// Copyright © 2010-2017 The CefSharp Authors. All rights reserved. +// +// Use of this source code is governed by a BSD-style license that can be found in the LICENSE file. + +using System; using System.Threading; using System.Threading.Tasks; @@ -22,14 +26,15 @@ public class AsyncTaskCompletionSource public Task Task => tcs.Task; /// - /// Set the TaskCompletionSource in an async fashion. This prevents the Task Continuation being executed sync on the same thread - /// This is required otherwise contintinuations will happen on CEF UI threads + /// Set the TaskCompletionSource in an async fashion. This prevents the Task Continuation being executed + /// sync on the same thread. This is required otherwise contintinuations will happen on CEF UI threads. /// /// result public void TrySetResultAsync(T result) { if (Interlocked.Exchange(ref resultSet, 1) == 0) - System.Threading.Tasks.Task.Factory.StartNew(() => tcs.TrySetResult(result), CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); + System.Threading.Tasks.Task.Factory.StartNew(() => tcs.TrySetResult(result), + CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); } } } From ec98f0d01b959d3782d97304e4f68428de23a74f Mon Sep 17 00:00:00 2001 From: louis Date: Mon, 9 Apr 2018 14:56:11 +0200 Subject: [PATCH 3/3] comments and code style --- CefSharp/Internals/AsyncTaskCompletionSource.cs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/CefSharp/Internals/AsyncTaskCompletionSource.cs b/CefSharp/Internals/AsyncTaskCompletionSource.cs index b558bf3c24..65d1cf7fd1 100644 --- a/CefSharp/Internals/AsyncTaskCompletionSource.cs +++ b/CefSharp/Internals/AsyncTaskCompletionSource.cs @@ -9,9 +9,9 @@ namespace CefSharp { /// - /// Wraps by providing a way to set its result in an async fashion. - /// This prevents the Task Continuation being executed sync on the same thread, which may be required when - /// continuations must not run on CEF UI threads. + /// Wraps by providing a way to set its result in an asynchronous + /// fashion. This prevents the resulting Task continuations from being executed synchronously on the same + /// thread as the one completing the Task, which may be better when that thread is a CEF UI thread. /// /// public class AsyncTaskCompletionSource @@ -20,21 +20,24 @@ public class AsyncTaskCompletionSource private int resultSet = 0; /// - /// Gets the created by this + /// Gets the created by the wrapped instance. /// /// public Task Task => tcs.Task; /// - /// Set the TaskCompletionSource in an async fashion. This prevents the Task Continuation being executed - /// sync on the same thread. This is required otherwise contintinuations will happen on CEF UI threads. + /// Sets the wrapped instance result in an asynchronous fashion. + /// This prevents the Task continuations from being executed synchronously on the same thread as the + /// one calling this method, which is required when this thread is a CEF UI thread. /// /// result public void TrySetResultAsync(T result) { if (Interlocked.Exchange(ref resultSet, 1) == 0) + { System.Threading.Tasks.Task.Factory.StartNew(() => tcs.TrySetResult(result), CancellationToken.None, TaskCreationOptions.None, TaskScheduler.Default); + } } } }