diff --git a/accessible/ipc/win/HandlerProvider.cpp b/accessible/ipc/win/HandlerProvider.cpp index 7bf5e8f5ded0..6e8e5e52c38d 100644 --- a/accessible/ipc/win/HandlerProvider.cpp +++ b/accessible/ipc/win/HandlerProvider.cpp @@ -222,7 +222,8 @@ void HandlerProvider::BuildStaticIA2Data( } } -void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data) { +void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data, + bool aMarshaledByCom) { MOZ_ASSERT(aOutIA2Data); MOZ_ASSERT(NS_IsMainThread()); MOZ_ASSERT(IsTargetInterfaceCacheable()); @@ -242,13 +243,24 @@ void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data) { auto hasFailed = [&hr]() -> bool { return FAILED(hr); }; - auto cleanup = [aOutIA2Data]() -> void { - CleanupDynamicIA2Data(*aOutIA2Data); + auto cleanup = [aOutIA2Data, aMarshaledByCom]() -> void { + CleanupDynamicIA2Data(*aOutIA2Data, aMarshaledByCom); }; mscom::ExecuteWhen onFail(hasFailed, cleanup); + // When allocating memory to be returned to the client, you *must* use + // allocMem, not CoTaskMemAlloc! + auto allocMem = [aMarshaledByCom](size_t aSize) { + if (aMarshaledByCom) { + return ::CoTaskMemAlloc(aSize); + } + // We use midl_user_allocate rather than CoTaskMemAlloc because this + // struct is being marshaled by RPC, not COM. + return ::midl_user_allocate(aSize); + }; + const VARIANT kChildIdSelf = {VT_I4}; VARIANT varVal; @@ -363,10 +375,8 @@ void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data) { return; } if (aOutIA2Data->mNRowHeaderCells > 0) { - // We use midl_user_allocate rather than CoTaskMemAlloc because this - // struct is being marshaled by RPC, not COM. aOutIA2Data->mRowHeaderCellIds = static_cast( - ::midl_user_allocate(sizeof(long) * aOutIA2Data->mNRowHeaderCells)); + allocMem(sizeof(long) * aOutIA2Data->mNRowHeaderCells)); for (long i = 0; i < aOutIA2Data->mNRowHeaderCells; ++i) { RefPtr headerAcc; hr = headers[i]->QueryInterface(IID_IAccessible2, @@ -385,11 +395,8 @@ void HandlerProvider::BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data) { return; } if (aOutIA2Data->mNColumnHeaderCells > 0) { - // We use midl_user_allocate rather than CoTaskMemAlloc because this - // struct is being marshaled by RPC, not COM. - aOutIA2Data->mColumnHeaderCellIds = - static_cast(::midl_user_allocate( - sizeof(long) * aOutIA2Data->mNColumnHeaderCells)); + aOutIA2Data->mColumnHeaderCellIds = static_cast( + allocMem(sizeof(long) * aOutIA2Data->mNColumnHeaderCells)); for (long i = 0; i < aOutIA2Data->mNColumnHeaderCells; ++i) { RefPtr headerAcc; hr = headers[i]->QueryInterface(IID_IAccessible2, @@ -575,7 +582,8 @@ HandlerProvider::Refresh(DynamicIA2Data* aOutData) { if (!mscom::InvokeOnMainThread("HandlerProvider::BuildDynamicIA2Data", this, &HandlerProvider::BuildDynamicIA2Data, - std::forward(aOutData))) { + std::forward(aOutData), + /* aMarshaledByCom */ true)) { return E_FAIL; } diff --git a/accessible/ipc/win/HandlerProvider.h b/accessible/ipc/win/HandlerProvider.h index d98bcbc2e111..bfaa58667d33 100644 --- a/accessible/ipc/win/HandlerProvider.h +++ b/accessible/ipc/win/HandlerProvider.h @@ -75,7 +75,16 @@ class HandlerProvider final : public IGeckoBackChannel, NotNull aInterceptor); void BuildStaticIA2Data(NotNull aInterceptor, StaticIA2Data* aOutData); - void BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data); + /** + * Pass true for aMarshaledByCom if this struct is being directly marshaled as + * an out parameter of a COM method, currently only + * IGeckoBackChannel::Refresh. + * When aMarshaledByCom is false, this means the struct is being marshaled + * by RPC encoding functions. This means we must allocate memory differently, + * even though we're using this as part of a COM handler payload. + */ + void BuildDynamicIA2Data(DynamicIA2Data* aOutIA2Data, + bool aMarshaledByCom = false); void BuildInitialIA2Data(NotNull aInterceptor, StaticIA2Data* aOutStaticData, DynamicIA2Data* aOutDynamicData); diff --git a/accessible/ipc/win/handler/AccessibleHandler.cpp b/accessible/ipc/win/handler/AccessibleHandler.cpp index 7246d7982bbc..296bf3f2e032 100644 --- a/accessible/ipc/win/handler/AccessibleHandler.cpp +++ b/accessible/ipc/win/handler/AccessibleHandler.cpp @@ -82,6 +82,7 @@ AccessibleHandler::AccessibleHandler(IUnknown* aOuter, HRESULT* aResult) mIATableCellPassThru(nullptr), mIAHypertextPassThru(nullptr), mCachedData(), + mCachedDynamicDataMarshaledByCom(false), mCacheGen(0), mCachedHyperlinks(nullptr), mCachedNHyperlinks(-1), @@ -103,7 +104,8 @@ AccessibleHandler::AccessibleHandler(IUnknown* aOuter, HRESULT* aResult) } AccessibleHandler::~AccessibleHandler() { - CleanupDynamicIA2Data(mCachedData.mDynamicData); + CleanupDynamicIA2Data(mCachedData.mDynamicData, + mCachedDynamicDataMarshaledByCom); if (mCachedData.mGeckoBackChannel) { mCachedData.mGeckoBackChannel->Release(); } @@ -218,12 +220,16 @@ AccessibleHandler::MaybeUpdateCachedData() { return E_POINTER; } + // Clean up the old data. + CleanupDynamicIA2Data(mCachedData.mDynamicData, + mCachedDynamicDataMarshaledByCom); HRESULT hr = mCachedData.mGeckoBackChannel->Refresh(&mCachedData.mDynamicData); if (SUCCEEDED(hr)) { // We just updated the cache, so update this object's cache generation // so we only update the cache again after the next change. mCacheGen = gen; + mCachedDynamicDataMarshaledByCom = true; } return hr; } @@ -466,8 +472,10 @@ AccessibleHandler::ReadHandlerPayload(IStream* aStream, REFIID aIid) { return E_FAIL; } // Clean up the old data. - CleanupDynamicIA2Data(mCachedData.mDynamicData); + CleanupDynamicIA2Data(mCachedData.mDynamicData, + mCachedDynamicDataMarshaledByCom); mCachedData = newData; + mCachedDynamicDataMarshaledByCom = false; // These interfaces have been aggregated into the proxy manager. // The proxy manager will resolve these interfaces now on QI, diff --git a/accessible/ipc/win/handler/AccessibleHandler.h b/accessible/ipc/win/handler/AccessibleHandler.h index c3656b31a710..ac26cc6a891b 100644 --- a/accessible/ipc/win/handler/AccessibleHandler.h +++ b/accessible/ipc/win/handler/AccessibleHandler.h @@ -280,6 +280,7 @@ class AccessibleHandler final : public mscom::Handler, IAccessibleTableCell* mIATableCellPassThru; // weak IAccessibleHypertext2* mIAHypertextPassThru; // weak IA2Payload mCachedData; + bool mCachedDynamicDataMarshaledByCom; UniquePtr mSerializer; uint32_t mCacheGen; IAccessibleHyperlink** mCachedHyperlinks; diff --git a/accessible/ipc/win/handler/HandlerDataCleanup.h b/accessible/ipc/win/handler/HandlerDataCleanup.h index f7574be838e3..14d9995c1914 100644 --- a/accessible/ipc/win/handler/HandlerDataCleanup.h +++ b/accessible/ipc/win/handler/HandlerDataCleanup.h @@ -37,7 +37,22 @@ inline void ReleaseStaticIA2DataInterfaces(StaticIA2Data& aData) { } } -inline void CleanupDynamicIA2Data(DynamicIA2Data& aData) { +/** + * Pass true for aMarshaledByCom if this struct was directly marshaled as an + * out parameter of a COM method, currently only IGeckoBackChannel::Refresh. + */ +inline void CleanupDynamicIA2Data(DynamicIA2Data& aData, + bool aMarshaledByCom = false) { + // If freeing generic memory returned to the client, you *must* use freeMem, + // not CoTaskMemFree! + auto freeMem = [aMarshaledByCom](void* aMem) { + if (aMarshaledByCom) { + ::CoTaskMemFree(aMem); + } else { + ::midl_user_free(aMem); + } + }; + ::VariantClear(&aData.mRole); if (aData.mKeyboardShortcut) { ::SysFreeString(aData.mKeyboardShortcut); @@ -67,10 +82,10 @@ inline void CleanupDynamicIA2Data(DynamicIA2Data& aData) { ::SysFreeString(aData.mIA2Locale.variant); } if (aData.mRowHeaderCellIds) { - ::midl_user_free(aData.mRowHeaderCellIds); + freeMem(aData.mRowHeaderCellIds); } if (aData.mColumnHeaderCellIds) { - ::midl_user_free(aData.mColumnHeaderCellIds); + freeMem(aData.mColumnHeaderCellIds); } }