I finally have a theory. I reproduced this with my own release build with symbols and got a more plausible stack trace (ironically), with ThreadCondition instead of ConditionVariable on the stack. 

InspectorTaskRunner has a ThreadCondition. On Windows, it's initialized thusly:
https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/wtf/threading_win.cc?rcl=f7de3ed33a3ecf175b858be5358c8fe60e58d2ed&l=178

The member var condition_ is initially garbage, so it's possible app verifier may think it has already been initialized when we call InitializeConditionVariable. 

Changing the constructor to look like this:
ThreadCondition::ThreadCondition(Mutex& mutex)
    : condition_(CONDITION_VARIABLE_INIT), mutex_(mutex.Impl()) {}

seems to fix the problem.  Why app verifier is complaining about AVrfpRtlInitializeSRWLock is mysterious. Bruce, is this plausible? I think there are a few other places where we try to initialize locks and cv's that are member variables that could conceivably trigger app verifier but I want to make sure I'm on the right track first...

I think that App verifier's detection of multiple initialization is based on maintaining a database of objects that have been initialized, so it shouldn't be vulnerable to false positives. Do you have a crash dump showing the complaint? Attaching that plus a text file containing the debugger spew should be helpful in understanding what is really going on.

If it's based on the address of the object that's been initialized, why wouldn't that be vulnerable to false positives, for objects on the heap, as opposed to global vars?

If there is a de-initialize function as well as an initialize function then they could maintain the database. Otherwise not. Since InitializeSRWLock doesn't have a corresponding deinitialize function I guess you are right that false positive are unavoidable. That is disappointing.

So maybe this is bogus, in which case we need to disable the appropriate check, which then reduces the value of app verifier.

Aside: for Application Verifier bugs it is always a good idea to include the output spew as well as the call stack, as that explains exactly what rule has been broken.

I'll attach the spew when I get a chance - running app verifier on chrome is the mother of all context switches.  Oh, wait, I forgot I put it in a doc:

VERIFIER STOP 0000000000000251: pid 0x4CFC: The SRW Lock is already initialized. 

	00002337E88BE9D0 : SRW Lock
	0000000000001D1C : ThreadId of the thread that initialized the SRW lock.
	00000273BCD17FD0 : Address of the initialization stack trace. Use dps <address> to see where the SRW lock was initialized.
	0000000000000000 : Not used


When I ran under windbg, dps <address> claimed the initializing stack trace was exactly the same as the error stack trace.

I don't think we need to disable this check if we initialize the member variables like this : condition_(CONDITION_VARIABLE_INIT)

Got it. condition variables can be intialized with condition_(CONDITION_VARIABLE_INIT) and srwlock variables can be initialized with srwlock_(SRWLOCK_INIT).

It's a bit weird that the error message references SRW lock but the fix is for a condition variable, but ???

Right re initializing the vars. I don't know why the error message references SRW locks (something internal in Windows?), but my proposed change seems to fix the problem.

The only weird thing is that I cannot reproduce this in a standalone app. I wonder what the difference is?

In my test (attached) I have SRWLock and ConditionVariable objects and they call the init functions and the addresses are necessarily reused (the stack ones for sure, the heap ones almost certainly) and with App Verifier enabled I can't trigger the warning. If I could I'd report a bug.

Anyway, initializing with CONDITION_VARIABLE_INIT is legitimate, so we might as well, but I wish I knew whether there was something more to the behavior.

Deleted: DoubleInitIsNotACrime.zip 3.8 KB

DoubleInitIsNotACrime.zip 3.8 KB Download

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afe862d373bf453c8d5806148ebd1145711ad57c

commit afe862d373bf453c8d5806148ebd1145711ad57c
Author: David Bienvenu <davidbienvenu@chromium.org>
Date: Fri Oct 26 13:14:22 2018

fix app verify exception with condition var

Use static initializer for condition_ member var instead of win api call.

Bug:  891924 
Change-Id: I61860a13f286389cf955a74d8e4b3e5c3d30bbb9
Reviewed-on: https://chromium-review.googlesource.com/c/1299916
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: David Bienvenu <davidbienvenu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603069}
[modify] https://crrev.com/afe862d373bf453c8d5806148ebd1145711ad57c/third_party/blink/renderer/platform/wtf/threading_win.cc

This should be fixed - I'll try app verifier with the next canary build.

Thanks for the fix. I finally have a repro of App Verifier detecting this. I've included my entire test program below but the one that it triggers on is the ConditionVariable on the heap. Here are the rules as far as I can tell:
1) This error doesn't fire for addresses on the stack.
2) This error doesn't fire if the contents of the CONDITION_VARIABLE are zero
3) This error doesn't fire if the memory has been freed by the heap in-between calls to InitializeConditionVariable

So, this Application Verifier bug should only be hit under normal circumstances if you use a custom heap or otherwise somehow reuse memory without going through HeapFree. It's not totally clear how Chrome was hitting this but I don't *think* it represents a Chrome bug.

Here's my test code:


#include <Windows.h>
#include <iostream>
#include <memory>

class SRWLock {
public:
  SRWLock() {
    for (int i = 0; i < 100; ++i)
      InitializeSRWLock(&lock_);
  }

private:
  SRWLOCK lock_;
};

class ConditionVariable {
public:
  ConditionVariable() {
    InitializeConditionVariable(&lock_);
    size_t x = 1;
    memcpy(&lock_, &x, sizeof(lock_));
    InitializeConditionVariable(&lock_);
  }
  ~ConditionVariable() {
  }
private:
  CONDITION_VARIABLE lock_;
};

int main() {
  for (int i = 0; i < 1000; ++i) {
    SRWLock s;
    ConditionVariable c;
    auto p1 = std::make_unique<SRWLock>();
    auto p2 = std::make_unique<ConditionVariable>();
  }
}

Cool, interesting about the HeapFree thing.