Double lock pattern of ClientDependencySettings.Instance can result in null reference exception.

[alan-strickland]

We recently had an issue with a asp.net web forms site that utilises this library where IIS was repeatedly crashing. This library did not cause the crash however it did highlight an issue with ClientDependencySettings and the Instance property where the double lock pattern was not safely implemented and could result in a thread throwing a null reference exception on the Providers property.

Example exception call stack:

System.Web.HttpUnhandledException (0x80004005): Exception of type 'System.Web.HttpUnhandledException' was thrown. ---> System.NullReferenceException: Object reference not set to an instance of an object.
at ClientDependency.Core.ProviderDependencyList.ProviderIs(BaseFileRegistrationProvider provider) in C:\Users\Shannon\Documents\_Projects\ClientDependency\ClientDependency\ClientDependency.Core\ProviderDependencyList.cs:line 20
at System.Linq.Enumerable.WhereListIterator`1.MoveNext()
at System.Linq.Enumerable.<DefaultIfEmptyIterator>d__93`1.MoveNext()
at System.Linq.Enumerable.SingleOrDefault[TSource](IEnumerable`1 source)
at ClientDependency.Core.BaseLoader.RegisterClientDependencies(BaseFileRegistrationProvider provider, IEnumerable`1 dependencies, IEnumerable`1 paths, ProviderCollection currProviders) in C:\Users\Shannon\Documents\_Projects\ClientDependency\ClientDependency\ClientDependency.Core\BaseLoader.cs:line 83
at ClientDependency.Core.BaseLoader.RegisterClientDependencies(List`1 dependencies, IEnumerable`1 paths) in C:\Users\Shannon\Documents\_Projects\ClientDependency\ClientDependency\ClientDependency.Core\BaseLoader.cs:line 154
at ClientDependency.Core.BaseLoader.RegisterDependency(Int32 group, Int32 priority, String filePath, String pathNameAlias, ClientDependencyType type, Object htmlAttributes, String provider, Boolean forceBundle) in C:\Users\Shannon\Documents\_Projects\ClientDependency\ClientDependency\ClientDependency.Core\BaseLoader.cs:line 307

The line that is throwing the exception is return Provider.Name == provider.Name; in ProviderDependencyList.

To resolve this I am calling the LoadProviders method prior to assigning the new settings instance to the _settings field.

It might be better to use Lazy<> here instead and remove the double lock pattern altogether, but the change as it is resolved the issue we had.

[Shazwazza]

Thanks @alan-strickland . The memory barrier is curious because the _settings is marked volatile so there should be a memory barrier in there already. But also your fix relies on an HttpContext to be there too?

This is an old project but agree it's simpler to use Lazy if possible.

[alan-strickland]

You're right I had missed the volatile keyword the memory barrier is not required.

I think it can rely on HttpContext because the empty constructor is also dependent on HttpContext and throws InvalidOperationException if HttpContext.Current is null. So I didn't think I was introducing a dependency that didn't already exist.

Something I can't understand is why private static Action _loadProviders = null is required. The method _loadProviders is called from each constructor apart from the empty constructor which is only used by the static property Instance which also calls _loadProviders, couldn't all the constructors just call LoadProviders.

As you say though its an old project and I didn't want to refactor it too much to resolve the problem I had.