Fix Crst ordering issue with SendMethodDetailsEvent

[sywhang]

Fix #643.

This fixes a crst ordering issue between CrstEtwTypeHashLock and CrstSingleUseLock.

EtwTypeHashLock protects an internal hash table we use for caching type infos we use for tracing type events. This really shouldn't be acquired after all the locks that were previously listed, but was doing so because of constructors to TypeLoggingInfo and LoggedTypesFromModule.

This PR pulls out the calls to these constructors from the lock, since they don't need to be inside the lock.

[noahfalk]

This change left a few race conditions unhandled (comments below). Other than that looks good : )

[noahfalk]

Assuming that multiple threads can call this method at the same time then it is possible the order of operations will look like this:
Thread 1: s_pAllLoggedTypes->allLoggedTypesHash.Lookup(pLoaderModule) returns NULL
Thread 2: s_pAllLoggedTypes->allLoggedTypesHash.Lookup(pLoaderModule) returns NULL
Thread 1: s_pAllLoggedTypes->allLoggedTypesHash.Add(pLoggedTypesFromModule)
Thread 1: add types to thread1's pLoggedTypesFromModule
Thread 2: s_pAllLoggedTypes->allLoggedTypesHash.Add(pLoggedTypesFromModule)

At this point the types that thread 1 added have been lost because thread 2 replaced the module logged types with an empty instance when it called Add(). The typical solution is to write the code like this:

Thingy* pThingy = null;
{
    CrstHolder()
    pThingy = dictionary.Lookup(key);
}
if(pThingy == null) 
{
    pThingy = CreateNewThingy();
    {
        CrstHolder();
        Thingy* pThingyRecheck = dictionary.Lookup(key);
        if(pThingyRecheck == null) // usual case, no other thread created it first
        {
            dictionary.Add(key, pThingy);
        }
        else // rare case, another thread already added it when we left the lock
        {
            FreeThingy(pThingy);
            pThingy = pThingyRecheck;
        }
    }
}

A 2nd minor issue, the current code calls Add() every time, even if the initial lookup was successful. It works, but it is a little unexpected and does unneeded work. My snippet above only calls Add() if the initial lookup fails.

[noahfalk]

Same issue as above, another Lookup() is necessary to ensure that a 2nd thread has not already added this item while the lock wasn't held.

[davidwrighton]

See my comment around incorrect use the the SHash Add function.

[davidwrighton]

This isn't correct. If in the intervening time between a allLoggedTypesHash.Lookup call above, and this Add, another thread may have added to the allLoggedTypesHash. In that case, this Add will result in multiple entries in the SHash. That itself isn't a problem as SHash supports that, and for the ETW event stream, multiple BulkType events emitted won't ever cause a failure, but the logic in TypeSystemLog::OnModuleUnload won't then correctly delete all of the LoggedTypesFromModule objects, and if a new module is loaded as the same memory address as the module that wasn't properly cleaned up, then incorrect behavior may be observed.

Also, it appears that this unconditionally adds the pLoggedTypesFromModule to the hash even if it was found via Lookup above. That should not be done with a SHash based hash table.

Instead the logic should use a NewHolder to hold the newly allocated LoggedTypesFromModule, and then take the critical section, and attempt to add the newly allocated LoggedTypesFromModule object into the hashtable, by taking a lock, attempting to Lookup, if the lookup fails, then Adding, and releasing the NewHolder.

[sywhang]

Closing this because it's been a while since I worked on this and the merge conflicts have gotten sufficiently large that working from a separate branch to address the fixes suggested from this PR was better... I opened #36932 from the new branch.