4

The code below spawns a thread that waits for 5 seconds before iterating (recursively) over all the accessibles (widgets) in the foreground application.

If (during the 5 second delay) I switch to a Windows 10 Metro app (like Calc or Edge) then the call to CoUninitialize in the main thread will result in an access violation. Why?

#include <future>
#include <chrono>

#include <windows.h>
#include <oleacc.h>
#pragma comment(lib,"Oleacc.lib")

// Adapted from https://msdn.microsoft.com/en-us/library/windows/desktop/dd317975%28v=vs.85%29.aspx
HRESULT WalkTreeWithAccessibleChildren(IAccessible* pAcc, int depth)
{
  HRESULT hr;
  long childCount;
  long returnCount;

  if (!pAcc)
  {
    return E_INVALIDARG;
  }
  hr = pAcc->get_accChildCount(&childCount);
  if (FAILED(hr))
  {
    return hr;
  };
  if (childCount == 0)
  {
    return S_FALSE;
  }
  VARIANT* pArray = new VARIANT[childCount];
  hr = AccessibleChildren(pAcc, 0L, childCount, pArray, &returnCount);
  if (FAILED(hr))
  {
    return hr;
  };

  // Iterate through children.
  for (int x = 0; x < returnCount; x++)
  {
    VARIANT vtChild = pArray[x];
    // If it's an accessible object, get the IAccessible, and recurse.
    if (vtChild.vt == VT_DISPATCH)
    {
      IDispatch* pDisp = vtChild.pdispVal;
      IAccessible* pChild = NULL;
      hr = pDisp->QueryInterface(IID_IAccessible, (void**)&pChild);
      if (hr == S_OK)
      {
        WalkTreeWithAccessibleChildren(pChild, depth + 1);
        pChild->Release();
      }
      pDisp->Release();
    }
  }
  delete[] pArray;
  return S_OK;
}

int main(int argc, char *argv[])
{
  CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED);

  auto future = std::async(std::launch::async,
    []
    {
      // Switch to a Windows 10 Metro app like the Calculator or Edge.
      std::this_thread::sleep_for(std::chrono::milliseconds(5000));

      auto hwnd = GetForegroundWindow();
      if (!hwnd) abort();

      CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED);
      IAccessible* pAcc = NULL;
      HRESULT hr = AccessibleObjectFromWindow(hwnd, OBJID_CLIENT, IID_IAccessible, (void**)&pAcc);
      if (hr == S_OK) {
        WalkTreeWithAccessibleChildren(pAcc, 0);
        pAcc->Release();
      }
      CoUninitialize();
    }
  );
  future.wait();

  CoUninitialize();
}

The error message is:

Unhandled exception at 0x7722B9E7 (combase.dll) in Test.exe: 0xC0000005: Access violation reading location 0x00000008.

bwiwov
  • 51
  • 6
  • 1
    Does it still do it if you comment out the call to `WalkTreeWithAccessibleChildren` ? – Jonathan Potter Feb 18 '16 at 10:41
  • 1
    Fibbing about the apartment promise is never not a mistake. It *looks* like the worker thread is leaking and the final CoUninitialize() tries to cleanup. But fails at it, the leaked object no longer has an owner thread because the worker thread called CoUninitialize(). Either commenting CoUninitialize() in the worker or starting the main thread with COINIT_MULTITHREADED fixes the problem. Finding the leak is going to be unpractical, there is a mountain of code underneath. Ugly, probably best to avoid a worker thread completely. – Hans Passant Feb 18 '16 at 10:50
  • The COM library is reference counted, so do not skip `CoUninitialize()`. You should pay attention to the return value of `CoInitialize/Ex()` and only call `CoUninitialize()` if the return value is `S_OK` or `S_FALSE`. That being said, a thread that uses `COINIT_APARTMENTTHREADED` must have a message loop. I don't see that in this code. I also don't see any object leaks in this code (though there is a small memory leak if `AccessibleChildren()` fails). I would not suggest using `abort()`, though. Just `return` instead. – Remy Lebeau Feb 18 '16 at 20:10
  • @JonathanPotter No it doesn't happen if I don't exercise the accessibility API. – bwiwov Feb 22 '16 at 00:29
  • @HansPassant I incorporated all three of your hacks into the revised code. Unfortunately changing the apartment type of the main thread would be difficult (my real code is using a library that must run in the main thread and requires STA). It would also be difficult to eliminate the worker thread (in my real code the worker runs time-consuming code). Currently I'm leaning towards your suggestion to comment out the CoUninitialize (actually the CoUninitialize is in a library that I don't have control over, but I achieve the same affect with an unbalanced CoInitialize). – bwiwov Feb 22 '16 at 00:37
  • @RemyLebeau Checking the return value indicated that CoInitialize was in fact failing because of apartment-type mismatch. However even after fixing this problem I still get access violations. I tried processing events in the main thread (while waiting for the worker to complete) but it didn't help. – bwiwov Feb 22 '16 at 00:48

1 Answers1

1

As per @RemyLebeau's recommendation, I added code to check the return value of CoInitialize(nullptr, COINIT_APARTMENTTHREADED) in the lambda. Turns out it was failing with 0x80010106 (Cannot change thread mode after it is set). It failed with this value even if I shuffled the code around and made the call at the very beginning of the lambda. This suggests that MSVS's implementation of std::async actually creates a multi-threaded apartment in the thread before calling the lambda (wtf!). In the end I was able to avoid this problem by using WINAPI directly (i.e. CreateThread) which doesn't suffer from this behavior. This fix alone though, was not sufficient to prevent the access violations.

I haven't yet discovered a way to properly fix the access violation but I have discovered several hacks that prevent it from happening:

  1. Create a window and show it. Note: Creating the window in itself is not sufficient, it actually needs to be visible.
  2. Configure CoInitializeEx in the main thread as COINIT_MULTITHREADED. Note: Configuring CoInitializeEx in the worker thread as COINIT_MULTITHREADED does not help.
  3. Do the enumeration in the main thread and forgo a worker thread altogether.
  4. Wait >15 seconds after the worker thread completes before calling CoUninitialize.
  5. Insert an extra (unmatched) call to CoInitialize, to ensure that the reference count never drops to 0 and therefore COM is never really uninitialized.

Unfortunately hacks 1-3 are not feasible in the real-world code that this test-case is based on. I'm loathe to force the user to wait >15 seconds for the application to exit. Therefore right now I'm leaning towards hack #5.

Any resource leaks in the client itself are not that important since the process will exit and the resources will be reclaimed by the operating system (although it will frustrate any leak testing). What is important is that it causes the accessibility server (MicrosoftEdge.exe) to leak a few kB of memory most times I run the test case.

The revised code implements the CreateThread fix as well as all 5 'hacks'. At least one of the hacks must be enabled to prevent the access violation:

#define HACK 0 // Set this between 1-5 to enable one of the hacks.

#include <future>
#include <chrono>

#include <windows.h>
#include <oleacc.h>
#pragma comment(lib,"Oleacc.lib")

// Adapted from https://msdn.microsoft.com/en-us/library/windows/desktop/dd317975%28v=vs.85%29.aspx
HRESULT WalkTreeWithAccessibleChildren(IAccessible* pAcc, int depth)
{
  HRESULT hr;
  long childCount;
  long returnCount;

  if (!pAcc)
  {
    return E_INVALIDARG;
  }
  hr = pAcc->get_accChildCount(&childCount);
  if (FAILED(hr))
  {
    return hr;
  };
  if (childCount == 0)
  {
    return S_FALSE;
  }
  VARIANT* pArray = new VARIANT[childCount];
  hr = AccessibleChildren(pAcc, 0L, childCount, pArray, &returnCount);
  if (FAILED(hr))
  {
    delete[] pArray;
    return hr;
  };

  // Iterate through children.
  for (int x = 0; x < returnCount; x++)
  {
    VARIANT vtChild = pArray[x];
    // If it's an accessible object, get the IAccessible, and recurse.
    if (vtChild.vt == VT_DISPATCH)
    {
      IDispatch* pDisp = vtChild.pdispVal;
      IAccessible* pChild = NULL;
      hr = pDisp->QueryInterface(IID_IAccessible, (void**)&pChild);
      if (hr == S_OK)
      {
        WalkTreeWithAccessibleChildren(pChild, depth + 1);
        pChild->Release();
      }
      pDisp->Release();
    }
  }
  delete[] pArray;
  return S_OK;
}

DWORD WINAPI ThreadProc(LPVOID lpParameter)
{
  HRESULT result{};

  // Switch to a Windows 10 Metro app like the Calculator or Edge.
  std::this_thread::sleep_for(std::chrono::milliseconds(5000));

  auto hwnd = GetForegroundWindow();
  if (!hwnd) {
    abort();
  }

  result = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED);
  if (FAILED(result)) {
    abort();
  }

  IAccessible* pAcc = NULL;
  result = AccessibleObjectFromWindow(hwnd, OBJID_CLIENT, IID_IAccessible, (void**)&pAcc);
  if (result == S_OK) {
    WalkTreeWithAccessibleChildren(pAcc, 0);
    pAcc->Release();
  }

  CoUninitialize();

  return 0;
}

int APIENTRY wWinMain(_In_ HINSTANCE hInstance,
  _In_opt_ HINSTANCE hPrevInstance,
  _In_ LPTSTR    lpCmdLine,
  _In_ int       nCmdShow)
{
  HRESULT result{};
  DWORD dw{};

#if HACK == 1
  HWND hwnd = CreateWindowA("STATIC", nullptr, 0,
    CW_USEDEFAULT, CW_USEDEFAULT,
    CW_USEDEFAULT, CW_USEDEFAULT,
    0, 0, 0, nullptr);
  if (!hwnd) {
    abort();
  }
  ShowWindow(hwnd, SW_SHOWNORMAL);
#endif

  result = CoInitializeEx(nullptr,
#if HACK == 2
    COINIT_MULTITHREADED
#else
    COINIT_APARTMENTTHREADED
#endif
  );
  if (FAILED(result)) {
    abort();
  }

#if HACK == 3
  ThreadProc(nullptr);
#else
  HANDLE threadHandle = CreateThread(nullptr, 0, &ThreadProc, nullptr, 0, nullptr);
  if (!threadHandle) {
    auto error = GetLastError();
    abort();
  }

  dw = WaitForSingleObject(threadHandle, INFINITE);
  if (dw == WAIT_FAILED) {
    auto error = GetLastError();
    abort();
  }
#endif

#if HACK == 4
  std::this_thread::sleep_for(std::chrono::milliseconds(16000));
#endif

#if HACK == 5
  CoInitialize(nullptr);
#endif

  CoUninitialize();

  return 0;
}
bwiwov
  • 51
  • 6
  • Seeing that the `std::async` threads are already in the MTA, perhaps you could just use the MTA in the thread – M.M Feb 19 '16 at 01:31
  • Hmm, that can't be it. I tested having the worker use COINIT_MULTITHREADED, so CoInitializeEx() would not fail, that did not solve the problem. Also tested pumping with PeekMessage/DispatchMessage while waiting for the worker to finish, as required by STA, did not solve the problem. Just don't use a worker. – Hans Passant Feb 21 '16 at 09:01
  • @HansPassant The worker thread is a crucial part of the real-world code that this test-case is based on and I can't easily remove it. On the other hand it's not really feasible for me to create and show a dummy window as demonstrated in this answer either... At this point I'm seriously considering adding an unmatched CoInitialize just to bump up the reference count so that COM will never be uninitialized. – bwiwov Feb 21 '16 at 23:31
  • 1
    Perhaps the real problem is that there are still objects alive in the apartment when you call `CoUninitialize` (which causes undefined behaviour for an STA) – M.M Feb 22 '16 at 00:34
  • @M.M How could this happen? Doesn't the code in `WalkTreeWithAccessibleChildren` clean up all the COM objects that it creates? – bwiwov Feb 22 '16 at 00:52
  • Not sure, I'm suggesting that as something to try and investigate through debugging, perhaps there are objects not being freed that you are not aware of – M.M Feb 22 '16 at 01:42