[PATCH v2 0/2] MR8555: windows.devices.enumeration: Reimplement DeviceInformationStatics::FindAllAsync and DeviceWatcher using the device query API.
Implement `DeviceWatcher` on top of a `HDEVQUERY` object, obtained from `DevCreateObjectQuery`. All events from the query object's associated callback are directly forwarded to the `DeviceWatcher`'s corresponding event handlers. As closing/freeing the `HDEVQUERY` is performed asynchronously, I have also added `IWeakReference` support for the device watcher. -- v2: windows.devices.enumeration: Implement DeviceInformationStatics::DeviceWatcher using DevCreateObjectQuery. https://gitlab.winehq.org/wine/wine/-/merge_requests/8555
From: Vibhav Pant <vibhavp(a)gmail.com> --- dlls/windows.devices.enumeration/Makefile.in | 2 +- dlls/windows.devices.enumeration/main.c | 27 ++++++++++++++------ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/dlls/windows.devices.enumeration/Makefile.in b/dlls/windows.devices.enumeration/Makefile.in index 5e3f6fbb6fa..853c1303efa 100644 --- a/dlls/windows.devices.enumeration/Makefile.in +++ b/dlls/windows.devices.enumeration/Makefile.in @@ -1,5 +1,5 @@ MODULE = windows.devices.enumeration.dll -IMPORTS = advapi32 combase setupapi uuid +IMPORTS = advapi32 cfgmgr32 combase setupapi uuid SOURCES = \ access.c \ diff --git a/dlls/windows.devices.enumeration/main.c b/dlls/windows.devices.enumeration/main.c index c927ccff18d..2cd727a4149 100644 --- a/dlls/windows.devices.enumeration/main.c +++ b/dlls/windows.devices.enumeration/main.c @@ -21,6 +21,8 @@ #include "initguid.h" #include "private.h" #include "setupapi.h" +#include "devfiltertypes.h" +#include "devquery.h" #include "wine/debug.h" @@ -519,12 +521,6 @@ static HRESULT WINAPI device_statics_CreateFromIdAsyncAdditionalProperties( IDev return E_NOTIMPL; } -static HRESULT append_device_information( IDeviceInformation *info, void *context ) -{ - IVector_IInspectable *vector = context; - return IVector_IInspectable_Append( vector, (IInspectable *)info ); -} - static HRESULT find_all_async( IUnknown *invoker, IUnknown *param, PROPVARIANT *result ) { static const struct vector_iids iids = @@ -536,13 +532,28 @@ static HRESULT find_all_async( IUnknown *invoker, IUnknown *param, PROPVARIANT * }; IVectorView_DeviceInformation *view; IVector_IInspectable *vector; + const DEV_OBJECT *objects; + ULONG len, i; HRESULT hr; TRACE( "invoker %p, param %p, result %p\n", invoker, param, result ); if (FAILED(hr = vector_create( &iids, (void *)&vector ))) return hr; - hr = enum_device_information( append_device_information, vector ); - + if (FAILED(hr = DevGetObjects( DevObjectTypeDeviceInterfaceDisplay, DevQueryFlagNone, 0, NULL, 0, NULL, &len, &objects ))) + { + IVector_IInspectable_Release( vector ); + return hr; + } + for (i = 0; i < len && SUCCEEDED(hr); i++) + { + IDeviceInformation *info; + if (SUCCEEDED(hr = device_information_create( objects[i].pszObjectId, &info ))) + { + hr = IVector_IInspectable_Append( vector, (IInspectable *)info ); + IDeviceInformation_Release( info ); + } + } + DevFreeObjects( len, objects ); if (SUCCEEDED(hr)) hr = IVector_IInspectable_GetView( vector, (void *)&view ); IVector_IInspectable_Release( vector ); if (FAILED(hr)) return hr; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8555
From: Vibhav Pant <vibhavp(a)gmail.com> --- dlls/windows.devices.enumeration/Makefile.in | 5 +- dlls/windows.devices.enumeration/main.c | 212 +++++++++--------- dlls/windows.devices.enumeration/private.h | 1 + .../tests/devices.c | 4 +- dlls/windows.devices.enumeration/weakref.c | 175 +++++++++++++++ dlls/windows.devices.enumeration/weakref.h | 38 ++++ 6 files changed, 330 insertions(+), 105 deletions(-) create mode 100644 dlls/windows.devices.enumeration/weakref.c create mode 100644 dlls/windows.devices.enumeration/weakref.h diff --git a/dlls/windows.devices.enumeration/Makefile.in b/dlls/windows.devices.enumeration/Makefile.in index 853c1303efa..0a204835ae1 100644 --- a/dlls/windows.devices.enumeration/Makefile.in +++ b/dlls/windows.devices.enumeration/Makefile.in @@ -1,5 +1,5 @@ MODULE = windows.devices.enumeration.dll -IMPORTS = advapi32 cfgmgr32 combase setupapi uuid +IMPORTS = cfgmgr32 combase uuid SOURCES = \ access.c \ @@ -9,4 +9,5 @@ SOURCES = \ event_handlers.c \ information.c \ main.c \ - vector.c + vector.c \ + weakref.c diff --git a/dlls/windows.devices.enumeration/main.c b/dlls/windows.devices.enumeration/main.c index 2cd727a4149..48059a4d475 100644 --- a/dlls/windows.devices.enumeration/main.c +++ b/dlls/windows.devices.enumeration/main.c @@ -2,6 +2,7 @@ * * Copyright 2021 Gijs Vermeulen * Copyright 2022 Julian Klemann for CodeWeavers + * Copyright 2025 Vibhav Pant * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -20,7 +21,7 @@ #include "initguid.h" #include "private.h" -#include "setupapi.h" +#include "devpropdef.h" #include "devfiltertypes.h" #include "devquery.h" @@ -28,62 +29,10 @@ WINE_DEFAULT_DEBUG_CHANNEL(enumeration); -typedef HRESULT (*enum_device_information_cb)( IDeviceInformation *info, void *context ); - -static HRESULT enum_device_information( enum_device_information_cb callback, void *context ) -{ - HKEY iface_key; - HRESULT hr; - DWORD i; - - if (!(iface_key = SetupDiOpenClassRegKeyExW( NULL, KEY_ENUMERATE_SUB_KEYS, DIOCR_INTERFACE, NULL, NULL ))) - return HRESULT_FROM_WIN32( GetLastError() ); - - for (i = 0, hr = S_OK; SUCCEEDED(hr); i++) - { - char buffer[sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W) + MAX_PATH * sizeof(WCHAR)]; - SP_DEVICE_INTERFACE_DETAIL_DATA_W *detail = (void *)buffer; - SP_DEVICE_INTERFACE_DATA iface = {.cbSize = sizeof(iface)}; - HDEVINFO set = INVALID_HANDLE_VALUE; - GUID iface_class; - WCHAR name[40]; - DWORD j, len; - LSTATUS ret; - - len = ARRAY_SIZE(name); - ret = RegEnumKeyExW( iface_key, i, name, &len, NULL, NULL, NULL, NULL ); - if (ret == ERROR_NO_MORE_ITEMS) break; - if (ret) hr = HRESULT_FROM_WIN32( ret ); - - if (SUCCEEDED(hr) && SUCCEEDED(hr = CLSIDFromString( name, &iface_class ))) - { - set = SetupDiGetClassDevsW( &iface_class, NULL, NULL, DIGCF_DEVICEINTERFACE ); - if (set == INVALID_HANDLE_VALUE) hr = HRESULT_FROM_WIN32( GetLastError() ); - } - - for (j = 0; SUCCEEDED(hr) && SetupDiEnumDeviceInterfaces( set, NULL, &iface_class, j, &iface ); j++) - { - IDeviceInformation *info; - - detail->cbSize = sizeof(SP_DEVICE_INTERFACE_DETAIL_DATA_W); - if (!SetupDiGetDeviceInterfaceDetailW( set, &iface, detail, sizeof(buffer), NULL, NULL )) continue; - - if (SUCCEEDED(hr = device_information_create( detail->DevicePath, &info ))) - { - hr = callback( info, context ); - IDeviceInformation_Release( info ); - } - } - } - - RegCloseKey( iface_key ); - return hr; -} - struct device_watcher { IDeviceWatcher IDeviceWatcher_iface; - LONG ref; + struct weak_reference_source weak_reference_source; struct list added_handlers; struct list enumerated_handlers; @@ -92,6 +41,7 @@ struct device_watcher CRITICAL_SECTION cs; DeviceWatcherStatus status; + HDEVQUERY query; }; static inline struct device_watcher *impl_from_IDeviceWatcher( IDeviceWatcher *iface ) @@ -114,6 +64,13 @@ static HRESULT WINAPI device_watcher_QueryInterface( IDeviceWatcher *iface, REFI return S_OK; } + if (IsEqualGUID( iid, &IID_IWeakReferenceSource )) + { + *out = &impl->weak_reference_source.IWeakReferenceSource_iface; + IWeakReferenceSource_AddRef(*out); + return S_OK; + } + FIXME( "%s not implemented, returning E_NOINTERFACE.\n", debugstr_guid( iid ) ); *out = NULL; return E_NOINTERFACE; @@ -122,7 +79,7 @@ static HRESULT WINAPI device_watcher_QueryInterface( IDeviceWatcher *iface, REFI static ULONG WINAPI device_watcher_AddRef( IDeviceWatcher *iface ) { struct device_watcher *impl = impl_from_IDeviceWatcher( iface ); - ULONG ref = InterlockedIncrement( &impl->ref ); + ULONG ref = weak_reference_strong_add_ref( &impl->weak_reference_source ); TRACE( "iface %p, ref %lu.\n", iface, ref ); return ref; } @@ -130,7 +87,7 @@ static ULONG WINAPI device_watcher_AddRef( IDeviceWatcher *iface ) static ULONG WINAPI device_watcher_Release( IDeviceWatcher *iface ) { struct device_watcher *impl = impl_from_IDeviceWatcher( iface ); - ULONG ref = InterlockedDecrement( &impl->ref ); + ULONG ref = weak_reference_strong_release( &impl->weak_reference_source ); TRACE( "iface %p, ref %lu.\n", iface, ref ); if (!ref) @@ -141,6 +98,7 @@ static ULONG WINAPI device_watcher_Release( IDeviceWatcher *iface ) WindowsDeleteString( impl->filter ); impl->cs.DebugInfo->Spare[0] = 0; DeleteCriticalSection( &impl->cs ); + if (impl->query) DevCloseObjectQuery( impl->query ); free( impl ); } @@ -248,37 +206,87 @@ static HRESULT WINAPI device_watcher_get_Status( IDeviceWatcher *iface, DeviceWa return S_OK; } -static HRESULT add_device_information( IDeviceInformation *info, void *invoker ) +static const char *debugstr_DEV_QUERY_RESULT_ACTION_DATA( const DEV_QUERY_RESULT_ACTION_DATA *data ) { - struct device_watcher *impl = impl_from_IDeviceWatcher( (IDeviceWatcher *)invoker ); - typed_event_handlers_notify( &impl->added_handlers, (IInspectable *)invoker, (IInspectable *)info ); - return S_OK; + const DEV_OBJECT *obj = &data->Data.DeviceObject; + if (!data) return wine_dbg_sprintf( "(null)" ); + if (data->Action == DevQueryResultStateChange) + return wine_dbg_sprintf( "{%d {%d}}", data->Action, data->Data.State ); + return wine_dbg_sprintf( "{%d {{%d %s %lu %p}}}", data->Action, obj->ObjectType, debugstr_w( obj->pszObjectId ), obj->cPropertyCount, obj->pProperties ); } -static HRESULT device_watcher_start_async( IUnknown *invoker, IUnknown *param, PROPVARIANT *result ) +static void WINAPI device_object_query_callback( HDEVQUERY query, void *data, + const DEV_QUERY_RESULT_ACTION_DATA *action_data ) { - struct device_watcher *impl = impl_from_IDeviceWatcher( (IDeviceWatcher *)invoker ); - DeviceWatcherStatus status; + struct device_watcher *watcher; + IWeakReference *weak = data; + IDeviceWatcher *iface; HRESULT hr; - hr = enum_device_information( add_device_information, invoker ); + TRACE( "query %p, data %p, action_data %s.\n", query, data, debugstr_DEV_QUERY_RESULT_ACTION_DATA( action_data ) ); - EnterCriticalSection( &impl->cs ); - if (FAILED(hr)) status = DeviceWatcherStatus_Aborted; - else if (impl->status == DeviceWatcherStatus_Stopping) status = DeviceWatcherStatus_Stopped; - else status = DeviceWatcherStatus_EnumerationCompleted; - impl->status = status; - LeaveCriticalSection( &impl->cs ); + if (FAILED(hr = IWeakReference_Resolve( weak, &IID_IDeviceWatcher, (IInspectable **)&iface )) || !iface) + { + if (action_data->Action == DevQueryResultStateChange && + (action_data->Data.State == DevQueryStateClosed || action_data->Data.State == DevQueryStateAborted)) + IWeakReference_Release( weak ); /* No more callbacks are expected, so we can release the weak ref. */ + return; + } + watcher = impl_from_IDeviceWatcher( iface ); - if (status == DeviceWatcherStatus_Stopped) typed_event_handlers_notify( &impl->stopped_handlers, (IInspectable *)invoker, NULL ); - if (status == DeviceWatcherStatus_EnumerationCompleted) typed_event_handlers_notify( &impl->enumerated_handlers, (IInspectable *)invoker, NULL ); - return S_OK; + switch (action_data->Action) + { + case DevQueryResultStateChange: + switch (action_data->Data.State) + { + case DevQueryStateClosed: + EnterCriticalSection( &watcher->cs ); + watcher->status = DeviceWatcherStatus_Stopped; + watcher->query = NULL; + LeaveCriticalSection( &watcher->cs ); + typed_event_handlers_notify( &watcher->stopped_handlers, (IInspectable *)iface, NULL ); + IWeakReference_Release( weak ); + break; + case DevQueryStateAborted: + EnterCriticalSection( &watcher->cs ); + watcher->status = DeviceWatcherStatus_Aborted; + DevCloseObjectQuery( watcher->query ); + watcher->query = NULL; + LeaveCriticalSection( &watcher->cs ); + IWeakReference_Release( weak ); + break; + case DevQueryStateEnumCompleted: + EnterCriticalSection( &watcher->cs ); + watcher->status = DeviceWatcherStatus_EnumerationCompleted; + LeaveCriticalSection( &watcher->cs ); + + typed_event_handlers_notify( &watcher->enumerated_handlers, (IInspectable *)iface, NULL ); + break; + default: + FIXME( "Unhandled DEV_QUERY_STATE value: %d\n", action_data->Data.State ); + break; + } + break; + case DevQueryResultAdd: + { + IDeviceInformation *info; + if (FAILED(hr = device_information_create( action_data->Data.DeviceObject.pszObjectId, &info ))) + break; + typed_event_handlers_notify( &watcher->added_handlers, (IInspectable *)iface, (IInspectable *)info ); + IDeviceInformation_Release( info ); + break; + } + default: + FIXME( "Unhandled DEV_QUERY_RESULT_ACTION value: %d\n", action_data->Action ); + break; + } + + IDeviceWatcher_Release( iface ); } static HRESULT WINAPI device_watcher_Start( IDeviceWatcher *iface ) { struct device_watcher *impl = impl_from_IDeviceWatcher( iface ); - IAsyncAction *async; HRESULT hr = S_OK; FIXME( "iface %p: semi-stub!\n", iface ); @@ -292,17 +300,31 @@ static HRESULT WINAPI device_watcher_Start( IDeviceWatcher *iface ) EnterCriticalSection( &impl->cs ); switch (impl->status) { - case DeviceWatcherStatus_EnumerationCompleted: hr = E_ILLEGAL_METHOD_CALL; break; - case DeviceWatcherStatus_Started: hr = E_ILLEGAL_METHOD_CALL; break; - case DeviceWatcherStatus_Stopping: hr = E_ILLEGAL_METHOD_CALL; break; - default: hr = E_ILLEGAL_METHOD_CALL; - case DeviceWatcherStatus_Aborted: case DeviceWatcherStatus_Created: case DeviceWatcherStatus_Stopped: + { + IWeakReferenceSource *weak_src; + IWeakReference *weak; + HRESULT hr; + + hr = IDeviceWatcher_QueryInterface( iface, &IID_IWeakReferenceSource, (void **)&weak_src ); + if (FAILED(hr)) break; + hr = IWeakReferenceSource_GetWeakReference( weak_src, &weak ); + IWeakReferenceSource_Release( weak_src ); + if (FAILED(hr)) break; + hr = DevCreateObjectQuery( DevObjectTypeDeviceInterfaceDisplay, DevQueryFlagAsyncClose, 0, NULL, 0, NULL, device_object_query_callback, weak, + &impl->query ); + if (FAILED(hr)) + { + IWeakReference_Release( weak ); + break; + } impl->status = DeviceWatcherStatus_Started; - hr = async_action_create( (IUnknown *)iface, device_watcher_start_async, &async ); - if (SUCCEEDED(hr)) IAsyncAction_Release( async ); + break; + } + default: + hr = E_ILLEGAL_METHOD_CALL; break; } LeaveCriticalSection( &impl->cs ); @@ -310,22 +332,9 @@ static HRESULT WINAPI device_watcher_Start( IDeviceWatcher *iface ) return hr; } -static HRESULT device_watcher_stop_async( IUnknown *invoker, IUnknown *param, PROPVARIANT *result ) -{ - struct device_watcher *impl = impl_from_IDeviceWatcher( (IDeviceWatcher *)invoker ); - - EnterCriticalSection( &impl->cs ); - impl->status = DeviceWatcherStatus_Stopped; - LeaveCriticalSection( &impl->cs ); - - typed_event_handlers_notify( &impl->stopped_handlers, (IInspectable *)invoker, NULL ); - return S_OK; -} - static HRESULT WINAPI device_watcher_Stop( IDeviceWatcher *iface ) { struct device_watcher *impl = impl_from_IDeviceWatcher( iface ); - IAsyncAction *async; HRESULT hr = S_OK; TRACE( "iface %p\n", iface ); @@ -338,15 +347,11 @@ static HRESULT WINAPI device_watcher_Stop( IDeviceWatcher *iface ) case DeviceWatcherStatus_Stopped: hr = E_ILLEGAL_METHOD_CALL; break; case DeviceWatcherStatus_Stopping: hr = E_ILLEGAL_METHOD_CALL; break; default: hr = E_ILLEGAL_METHOD_CALL; - case DeviceWatcherStatus_EnumerationCompleted: - impl->status = DeviceWatcherStatus_Stopping; - hr = async_action_create( (IUnknown *)iface, device_watcher_stop_async, &async ); - if (SUCCEEDED(hr)) IAsyncAction_Release( async ); - break; case DeviceWatcherStatus_Started: impl->status = DeviceWatcherStatus_Stopping; - /* an async start is in progress, let it handle stopped state change */ + if (impl->query) + DevCloseObjectQuery( impl->query ); break; } LeaveCriticalSection( &impl->cs ); @@ -386,10 +391,15 @@ static HRESULT device_watcher_create( HSTRING filter, IDeviceWatcher **out ) if (!(impl = calloc( 1, sizeof(*impl) ))) return E_OUTOFMEMORY; - impl->ref = 1; impl->IDeviceWatcher_iface.lpVtbl = &device_watcher_vtbl; + if (FAILED(hr = weak_reference_source_init( &impl->weak_reference_source, (IUnknown *)&impl->IDeviceWatcher_iface ))) + { + free( impl ); + return hr; + } if (FAILED(hr = WindowsDuplicateString( filter, &impl->filter ))) { + weak_reference_strong_release( &impl->weak_reference_source ); free( impl ); return hr; } diff --git a/dlls/windows.devices.enumeration/private.h b/dlls/windows.devices.enumeration/private.h index e0a8e46f8b5..ad15b7916ca 100644 --- a/dlls/windows.devices.enumeration/private.h +++ b/dlls/windows.devices.enumeration/private.h @@ -40,6 +40,7 @@ #include "wine/list.h" #include "async_private.h" +#include "weakref.h" struct vector_iids { diff --git a/dlls/windows.devices.enumeration/tests/devices.c b/dlls/windows.devices.enumeration/tests/devices.c index d9247bb6ced..e669d031d7b 100644 --- a/dlls/windows.devices.enumeration/tests/devices.c +++ b/dlls/windows.devices.enumeration/tests/devices.c @@ -401,7 +401,7 @@ static void test_DeviceInformation( void ) ok( status == DeviceWatcherStatus_Started, "got status %u\n", status ); ref = IDeviceWatcher_AddRef( device_watcher ); - todo_wine ok( ref == 2, "got ref %lu\n", ref ); + ok( ref == 2, "got ref %lu\n", ref ); hr = IDeviceWatcher_Stop( device_watcher ); ok( hr == S_OK, "got hr %#lx\n", hr ); ok( !WaitForSingleObject( stopped_handler.event, 1000 ), "wait for stopped_handler.event failed\n" ); @@ -448,7 +448,7 @@ static void test_DeviceInformation( void ) ok( status == DeviceWatcherStatus_Started, "got status %u\n", status ); ref = IDeviceWatcher_AddRef( device_watcher ); - todo_wine ok( ref == 2, "got ref %lu\n", ref ); + ok( ref == 2, "got ref %lu\n", ref ); hr = IDeviceWatcher_Stop( device_watcher ); ok( hr == S_OK, "got hr %#lx\n", hr ); ok( !WaitForSingleObject( stopped_handler.event, 1000 ), "wait for stopped_handler.event failed\n" ); diff --git a/dlls/windows.devices.enumeration/weakref.c b/dlls/windows.devices.enumeration/weakref.c new file mode 100644 index 00000000000..6a10390dd4c --- /dev/null +++ b/dlls/windows.devices.enumeration/weakref.c @@ -0,0 +1,175 @@ +/* WinRT weak reference helpers + * + * Copyright 2025 Zhiyi Zhang for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#define COBJMACROS +#include "weakref.h" + +/* The control block is contained in struct weak_reference so that we don't have to allocate a + * separate struct for it */ +struct weak_reference +{ + IWeakReference IWeakReference_iface; + IUnknown *object; + /* control block */ + LONG ref_strong; + LONG ref_weak; +}; + +static inline struct weak_reference *impl_from_IWeakReference( IWeakReference *iface ) +{ + return CONTAINING_RECORD(iface, struct weak_reference, IWeakReference_iface); +} + +static HRESULT WINAPI weak_reference_QueryInterface( IWeakReference *iface, REFIID iid, void **out ) +{ + struct weak_reference *impl = impl_from_IWeakReference( iface ); + + if (IsEqualGUID( iid, &IID_IUnknown ) || IsEqualGUID( iid, &IID_IWeakReference )) + { + *out = &impl->IWeakReference_iface; + IWeakReference_AddRef( *out ); + return S_OK; + } + + *out = NULL; + return E_NOINTERFACE; +} + +static ULONG WINAPI weak_reference_AddRef( IWeakReference *iface ) +{ + struct weak_reference *impl = impl_from_IWeakReference( iface ); + return InterlockedIncrement( &impl->ref_weak ); +} + +static ULONG WINAPI weak_reference_Release( IWeakReference *iface ) +{ + struct weak_reference *impl = impl_from_IWeakReference( iface ); + ULONG ref = InterlockedDecrement( &impl->ref_weak ); + + if (!ref) + free( impl ); + return ref; +} + +static HRESULT WINAPI weak_reference_Resolve( IWeakReference *iface, REFIID iid, IInspectable **out ) +{ + struct weak_reference *impl = impl_from_IWeakReference( iface ); + HRESULT hr; + LONG ref; + + *out = NULL; + do + { + if (!(ref = ReadNoFence( &impl->ref_strong ))) + return S_OK; + } while (ref != InterlockedCompareExchange( &impl->ref_strong, ref + 1, ref )); + + hr = IUnknown_QueryInterface( impl->object, iid, (void **)out ); + IUnknown_Release( impl->object ); + return hr; +} + +static const struct IWeakReferenceVtbl weak_reference_vtbl = +{ + weak_reference_QueryInterface, + weak_reference_AddRef, + weak_reference_Release, + weak_reference_Resolve, +}; + +static inline struct weak_reference_source *impl_from_IWeakReferenceSource( IWeakReferenceSource *iface ) +{ + return CONTAINING_RECORD(iface, struct weak_reference_source, IWeakReferenceSource_iface); +} + +static HRESULT WINAPI weak_reference_source_QueryInterface( IWeakReferenceSource *iface, REFIID iid, void **out ) +{ + struct weak_reference_source *weak_reference_source = impl_from_IWeakReferenceSource( iface ); + struct weak_reference *weak_reference = impl_from_IWeakReference( weak_reference_source->weak_reference ); + return IUnknown_QueryInterface( weak_reference->object, iid, out ); +} + +static ULONG WINAPI weak_reference_source_AddRef( IWeakReferenceSource *iface ) +{ + struct weak_reference_source *weak_reference_source = impl_from_IWeakReferenceSource( iface ); + struct weak_reference *weak_reference = impl_from_IWeakReference( weak_reference_source->weak_reference ); + return IUnknown_AddRef( weak_reference->object ); +} + +static ULONG WINAPI weak_reference_source_Release( IWeakReferenceSource *iface ) +{ + struct weak_reference_source *weak_reference_source = impl_from_IWeakReferenceSource( iface ); + struct weak_reference *weak_reference = impl_from_IWeakReference( weak_reference_source->weak_reference ); + return IUnknown_Release( weak_reference->object ); +} + +static HRESULT WINAPI weak_reference_source_GetWeakReference( IWeakReferenceSource *iface, + IWeakReference **weak_reference ) +{ + struct weak_reference_source *impl = impl_from_IWeakReferenceSource( iface ); + + *weak_reference = impl->weak_reference; + IWeakReference_AddRef( *weak_reference ); + return S_OK; +} + +static const struct IWeakReferenceSourceVtbl weak_reference_source_vtbl = +{ + weak_reference_source_QueryInterface, + weak_reference_source_AddRef, + weak_reference_source_Release, + weak_reference_source_GetWeakReference, +}; + +static HRESULT weak_reference_create( IUnknown *object, IWeakReference **out ) +{ + struct weak_reference *weak_reference; + + if (!(weak_reference = calloc( 1, sizeof(*weak_reference) ))) + return E_OUTOFMEMORY; + + weak_reference->IWeakReference_iface.lpVtbl = &weak_reference_vtbl; + weak_reference->ref_strong = 1; + weak_reference->ref_weak = 1; + weak_reference->object = object; + *out = &weak_reference->IWeakReference_iface; + return S_OK; +} + +HRESULT weak_reference_source_init( struct weak_reference_source *source, IUnknown *object ) +{ + source->IWeakReferenceSource_iface.lpVtbl = &weak_reference_source_vtbl; + return weak_reference_create( object, &source->weak_reference ); +} + +ULONG weak_reference_strong_add_ref( struct weak_reference_source *source ) +{ + struct weak_reference *impl = impl_from_IWeakReference( source->weak_reference ); + return InterlockedIncrement( &impl->ref_strong ); +} + +ULONG weak_reference_strong_release( struct weak_reference_source *source ) +{ + struct weak_reference *impl = impl_from_IWeakReference( source->weak_reference ); + ULONG ref = InterlockedDecrement( &impl->ref_strong ); + + if (!ref) + IWeakReference_Release( source->weak_reference ); + return ref; +} diff --git a/dlls/windows.devices.enumeration/weakref.h b/dlls/windows.devices.enumeration/weakref.h new file mode 100644 index 00000000000..7d4bc22b087 --- /dev/null +++ b/dlls/windows.devices.enumeration/weakref.h @@ -0,0 +1,38 @@ +/* WinRT weak reference helpers + * + * Copyright 2025 Zhiyi Zhang for CodeWeavers + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA + */ + +#ifndef __WINE_WEAKREF_H +#define __WINE_WEAKREF_H + +#include "weakreference.h" + +struct weak_reference_source +{ + IWeakReferenceSource IWeakReferenceSource_iface; + IWeakReference *weak_reference; +}; + +/* Initialize a IWeakReferenceSource with the object to manage */ +HRESULT weak_reference_source_init( struct weak_reference_source *source, IUnknown *object ); +/* Add a strong reference to the managed object */ +ULONG weak_reference_strong_add_ref( struct weak_reference_source *source ); +/* Release a strong reference to the managed object */ +ULONG weak_reference_strong_release( struct weak_reference_source *source ); + +#endif /* __WINE_WEAKREF_H */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8555
On Wed Jul 30 08:13:21 2025 +0000, Rémi Bernon wrote:
As closing/freeing the `HDEVQUERY` is performed asynchronously, I have also added `IWeakReference` support for the device watcher. If this is only meant to fix some tests I don't think it's worth the added complexity. We only care about matching Windows (weak) refcounting when necessary: if an application depends on it, or if it is required to break reference cycle. We need weakref support as the HDEVQUERY object is created with async close events. That allows us to not manually create a Stopped event in order to implement `Stop`. However, this also means that if the watcher gets freed without being stopped first, the HDEVQUERY gets closed in Release. As DevCloseObjectQuery asynchronously frees the query object, using weak references allows us to implement Release without first waiting for the DevQueryStateChange event before freeing the device watcher.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111578
Rémi Bernon (@rbernon) commented about dlls/windows.devices.enumeration/main.c:
EnterCriticalSection( &impl->cs ); switch (impl->status) { - case DeviceWatcherStatus_EnumerationCompleted: hr = E_ILLEGAL_METHOD_CALL; break; - case DeviceWatcherStatus_Started: hr = E_ILLEGAL_METHOD_CALL; break; - case DeviceWatcherStatus_Stopping: hr = E_ILLEGAL_METHOD_CALL; break; - default: hr = E_ILLEGAL_METHOD_CALL;
Same comment as the before wrt. default case for the state machine. Actually, I wouldn't mind if you added a separate commit to change the `default: hr = E_ILLEGAL_METHOD_CALL;` into `default: assert( 0 ); break;` here and there, the fallthrough looks wrong. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111630
On Wed Jul 30 22:33:14 2025 +0000, Vibhav Pant wrote:
We need weakref support as the HDEVQUERY object is created with async close events. That allows us to not manually create a Stopped event in order to implement `Stop`. However, this also means that if the watcher gets freed without being stopped first, the HDEVQUERY gets closed in Release. As DevCloseObjectQuery asynchronously frees the query object, using weak references allows us to implement Release without first waiting for the DevQueryStateChange event before freeing the device watcher. Lets introduce the weak reference interface in a separate change then, and let's add a test to check that the IWeakReferenceSource interface is supposed to be implemented on the device watcher.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111631
Rémi Bernon (@rbernon) commented about dlls/windows.devices.enumeration/main.c:
case DeviceWatcherStatus_Aborted: case DeviceWatcherStatus_Created: case DeviceWatcherStatus_Stopped: + { + IWeakReferenceSource *weak_src; + IWeakReference *weak; + HRESULT hr; + + hr = IDeviceWatcher_QueryInterface( iface, &IID_IWeakReferenceSource, (void **)&weak_src ); + if (FAILED(hr)) break; + hr = IWeakReferenceSource_GetWeakReference( weak_src, &weak ); + IWeakReferenceSource_Release( weak_src ); + if (FAILED(hr)) break; + hr = DevCreateObjectQuery( DevObjectTypeDeviceInterfaceDisplay, DevQueryFlagAsyncClose, 0, NULL, 0, NULL, device_object_query_callback, weak, + &impl->query ); + if (FAILED(hr))
There's no need to go through `QueryInterface`. Also `GetWeakReference` cannot fail. ```suggestion:-5+0 IWeakReferenceSource_GetWeakReference( &impl->weak_reference_source.IWeakReferenceSource_iface, &weak ); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111632
Rémi Bernon (@rbernon) commented about dlls/windows.devices.enumeration/main.c:
case DeviceWatcherStatus_Stopped: hr = E_ILLEGAL_METHOD_CALL; break; case DeviceWatcherStatus_Stopping: hr = E_ILLEGAL_METHOD_CALL; break; default: hr = E_ILLEGAL_METHOD_CALL; - case DeviceWatcherStatus_EnumerationCompleted: - impl->status = DeviceWatcherStatus_Stopping; - hr = async_action_create( (IUnknown *)iface, device_watcher_stop_async, &async ); - if (SUCCEEDED(hr)) IAsyncAction_Release( async ); - break; case DeviceWatcherStatus_Started: impl->status = DeviceWatcherStatus_Stopping; - /* an async start is in progress, let it handle stopped state change */ + if (impl->query) + DevCloseObjectQuery( impl->query );
The state machine should guarantee that `impl->query` is set here. What about enforcing `DevCloseObjectQuery called && query == NULL` as an invariant everywhere, like this instead: ```suggestion:-1+0 DevCloseObjectQuery( impl->query ); impl->query = NULL; ``` And remove the corresponding `watcher->query = NULL` above on `DevQueryStateClosed`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111633
On Thu Jul 31 10:41:23 2025 +0000, Rémi Bernon wrote:
Same comment as the before wrt. default case for the state machine. Actually, I wouldn't mind if you added a separate commit to change the `default: hr = E_ILLEGAL_METHOD_CALL;` into `default: assert( 0 ); break;` here and there, the fallthrough looks wrong. Sure.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111640
On Thu Jul 31 10:41:23 2025 +0000, Rémi Bernon wrote:
There's no need to go through `QueryInterface`. Also `GetWeakReference` cannot fail. ```suggestion:-5+0 IWeakReferenceSource_GetWeakReference( &impl->weak_reference_source.IWeakReferenceSource_iface, &weak ); ``` Ah, sure. I thought that we were to assume that even the trivial interfaces we implement should be treated as opaque as thus falliable.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111641
On Thu Jul 31 12:36:16 2025 +0000, Vibhav Pant wrote:
Sure. Would `DEFAULT_UNREACHABLE` be better?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111642
On Thu Jul 31 12:42:03 2025 +0000, Vibhav Pant wrote:
Would `DEFAULT_UNREACHABLE` be better? I don't know, it doesn't seem to guard against misuse, only hints the compiler to get rid of an eventual warning.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111643
On Thu Jul 31 12:54:34 2025 +0000, Rémi Bernon wrote:
I don't know, it doesn't seem to guard against misuse, only hints the compiler to get rid of an eventual warning. Also looks dangerous as it may just expand to `default:` Gotcha, thanks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8555#note_111655
participants (3)
-
Rémi Bernon -
Vibhav Pant
-
Vibhav Pant (＠vibhavp)