[PATCH 1/2] ole32: Implement chained IInitializeSpy support.
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/ole32/compobj.c | 123 +++++++++++++++++++++++++++-------- dlls/ole32/compobj_private.h | 27 +++++++- dlls/ole32/tests/compobj.c | 15 ++--- 3 files changed, 126 insertions(+), 39 deletions(-) diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c index 66b1683d98..c65250442d 100644 --- a/dlls/ole32/compobj.c +++ b/dlls/ole32/compobj.c @@ -1733,6 +1733,19 @@ HWND apartment_getwindow(const struct apartment *apt) return apt->win; } +static void oletls_release_spy_list(struct list *list) +{ + struct init_spy *cursor, *cursor2; + + LIST_FOR_EACH_ENTRY_SAFE(cursor, cursor2, list, struct init_spy, entry) + { + list_remove(&cursor->entry); + if (cursor->spy) + IInitializeSpy_Release(cursor->spy); + heap_free(cursor); + } +} + static void COM_TlsDestroy(void) { struct oletls *info = NtCurrentTeb()->ReservedForOle; @@ -1741,7 +1754,8 @@ static void COM_TlsDestroy(void) if (info->apt) apartment_release(info->apt); if (info->errorinfo) IErrorInfo_Release(info->errorinfo); if (info->state) IUnknown_Release(info->state); - if (info->spy) IInitializeSpy_Release(info->spy); + oletls_release_spy_list(&info->spies.active); + oletls_release_spy_list(&info->spies.freed); if (info->context_token) IObjContext_Release(info->context_token); HeapFree(GetProcessHeap(), 0, info); NtCurrentTeb()->ReservedForOle = NULL; @@ -1783,6 +1797,7 @@ DWORD WINAPI CoBuildVersion(void) HRESULT WINAPI CoRegisterInitializeSpy(IInitializeSpy *spy, ULARGE_INTEGER *cookie) { struct oletls *info = COM_CurrentInfo(); + struct init_spy *entry; HRESULT hr; TRACE("(%p, %p)\n", spy, cookie); @@ -1794,19 +1809,33 @@ HRESULT WINAPI CoRegisterInitializeSpy(IInitializeSpy *spy, ULARGE_INTEGER *cook return E_INVALIDARG; } - if (info->spy) + hr = IInitializeSpy_QueryInterface(spy, &IID_IInitializeSpy, (void **)&spy); + if (FAILED(hr)) + return hr; + + if (list_empty(&info->spies.freed)) { - FIXME("Already registered?\n"); - return E_UNEXPECTED; + entry = heap_alloc(sizeof(*entry)); + if (!entry) + { + IInitializeSpy_Release(spy); + return E_OUTOFMEMORY; + } + entry->id = info->spies.next_id++; } - - hr = IInitializeSpy_QueryInterface(spy, &IID_IInitializeSpy, (void **) &info->spy); - if (SUCCEEDED(hr)) + else { - cookie->QuadPart = (DWORD_PTR)spy; - return S_OK; + entry = LIST_ENTRY(list_tail(&info->spies.freed), struct init_spy, entry); + list_remove(&entry->entry); } - return hr; + + entry->spy = spy; + list_add_head(&info->spies.active, &entry->entry); + + cookie->HighPart = GetCurrentThreadId(); + cookie->LowPart = entry->id; + + return S_OK; } /****************************************************************************** @@ -1827,14 +1856,42 @@ HRESULT WINAPI CoRegisterInitializeSpy(IInitializeSpy *spy, ULARGE_INTEGER *cook HRESULT WINAPI CoRevokeInitializeSpy(ULARGE_INTEGER cookie) { struct oletls *info = COM_CurrentInfo(); + struct init_spy *spy; + TRACE("(%s)\n", wine_dbgstr_longlong(cookie.QuadPart)); - if (!info || !info->spy || cookie.QuadPart != (DWORD_PTR)info->spy) + if (!info || list_empty(&info->spies.active)) return E_INVALIDARG; - IInitializeSpy_Release(info->spy); - info->spy = NULL; - return S_OK; + if (cookie.HighPart != GetCurrentThreadId() || cookie.LowPart >= info->spies.next_id) + return E_INVALIDARG; + + LIST_FOR_EACH_ENTRY(spy, &info->spies.active, struct init_spy, entry) + { + if (cookie.LowPart == spy->id) + { + IInitializeSpy_Release(spy->spy); + spy->spy = NULL; + list_remove(&spy->entry); + + /* Move to freed list, unless it was latest item id or last item in the active list. */ + if (spy->id == info->spies.next_id - 1) + info->spies.next_id--; + else if (list_empty(&info->spies.active)) + oletls_release_spy_list(&info->spies.freed); + else + { + list_add_tail(&info->spies.freed, &spy->entry); + spy = NULL; + } + + heap_free(spy); + + return S_OK; + } + } + + return E_INVALIDARG; } HRESULT enter_apartment( struct oletls *info, DWORD model ) @@ -1929,6 +1986,7 @@ HRESULT WINAPI CoInitialize(LPVOID lpReserved) HRESULT WINAPI DECLSPEC_HOTPATCH CoInitializeEx(LPVOID lpReserved, DWORD dwCoInit) { struct oletls *info = COM_CurrentInfo(); + struct init_spy *cursor; HRESULT hr; TRACE("(%p, %x)\n", lpReserved, (int)dwCoInit); @@ -1955,13 +2013,17 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoInitializeEx(LPVOID lpReserved, DWORD dwCoIni RunningObjectTableImpl_Initialize(); } - if (info->spy) - IInitializeSpy_PreInitialize(info->spy, dwCoInit, info->inits); + LIST_FOR_EACH_ENTRY(cursor, &info->spies.active, struct init_spy, entry) + { + IInitializeSpy_PreInitialize(cursor->spy, dwCoInit, info->inits); + } hr = enter_apartment( info, dwCoInit ); - if (info->spy) - IInitializeSpy_PostInitialize(info->spy, hr, dwCoInit, info->inits); + LIST_FOR_EACH_ENTRY(cursor, &info->spies.active, struct init_spy, entry) + { + hr = IInitializeSpy_PostInitialize(cursor->spy, hr, dwCoInit, info->inits); + } return hr; } @@ -1985,6 +2047,7 @@ HRESULT WINAPI DECLSPEC_HOTPATCH CoInitializeEx(LPVOID lpReserved, DWORD dwCoIni void WINAPI DECLSPEC_HOTPATCH CoUninitialize(void) { struct oletls * info = COM_CurrentInfo(); + struct init_spy *cursor; LONG lCOMRefCnt; TRACE("()\n"); @@ -1992,17 +2055,22 @@ void WINAPI DECLSPEC_HOTPATCH CoUninitialize(void) /* will only happen on OOM */ if (!info) return; - if (info->spy) - IInitializeSpy_PreUninitialize(info->spy, info->inits); + LIST_FOR_EACH_ENTRY(cursor, &info->spies.active, struct init_spy, entry) + { + IInitializeSpy_PreUninitialize(cursor->spy, info->inits); + } /* sanity check */ if (!info->inits) { - ERR("Mismatched CoUninitialize\n"); + ERR("Mismatched CoUninitialize\n"); + + LIST_FOR_EACH_ENTRY(cursor, &info->spies.active, struct init_spy, entry) + { + IInitializeSpy_PostUninitialize(cursor->spy, info->inits); + } - if (info->spy) - IInitializeSpy_PostUninitialize(info->spy, info->inits); - return; + return; } leave_apartment( info ); @@ -2024,8 +2092,11 @@ void WINAPI DECLSPEC_HOTPATCH CoUninitialize(void) ERR( "CoUninitialize() - not CoInitialized.\n" ); InterlockedExchangeAdd(&s_COMLockCount,1); /* restore the lock count. */ } - if (info->spy) - IInitializeSpy_PostUninitialize(info->spy, info->inits); + + LIST_FOR_EACH_ENTRY(cursor, &info->spies.active, struct init_spy, entry) + { + IInitializeSpy_PostUninitialize(cursor->spy, info->inits); + } } /****************************************************************************** diff --git a/dlls/ole32/compobj_private.h b/dlls/ole32/compobj_private.h index 1e564a3de1..1829866bff 100644 --- a/dlls/ole32/compobj_private.h +++ b/dlls/ole32/compobj_private.h @@ -153,6 +153,20 @@ struct apartment BOOL main; /* is this a main-threaded-apartment? (RO) */ }; +struct init_spy +{ + struct list entry; + IInitializeSpy *spy; + unsigned int id; +}; + +struct spy_chain +{ + struct list active; + struct list freed; + unsigned int next_id; +}; + /* this is what is stored in TEB->ReservedForOle */ struct oletls { @@ -160,7 +174,7 @@ struct oletls IErrorInfo *errorinfo; /* see errorinfo.c */ IUnknown *state; /* see CoSetState */ DWORD apt_mask; /* apartment mask (+0Ch on x86) */ - IInitializeSpy *spy; /* The "SPY" from CoInitializeSpy */ + void *unknown0; /* The "SPY" from CoInitializeSpy */ DWORD inits; /* number of times CoInitializeEx called */ DWORD ole_inits; /* number of times OleInitialize called */ GUID causality_id; /* unique identifier for each COM call */ @@ -171,6 +185,7 @@ struct oletls IUnknown *call_state; /* current call context (+3Ch on x86) */ DWORD unknown2[46]; IUnknown *cancel_object; /* cancel object set by CoSetCancelObject (+F8h on x86) */ + struct spy_chain spies; /* Spies from CoRegisterInitializeSpy */ }; @@ -262,7 +277,15 @@ APARTMENT *apartment_get_current_or_mta(void) DECLSPEC_HIDDEN; static inline struct oletls *COM_CurrentInfo(void) { if (!NtCurrentTeb()->ReservedForOle) - NtCurrentTeb()->ReservedForOle = heap_alloc_zero(sizeof(struct oletls)); + { + struct oletls *oletls = heap_alloc_zero(sizeof(*oletls)); + if (oletls) + { + list_init(&oletls->spies.active); + list_init(&oletls->spies.freed); + } + NtCurrentTeb()->ReservedForOle = oletls; + } return NtCurrentTeb()->ReservedForOle; } diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index 33a7585365..ec98af0e02 100644 --- a/dlls/ole32/tests/compobj.c +++ b/dlls/ole32/tests/compobj.c @@ -3363,32 +3363,28 @@ static void test_IInitializeSpy(void) cookie.LowPart = 1; hr = CoRegisterInitializeSpy(&testinitialize, &cookie); ok(hr == S_OK, "got 0x%08x\n", hr); -todo_wine { ok(cookie.HighPart == GetCurrentThreadId(), "got high part 0x%08x, expected 0x%08x\n", cookie.HighPart, GetCurrentThreadId()); ok(cookie.LowPart == 0, "got wrong low part 0x%x\n", cookie.LowPart); -} + /* register same instance one more time */ cookie1.HighPart = 0; cookie1.LowPart = 0; hr = CoRegisterInitializeSpy(&testinitialize, &cookie1); -todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); ok(cookie1.HighPart == GetCurrentThreadId(), "got high part 0x%08x, expected 0x%08x\n", cookie1.HighPart, GetCurrentThreadId()); ok(cookie1.LowPart == 1, "got wrong low part 0x%x\n", cookie1.LowPart); -} + cookie2.HighPart = 0; cookie2.LowPart = 0; hr = CoRegisterInitializeSpy(&testinitialize, &cookie2); -todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); ok(cookie2.HighPart == GetCurrentThreadId(), "got high part 0x%08x, expected 0x%08x\n", cookie2.HighPart, GetCurrentThreadId()); ok(cookie2.LowPart == 2, "got wrong low part 0x%x\n", cookie2.LowPart); -} + hr = CoRevokeInitializeSpy(cookie1); -todo_wine ok(hr == S_OK, "got 0x%08x\n", hr); hr = CoRevokeInitializeSpy(cookie1); @@ -3397,21 +3393,18 @@ todo_wine cookie1.HighPart = 0; cookie1.LowPart = 0; hr = CoRegisterInitializeSpy(&testinitialize, &cookie1); -todo_wine { ok(hr == S_OK, "got 0x%08x\n", hr); ok(cookie1.HighPart == GetCurrentThreadId(), "got high part 0x%08x, expected 0x%08x\n", cookie1.HighPart, GetCurrentThreadId()); ok(cookie1.LowPart == 1, "got wrong low part 0x%x\n", cookie1.LowPart); -} + hr = CoRevokeInitializeSpy(cookie); ok(hr == S_OK, "got 0x%08x\n", hr); hr = CoRevokeInitializeSpy(cookie1); -todo_wine ok(hr == S_OK, "got 0x%08x\n", hr); hr = CoRevokeInitializeSpy(cookie2); -todo_wine ok(hr == S_OK, "got 0x%08x\n", hr); } -- 2.20.1
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/ole32/tests/compobj.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/dlls/ole32/tests/compobj.c b/dlls/ole32/tests/compobj.c index ec98af0e02..11c2e95945 100644 --- a/dlls/ole32/tests/compobj.c +++ b/dlls/ole32/tests/compobj.c @@ -3308,14 +3308,14 @@ static ULONG WINAPI testinitialize_Release(IInitializeSpy *iface) static HRESULT WINAPI testinitialize_PreInitialize(IInitializeSpy *iface, DWORD coinit, DWORD aptrefs) { - ok(0, "unexpected call\n"); - return E_NOTIMPL; + ok(coinit == (COINIT_APARTMENTTHREADED | COINIT_DISABLE_OLE1DDE), "Unexpected init flags %#x.\n", coinit); + return S_OK; } static HRESULT WINAPI testinitialize_PostInitialize(IInitializeSpy *iface, HRESULT hr, DWORD coinit, DWORD aptrefs) { - ok(0, "unexpected call\n"); - return E_NOTIMPL; + ok(coinit == (COINIT_APARTMENTTHREADED | COINIT_DISABLE_OLE1DDE), "Unexpected init flags %#x.\n", coinit); + return hr; } static HRESULT WINAPI testinitialize_PreUninitialize(IInitializeSpy *iface, DWORD aptrefs) @@ -3398,6 +3398,9 @@ static void test_IInitializeSpy(void) GetCurrentThreadId()); ok(cookie1.LowPart == 1, "got wrong low part 0x%x\n", cookie1.LowPart); + hr = CoInitializeEx(NULL, COINIT_APARTMENTTHREADED | COINIT_DISABLE_OLE1DDE); + ok(hr == S_OK, "Failed to initialize COM, hr %#x.\n", hr); + hr = CoRevokeInitializeSpy(cookie); ok(hr == S_OK, "got 0x%08x\n", hr); @@ -3406,6 +3409,8 @@ static void test_IInitializeSpy(void) hr = CoRevokeInitializeSpy(cookie2); ok(hr == S_OK, "got 0x%08x\n", hr); + + CoUninitialize(); } static HRESULT g_persistfile_qi_ret; -- 2.20.1
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=46491 Your paranoid android. === debian9 (64 bit WoW report) === ole32: clipboard.c:1278: Test failed: 1 1 clipboard.c:1282: Test failed: 1
On Wed, Jan 23, 2019 at 11:41:44AM +0300, Nikolay Sivov wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/ole32/compobj.c | 123 +++++++++++++++++++++++++++-------- dlls/ole32/compobj_private.h | 27 +++++++- dlls/ole32/tests/compobj.c | 15 ++--- 3 files changed, 126 insertions(+), 39 deletions(-)
diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c index 66b1683d98..c65250442d 100644 --- a/dlls/ole32/compobj.c +++ b/dlls/ole32/compobj.c @@ -1827,14 +1856,42 @@ HRESULT WINAPI CoRegisterInitializeSpy(IInitializeSpy *spy, ULARGE_INTEGER *cook HRESULT WINAPI CoRevokeInitializeSpy(ULARGE_INTEGER cookie) { struct oletls *info = COM_CurrentInfo(); + struct init_spy *spy; + TRACE("(%s)\n", wine_dbgstr_longlong(cookie.QuadPart));
- if (!info || !info->spy || cookie.QuadPart != (DWORD_PTR)info->spy) + if (!info || list_empty(&info->spies.active)) return E_INVALIDARG;
- IInitializeSpy_Release(info->spy); - info->spy = NULL; - return S_OK; + if (cookie.HighPart != GetCurrentThreadId() || cookie.LowPart >= info->spies.next_id) + return E_INVALIDARG; + + LIST_FOR_EACH_ENTRY(spy, &info->spies.active, struct init_spy, entry) + { + if (cookie.LowPart == spy->id) + { + IInitializeSpy_Release(spy->spy); + spy->spy = NULL; + list_remove(&spy->entry); + + /* Move to freed list, unless it was latest item id or last item in the active list. */ + if (spy->id == info->spies.next_id - 1) + info->spies.next_id--; + else if (list_empty(&info->spies.active)) + oletls_release_spy_list(&info->spies.freed); + else + { + list_add_tail(&info->spies.freed, &spy->entry); + spy = NULL; + } + + heap_free(spy); + + return S_OK; + } + } + + return E_INVALIDARG; }
I don't think the extra complication of a free list is worth it. How about just keeping the active list sorted, when you register a new spy just iterate through until you find a missing id and insert the new node at that point? This stuff is not going to be performance critical.
diff --git a/dlls/ole32/compobj_private.h b/dlls/ole32/compobj_private.h index 1e564a3de1..1829866bff 100644 --- a/dlls/ole32/compobj_private.h +++ b/dlls/ole32/compobj_private.h struct oletls { @@ -160,7 +174,7 @@ struct oletls IErrorInfo *errorinfo; /* see errorinfo.c */ IUnknown *state; /* see CoSetState */ DWORD apt_mask; /* apartment mask (+0Ch on x86) */ - IInitializeSpy *spy; /* The "SPY" from CoInitializeSpy */ + void *unknown0; /* The "SPY" from CoInitializeSpy */
Could you remove the comment?
DWORD inits; /* number of times CoInitializeEx called */ DWORD ole_inits; /* number of times OleInitialize called */ GUID causality_id; /* unique identifier for each COM call */ @@ -171,6 +185,7 @@ struct oletls IUnknown *call_state; /* current call context (+3Ch on x86) */ DWORD unknown2[46]; IUnknown *cancel_object; /* cancel object set by CoSetCancelObject (+F8h on x86) */ + struct spy_chain spies; /* Spies from CoRegisterInitializeSpy */ };
FWIW, it's not clear whether anything needs compatiblity with native's layout, some things appear to have changed anyway. For example, 'inits' is at 0x18 in 32-bit Windows 7; we have it at 0x14. Huw.
On 1/24/19 3:00 PM, Huw Davies wrote:
On Wed, Jan 23, 2019 at 11:41:44AM +0300, Nikolay Sivov wrote:
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- dlls/ole32/compobj.c | 123 +++++++++++++++++++++++++++-------- dlls/ole32/compobj_private.h | 27 +++++++- dlls/ole32/tests/compobj.c | 15 ++--- 3 files changed, 126 insertions(+), 39 deletions(-)
diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c index 66b1683d98..c65250442d 100644 --- a/dlls/ole32/compobj.c +++ b/dlls/ole32/compobj.c @@ -1827,14 +1856,42 @@ HRESULT WINAPI CoRegisterInitializeSpy(IInitializeSpy *spy, ULARGE_INTEGER *cook HRESULT WINAPI CoRevokeInitializeSpy(ULARGE_INTEGER cookie) { struct oletls *info = COM_CurrentInfo(); + struct init_spy *spy; + TRACE("(%s)\n", wine_dbgstr_longlong(cookie.QuadPart));
- if (!info || !info->spy || cookie.QuadPart != (DWORD_PTR)info->spy) + if (!info || list_empty(&info->spies.active)) return E_INVALIDARG;
- IInitializeSpy_Release(info->spy); - info->spy = NULL; - return S_OK; + if (cookie.HighPart != GetCurrentThreadId() || cookie.LowPart >= info->spies.next_id) + return E_INVALIDARG; + + LIST_FOR_EACH_ENTRY(spy, &info->spies.active, struct init_spy, entry) + { + if (cookie.LowPart == spy->id) + { + IInitializeSpy_Release(spy->spy); + spy->spy = NULL; + list_remove(&spy->entry); + + /* Move to freed list, unless it was latest item id or last item in the active list. */ + if (spy->id == info->spies.next_id - 1) + info->spies.next_id--; + else if (list_empty(&info->spies.active)) + oletls_release_spy_list(&info->spies.freed); + else + { + list_add_tail(&info->spies.freed, &spy->entry); + spy = NULL; + } + + heap_free(spy); + + return S_OK; + } + } + + return E_INVALIDARG; } I don't think the extra complication of a free list is worth it. How about just keeping the active list sorted, when you register a new spy just iterate through until you find a missing id and insert the new node at that point? This stuff is not going to be performance critical.
Sure, will do.
diff --git a/dlls/ole32/compobj_private.h b/dlls/ole32/compobj_private.h index 1e564a3de1..1829866bff 100644 --- a/dlls/ole32/compobj_private.h +++ b/dlls/ole32/compobj_private.h struct oletls { @@ -160,7 +174,7 @@ struct oletls IErrorInfo *errorinfo; /* see errorinfo.c */ IUnknown *state; /* see CoSetState */ DWORD apt_mask; /* apartment mask (+0Ch on x86) */ - IInitializeSpy *spy; /* The "SPY" from CoInitializeSpy */ + void *unknown0; /* The "SPY" from CoInitializeSpy */ Could you remove the comment?
I will.
DWORD inits; /* number of times CoInitializeEx called */ DWORD ole_inits; /* number of times OleInitialize called */ GUID causality_id; /* unique identifier for each COM call */ @@ -171,6 +185,7 @@ struct oletls IUnknown *call_state; /* current call context (+3Ch on x86) */ DWORD unknown2[46]; IUnknown *cancel_object; /* cancel object set by CoSetCancelObject (+F8h on x86) */ + struct spy_chain spies; /* Spies from CoRegisterInitializeSpy */ };
FWIW, it's not clear whether anything needs compatiblity with native's layout, some things appear to have changed anyway. For example, 'inits' is at 0x18 in 32-bit Windows 7; we have it at 0x14.
Probably. One use case that I'm aware of is dotnet/coreclr, they are using at least one field directly there, that's why I didn't want to touch existing layout [1]. But admittedly I don't know if this code path is active in practice, or if this code is used in .NET Core/Mono at all. [1] SetupOleContext() in https://github.com/dotnet/coreclr
Huw.
participants (3)
-
Huw Davies -
Marvin -
Nikolay Sivov