Use ConcurrentDictionary for RemoteWeakMap

Issue #140 resolved
David Desmaisons
created an issue

On some Neutronium scenario making a heavy usage of Chromium Fx, creating a new CfrTask or a CfrV8Value may result in a IndexOutOfRangeException thrown by the dictionary.Add of the RemoteWeakCache.Add.

This is most likely due to concurent access to the RemoteWeakCache dictionary .

The suggestion is to use a ConcurrentDictionary as dictionary implementation or add a lock on insertion (as it is done on remove).

If you agree with the proposal, I can create a pull request.

Thanks

Comments (6)

  1. David Desmaisons reporter

    Here is an example of corresponding callstack:

    An exception of type 'System.IndexOutOfRangeException' occurred in mscorlib.dll but was not handled in user code

       em System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
       em System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
       em Chromium.Remote.RemoteWeakCache.Add(IntPtr key, Object obj)
       em Chromium.Remote.CfrTask..ctor()
       em Neutronium.WebBrowserEngine.ChromiumFx.EngineBinding.ChromiumFxTask..ctor(Action perform)
    
  2. David Desmaisons reporter

    Update: Adding a lock this way:

    internal class RemoteWeakCache : WeakCacheBase {
                internal void Add(IntPtr key, object obj) {
                    // always locked by caller
                    lock (this){
                        cache.Add(key, new WeakReference(obj, false));
                    }
                }
            }
    

    Solved the issue.

  3. wborgsm

    While your solution is correct, the general contract for the weak cache is to lock on it before calling any of it's methods. That's what the "always locked by caller" comment means. In .NET 4.5 I would add a Debug.Assert(Monitor.IsEntered(this)) at that point.

    It works like this because some code (e.g. CfrV8Value::Wrap) needs to access Get and Add within the same synchronized block - and to avoid the overhead of locking twice, callers must lock before calling.

    So the offending code is in the client class constructors, for instance CfrTask::CfrTask.

    This is fixed in commit 7449b0f. Thank you for spotting this.

  4. Log in to comment