IE attachEvent on object tag causes memory corruption

Question

I've an ActiveX Control within an embedded IE7/8 HTML page that has the following event [id(1)] HRESULT MessageReceived([in] BSTR id, [in] BSTR json). On Windows the event is registered with OCX.attachEvent("MessageReceived", onMessageReceivedFunc).

Following code fires the event in the HTML page.

 HRESULT Fire_MessageReceived(BSTR id, BSTR json)
 {
  CComVariant varResult;
  T* pT = static_cast<T*>(this);
  int nConnectionIndex;
  CComVariant* pvars = new CComVariant[2];  
  int nConnections = m_vec.GetSize();
  for (nConnectionIndex = 0; nConnectionIndex < nConnections; nConnectionIndex++)
  {
   pT->Lock();
   CComPtr<IUnknown> sp = m_vec.GetAt(nConnectionIndex);
   pT->Unlock();
   IDispatch* pDispatch = reinterpret_cast<IDispatch*>(sp.p);
   if (pDispatch != NULL)
   {
    VariantClear(&varResult);

    pvars[1] = id;
    pvars[0] = json;

    DISPPARAMS disp = { pvars, NULL, 2, 0 };
    pDispatch->Invoke(0x1, IID_NULL, LOCALE_USER_DEFAULT, DISPATCH_METHOD, &disp, &varResult, NULL, NULL);
   }
  }
  delete[] pvars; // -> Memory Corruption here!
  return varResult.scode;
 }

After I enabled gflags.exe with application verifier, the following strange behaviour occur: After Invoke() that is executing the JavaScript callback, the BSTR from pvars[1] is copied to pvars[0] for some unknown reason!? The delete[] of pvars causes a double free of the same string then which ends in a heap corruption.

Does anybody has an idea whats going on here? Is this a IE bug or is there a trick within the OCX Implementation that I'm missing?

If I use the tag like:

<script for="OCX" event="MessageReceived(id, json)" language="JavaScript" type="text/javascript">
    window.onMessageReceivedFunc(windowId, json);
</script>

... the strange copy operation does not occur.

The following code also seem to be ok due to the fact that the caller of Fire_MessageReceived() is responsible for freeing the BSTRs.

HRESULT Fire_MessageReceived(BSTR srcWindowId, BSTR json)
 {
  CComVariant varResult;
  T* pT = static_cast<T*>(this);
  int nConnectionIndex;  
  VARIANT pvars[2];  
  int nConnections = m_vec.GetSize();
  for (nConnectionIndex = 0; nConnectionIndex < nConnections; nConnectionIndex++)
  {
   pT->Lock();
   CComPtr<IUnknown> sp = m_vec.GetAt(nConnectionIndex);
   pT->Unlock();
   IDispatch* pDispatch = reinterpret_cast<IDispatch*>(sp.p);
   if (pDispatch != NULL)
   {
    VariantClear(&varResult);

    pvars[1].vt = VT_BSTR;
    pvars[1].bstrVal = srcWindowId;
    pvars[0].vt = VT_BSTR;
    pvars[0].bstrVal = json;

    DISPPARAMS disp = { pvars, NULL, 2, 0 };
    pDispatch->Invoke(0x1, IID_NULL, LOCALE_USER_DEFAULT, DISPATCH_METHOD, &disp, &varResult, NULL, NULL);
   }
  }
  delete[] pvars;
  return varResult.scode;
 }

Thanks!

Answer 1

This is not an IE bug. There are a lot of things going on here that concern me, so I'll list them in the order I encountered them.

1. Why are you doing this: T* pT = static_cast<T*>(this);? You shouldn't ever have to do this. If Lock() and Unlock() are methods in your object, just call them.
2. Why are you calling Lock() and Unlock()? What do they do? All IE COM objects (which means your extension's COM objects) are STA. If they're single threaded, why are you doing locking?
3. You should change this: int nConnections = m_vec.GetSize(); to this: const int nConnections = m_vec.GetSize();, but this really doesn't have any bearing on your crash.
4. This is completely wrong: IDispatch* pDispatch = reinterpret_cast<IDispatch*>(sp.p);. Don't cast COM objects yourself. You need to call sp->QueryInterface(IID_IDispatch, (void**)&pDispatch); and check the HRESULT it returns for success. Then you don't have to check for NULL, since if it returns S_OK the out param is guaranteed to be non-NULL.
5. You don't have to call VariantClear() on a CComVariant; the whole point of CComVariant is that it does it for you. Even if you were using a standard VARIANT, you would call VariantInit() here (before you use it), not VariantClear() (which is for after you're done with it).
6. Don't use new and delete on the CComVariants. The whole point of CComVariant is that it will do memory management for you internally when it goes out of scope. The correct approach is to declare an array of CComVariants, similar to the way you declared a stack-based array of VARIANTs in your second code block. Then just get rid of the delete statement. I'm not sure why you're second example doesn't crash, since you're calling delete on a stack-allocated array. I suspect you're just getting lucky.
7. I don't think you should use CComVariant at all, since (a) you don't own the BSTRs, they're passed in, so presumably someone else is freeing them. CComVairant will SysFreeString() those bad-boys when it goes out of scope, and (b) DISPPARAMS doesn't take VARIANTs, it takes VARIANTARGs and they're not the same thing.
8. You should check the HRESULT that Invoke() returns. If it failed, that means your event didn't properly fire, so what you return in varResult.scode is uninitialized.
9. Also, since you're iterating over multiple connections, you're only returning the scode of the last one. If one fails, then the next one succeeds, what do you really want to return? You have to figure out how to handle that—I've glossed it over in my example below.

Here is how I would have done it:

HRESULT Fire_MessageReceived(BSTR srcWindowId, BSTR json) {
  CComVariant varResult;
  VARIANTARG vars[2];  
  const int nConnections = m_vec.GetSize();
  for (int i = 0; i < nConnections; ++i) {
    Lock();
    CComPtr<IUnknown> sp = m_vec.GetAt(nConnectionIndex);
    Unlock();

    IDispatch* pDispatch;
    HRESULT hr = sp->QueryInterface(IID_IDispatch, (void**)&pDispatch);
    if (SUCCEEDED(hr)) {
      pvars[1].vt = VT_BSTR;
      pvars[1].bstrVal = srcWindowId;
      pvars[0].vt = VT_BSTR;
      pvars[0].bstrVal = json;

      DISPPARAMS disp = { pvars, NULL, ARRAYSIZE(vars), 0 };
      hr = pDispatch->Invoke(0x1, IID_NULL, LOCALE_USER_DEFAULT, DISPATCH_METHOD, &disp, &varResult, NULL, NULL);
    }
  }

  return (SUCCEEDED(hr) ? varResult.scode : hr);
}

Answer 2

This sounds like a known IE bug. Add the FEATURE_LEGACY_DISPPARAMS feature control key and set its value to false.

HKEY_LOCAL_MACHINE\SOFTWARE\Wow6432Node\Microsoft\Internet Explorer\Main\FeatureControl\FEATURE_LEGACY_DISPPARAMS or HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Internet Explorer\Main\FeatureControl DWORD name: [exe name] DWORD value: 0 (disable legacy behavior to avoid crash)

Happens only when you pass more than one parameter and the parameters are types that need to be deleted (strings for example as opposed to numbers which aren't allocated).