[PATCH v3 5/5] comsvcs: Implement ISharedPropertyGroup_CreateProperty().

[Jactry Zeng]

Signed-off-by: Jactry Zeng <jzeng(a)codeweavers.com>

---
v3:
- Fix some leaks.
- Add more tests.
- Check SysAllocString() result in CreateProperty().
v2: No changes.
---
 dlls/comsvcs/comsvcs_private.h |   2 +
 dlls/comsvcs/property.c        | 250 ++++++++++++++++++++++++++++++++-
 dlls/comsvcs/tests/property.c  |  81 +++++++++++
 3 files changed, 331 insertions(+), 2 deletions(-)

diff --git a/dlls/comsvcs/comsvcs_private.h b/dlls/comsvcs/comsvcs_private.h
index eee6e9938c..3eeb9d751a 100644
--- a/dlls/comsvcs/comsvcs_private.h
+++ b/dlls/comsvcs/comsvcs_private.h
@@ -38,6 +38,7 @@ enum tid_t
     NULL_tid,
     ISharedPropertyGroupManager_tid,
     ISharedPropertyGroup_tid,
+    ISharedProperty_tid,
     LAST_tid
 };
 
@@ -46,6 +47,7 @@ static REFIID tid_ids[] =
     &IID_NULL,
     &IID_ISharedPropertyGroupManager,
     &IID_ISharedPropertyGroup,
+    &IID_ISharedProperty,
 };
 
 HRESULT get_typeinfo(enum tid_t tid, ITypeInfo **typeinfo);
diff --git a/dlls/comsvcs/property.c b/dlls/comsvcs/property.c
index a73964d288..b2b3920a2d 100644
--- a/dlls/comsvcs/property.c
+++ b/dlls/comsvcs/property.c
@@ -21,6 +21,13 @@
 
 WINE_DEFAULT_DEBUG_CHANNEL(comsvcs);
 
+struct shared_property
+{
+    ISharedProperty ISharedProperty_iface;
+    LONG refcount;
+    BSTR name;
+};
+
 struct group_manager
 {
     ISharedPropertyGroupManager ISharedPropertyGroupManager_iface;
@@ -37,10 +44,44 @@ struct property_group
     LONG isolation, release;
     struct list entry;
     BSTR name;
+    struct shared_property *properties;
+    size_t capacity, count;
+    CRITICAL_SECTION cs;
 };
 
 static struct group_manager *group_manager = NULL;
 
+static BOOL comsvcs_array_reserve(void **elements, size_t *capacity, size_t count, size_t size)
+{
+    size_t new_capacity, max_capacity;
+    void *new_elements;
+
+    if (count <= *capacity)
+        return TRUE;
+
+    max_capacity = ~(size_t)0 / size;
+    if (count > max_capacity)
+        return FALSE;
+
+    new_capacity = max(4, *capacity);
+    while (new_capacity < count && new_capacity <= max_capacity / 2)
+        new_capacity *= 2;
+    if (new_capacity < count)
+        new_capacity = max_capacity;
+
+    if (!*elements)
+        new_elements = heap_alloc_zero(new_capacity * size);
+    else
+        new_elements = heap_realloc(*elements, new_capacity * size);
+    if (!new_elements)
+        return FALSE;
+
+    *elements = new_elements;
+    *capacity = new_capacity;
+
+    return TRUE;
+}
+
 static inline struct group_manager *impl_from_ISharedPropertyGroupManager(ISharedPropertyGroupManager *iface)
 {
     return CONTAINING_RECORD(iface, struct group_manager, ISharedPropertyGroupManager_iface);
@@ -51,6 +92,139 @@ static inline struct property_group *impl_from_ISharedPropertyGroup(ISharedPrope
     return CONTAINING_RECORD(iface, struct property_group, ISharedPropertyGroup_iface);
 }
 
+static inline struct shared_property *impl_from_ISharedProperty(ISharedProperty *iface)
+{
+    return CONTAINING_RECORD(iface, struct shared_property, ISharedProperty_iface);
+}
+
+static HRESULT WINAPI shared_property_QueryInterface(ISharedProperty *iface, REFIID riid, void **out)
+{
+    struct shared_property *property = impl_from_ISharedProperty(iface);
+
+    TRACE("iface %p, riid %s, out %p.\n", iface, debugstr_guid(riid), out);
+
+    if (IsEqualGUID(riid, &IID_IUnknown)
+            || IsEqualGUID(riid, &IID_IDispatch)
+            || IsEqualGUID(riid, &IID_ISharedProperty))
+    {
+        *out = &property->ISharedProperty_iface;
+        IUnknown_AddRef((IUnknown *)*out);
+        return S_OK;
+    }
+
+    WARN("%s not implemented.\n", debugstr_guid(riid));
+    *out = NULL;
+    return E_NOINTERFACE;
+}
+
+static ULONG WINAPI shared_property_AddRef(ISharedProperty *iface)
+{
+    struct shared_property *property = impl_from_ISharedProperty(iface);
+    ULONG refcount = InterlockedIncrement(&property->refcount);
+
+    TRACE("%p increasing refcount to %u.\n", iface, refcount);
+
+    return refcount;
+}
+
+static ULONG WINAPI shared_property_Release(ISharedProperty *iface)
+{
+    struct shared_property *property = impl_from_ISharedProperty(iface);
+    ULONG refcount = InterlockedDecrement(&property->refcount);
+
+    TRACE("%p decreasing refcount to %u.\n", iface, refcount);
+
+    if (!refcount)
+    {
+        SysFreeString(property->name);
+    }
+
+    return refcount;
+}
+
+static HRESULT WINAPI shared_property_GetTypeInfoCount(ISharedProperty *iface, UINT *info)
+{
+    TRACE("iface %p, info %p.\n", iface, info);
+
+    if (!info)
+        return E_INVALIDARG;
+    *info = 1;
+    return S_OK;
+}
+
+static HRESULT WINAPI shared_property_GetTypeInfo(ISharedProperty *iface, UINT index, LCID lcid, ITypeInfo **info)
+{
+    TRACE("iface %p, index %u, lcid %u, info %p.\n", iface, index, lcid, info);
+
+    if (index)
+        return DISP_E_BADINDEX;
+
+    return get_typeinfo(ISharedProperty_tid, info);
+}
+
+static HRESULT WINAPI shared_property_GetIDsOfNames(ISharedProperty *iface, REFIID riid, LPOLESTR *names, UINT count,
+        LCID lcid, DISPID *dispid)
+{
+    ITypeInfo *typeinfo;
+    HRESULT hr;
+
+    TRACE("iface %p, riid %s, names %p, count %u, lcid %u, dispid %p.\n",
+            iface, debugstr_guid(riid), names, count, lcid, dispid);
+
+    hr = get_typeinfo(ISharedProperty_tid, &typeinfo);
+    if (SUCCEEDED(hr))
+    {
+        hr = ITypeInfo_GetIDsOfNames(typeinfo, names, count, dispid);
+        ITypeInfo_Release(typeinfo);
+    }
+
+    return hr;
+}
+
+static HRESULT WINAPI shared_property_Invoke(ISharedProperty *iface, DISPID member, REFIID riid, LCID lcid,
+        WORD flags, DISPPARAMS *params, VARIANT *result, EXCEPINFO *except, UINT *argerr)
+{
+    ITypeInfo *typeinfo;
+    HRESULT hr;
+
+    TRACE("iface %p, member %u, riid %s, lcid %u, flags %x, params %p, result %p, except %p, argerr %p.\n",
+            iface, member, debugstr_guid(riid), lcid, flags, params, result, except, argerr);
+
+    hr = get_typeinfo(ISharedProperty_tid, &typeinfo);
+    if (SUCCEEDED(hr))
+    {
+        hr = ITypeInfo_Invoke(typeinfo, iface, member, flags, params, result, except, argerr);
+        ITypeInfo_Release(typeinfo);
+    }
+
+    return hr;
+}
+
+static HRESULT WINAPI shared_property_get_Value(ISharedProperty *iface, VARIANT *value)
+{
+    FIXME("iface %p, value %p: stub.\n", iface, value);
+    return E_NOTIMPL;
+}
+
+static HRESULT WINAPI shared_property_put_Value(ISharedProperty *iface, VARIANT value)
+{
+    FIXME("iface %p, value %s: stub.\n", iface, debugstr_variant(&value));
+    return E_NOTIMPL;
+}
+
+static const ISharedPropertyVtbl shared_property_vtbl =
+{
+    shared_property_QueryInterface,
+    shared_property_AddRef,
+    shared_property_Release,
+    shared_property_GetTypeInfoCount,
+    shared_property_GetTypeInfo,
+    shared_property_GetIDsOfNames,
+    shared_property_Invoke,
+    shared_property_get_Value,
+    shared_property_put_Value
+};
+
 static HRESULT WINAPI property_group_QueryInterface(ISharedPropertyGroup *iface, REFIID riid, void **out)
 {
     struct property_group *group = impl_from_ISharedPropertyGroup(iface);
@@ -93,12 +267,14 @@ static ULONG WINAPI property_group_Release(ISharedPropertyGroup *iface)
         struct group_manager *manager = group->parent;
 
         SysFreeString(group->name);
+        heap_free(group->properties);
 
         EnterCriticalSection(&manager->cs);
         list_remove(&group->entry);
         LeaveCriticalSection(&manager->cs);
 
         ISharedPropertyGroupManager_Release(&manager->ISharedPropertyGroupManager_iface);
+        DeleteCriticalSection(&group->cs);
         heap_free(group);
     }
 
@@ -163,6 +339,18 @@ static HRESULT WINAPI property_group_Invoke(ISharedPropertyGroup *iface, DISPID
     return hr;
 }
 
+static struct shared_property *find_shared_property(struct property_group *group, BSTR name)
+{
+    int index;
+
+    for (index = 0; index < group->count; index++)
+    {
+        if (!lstrcmpW(group->properties[index].name, name))
+            return &group->properties[index];
+    }
+    return NULL;
+}
+
 static HRESULT WINAPI property_group_CreatePropertyByPosition(ISharedPropertyGroup *iface, int index,
         VARIANT_BOOL *exists, ISharedProperty **property)
 {
@@ -180,8 +368,54 @@ static HRESULT WINAPI property_group_get_PropertyByPosition(ISharedPropertyGroup
 static HRESULT WINAPI property_group_CreateProperty(ISharedPropertyGroup *iface, BSTR name, VARIANT_BOOL *exists,
         ISharedProperty **property)
 {
-    FIXME("iface %p, name %s, exists %p, property %p: stub.\n", iface, debugstr_w(name), exists, property);
-    return E_NOTIMPL;
+    struct property_group *group = impl_from_ISharedPropertyGroup(iface);
+    struct shared_property *shared_property;
+    BSTR property_name;
+    HRESULT hr;
+
+    TRACE("iface %p, name %s, exists %p, property %p.\n", iface, debugstr_w(name), exists, property);
+
+    EnterCriticalSection(&group->cs);
+
+    shared_property = find_shared_property(group, name);
+    if (!shared_property)
+    {
+        property_name = SysAllocString(name);
+        if (!property_name)
+        {
+            heap_free(shared_property);
+            LeaveCriticalSection(&group->cs);
+            return E_OUTOFMEMORY;
+        }
+
+        if (!comsvcs_array_reserve((void **)&group->properties, &group->capacity, group->count + 1,
+                        sizeof(*group->properties)))
+        {
+            SysFreeString(property_name);
+            LeaveCriticalSection(&group->cs);
+            return E_OUTOFMEMORY;
+        }
+
+        shared_property = &group->properties[group->count];
+        shared_property->refcount = 1;
+        shared_property->name = property_name;
+        shared_property->ISharedProperty_iface.lpVtbl = &shared_property_vtbl;
+
+        group->count++;
+        *exists = FALSE;
+        *property = &shared_property->ISharedProperty_iface;
+        hr = S_OK;
+    }
+    else
+    {
+        *exists = TRUE;
+        hr = ISharedProperty_QueryInterface(&shared_property->ISharedProperty_iface,
+                &IID_ISharedProperty, (void **)property);
+    }
+
+    LeaveCriticalSection(&group->cs);
+
+    return hr;
 }
 
 static HRESULT WINAPI property_group_get_Property(ISharedPropertyGroup *iface, BSTR name, ISharedProperty **property)
@@ -349,6 +583,17 @@ static HRESULT WINAPI group_manager_CreatePropertyGroup(ISharedPropertyGroupMana
         property_group->isolation = *isolation;
         property_group->release = *release;
         property_group->refcount = 1;
+        property_group->capacity = 0;
+        property_group->count = 0;
+        property_group->properties = NULL;
+        if (!comsvcs_array_reserve((void **)&property_group->properties, &property_group->capacity, 2,
+                        sizeof(*property_group->properties)))
+        {
+            heap_free(property_group);
+            LeaveCriticalSection(&manager->cs);
+            return E_OUTOFMEMORY;
+        }
+
         property_group->name = SysAllocString(name);
         if (!property_group->name)
         {
@@ -356,6 +601,7 @@ static HRESULT WINAPI group_manager_CreatePropertyGroup(ISharedPropertyGroupMana
             heap_free(property_group);
             return E_OUTOFMEMORY;
         }
+        InitializeCriticalSection(&property_group->cs);
 
         list_add_tail(&manager->property_groups, &property_group->entry);
 
diff --git a/dlls/comsvcs/tests/property.c b/dlls/comsvcs/tests/property.c
index 7321bbcfe6..1f62656e7a 100644
--- a/dlls/comsvcs/tests/property.c
+++ b/dlls/comsvcs/tests/property.c
@@ -320,12 +320,93 @@ static void test_property_group(void)
     ISharedPropertyGroupManager_Release(manager);
 }
 
+static void test_shared_property(void)
+{
+    ISharedProperty *property, *property1;
+    ISharedPropertyGroupManager *manager;
+    ULONG refcount, expected_refcount;
+    ISharedPropertyGroup *group;
+    BSTR name, property_name;
+    LONG isolation, release;
+    VARIANT_BOOL exists;
+    IDispatch *dispatch;
+    HRESULT hr;
+    static const struct test_name_id test_name_ids[] =
+    {
+        {L"Value", DISPID_VALUE},
+    };
+
+    hr = CoCreateInstance(&CLSID_SharedPropertyGroupManager, NULL, CLSCTX_INPROC_SERVER,
+            &IID_ISharedPropertyGroupManager, (void **)&manager);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+
+    name = SysAllocString(L"testgroupname");
+    isolation = 0;
+    release = 0;
+    exists = FALSE;
+    hr = ISharedPropertyGroupManager_CreatePropertyGroup(manager, name, &isolation, &release, &exists, &group);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+    SysFreeString(name);
+
+    property_name = SysAllocString(L"testpropertyname");
+    exists = TRUE;
+    expected_refcount = get_refcount(group);
+    hr = ISharedPropertyGroup_CreateProperty(group, property_name, &exists, &property);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+    ok(!exists, "Got unexpected value: %d.\n", exists);
+    refcount = get_refcount(group);
+    ok(refcount == expected_refcount, "Got refcount: %u, expected %u.\n", refcount, expected_refcount);
+
+    hr = ISharedPropertyGroup_get_PropertyByPosition(group, 0, &property1);
+    todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
+    hr = ISharedPropertyGroup_get_PropertyByPosition(group, 1, &property1);
+    todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
+
+    exists = FALSE;
+    expected_refcount = get_refcount(property) + 1;
+    hr = ISharedPropertyGroup_CreateProperty(group, property_name, &exists, &property1);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+    ok(!!exists, "Got unexpected value: %d.\n", exists);
+    refcount = get_refcount(property);
+    ok(refcount == expected_refcount, "Got refcount: %u, expected %u.\n", refcount, expected_refcount);
+    expected_refcount--;
+    ISharedProperty_Release(property1);
+    refcount = get_refcount(property);
+    ok(refcount == expected_refcount, "Got refcount: %u, expected %u.\n", refcount, expected_refcount);
+
+    expected_refcount = get_refcount(property) + 1;
+    property1 = NULL;
+    hr = ISharedPropertyGroup_get_Property(group, property_name, &property1);
+    todo_wine ok(hr == S_OK, "Got hr %#x.\n", hr);
+    todo_wine ok(property1 == property, "Got wrong pointer %p.\n", property);
+    refcount = get_refcount(property);
+    todo_wine ok(refcount == expected_refcount, "Got refcount: %u, expected %u.\n", refcount, expected_refcount);
+    if (property1)
+        ISharedProperty_Release(property1);
+    SysFreeString(property_name);
+
+    property_name = SysAllocString(L"Testpropertyname");
+    hr = ISharedPropertyGroup_get_Property(group, property_name, &property1);
+    todo_wine ok(hr == E_INVALIDARG, "Got hr %#x.\n", hr);
+    SysFreeString(property_name);
+
+    hr = ISharedProperty_QueryInterface(property, &IID_IDispatch, (void **)&dispatch);
+    ok(hr == S_OK, "Got hr %#x.\n", hr);
+    TEST_TYPEINFO(dispatch, test_name_ids, ARRAY_SIZE(test_name_ids), &IID_ISharedProperty);
+    IDispatch_Release(dispatch);
+
+    ISharedProperty_Release(property);
+    ISharedPropertyGroup_Release(group);
+    ISharedPropertyGroupManager_Release(manager);
+}
+
 START_TEST(property)
 {
     CoInitialize(NULL);
 
     test_interfaces();
     test_property_group();
+    test_shared_property();
 
     CoUninitialize();
 }
-- 
2.28.0

[Marvin]

Hi,

While running your changed tests, 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=77763

Your paranoid android.


=== debiant (32 bit report) ===

comsvcs:
property.c:130: Test failed: Got hr 0x80040154.
property.c:134: Test failed: Got hr 0x80040154.
Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00403b4d).

Report validation errors:
comsvcs:property crashed (c0000005)

=== debiant (32 bit French report) ===

comsvcs:
property.c:130: Test failed: Got hr 0x80040154.
property.c:134: Test failed: Got hr 0x80040154.
Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00403b4d).

Report validation errors:
comsvcs:property crashed (c0000005)

=== debiant (32 bit Japanese:Japan report) ===

comsvcs:
property.c:130: Test failed: Got hr 0x80040154.
property.c:134: Test failed: Got hr 0x80040154.
Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00403b4d).

Report validation errors:
comsvcs:property crashed (c0000005)

=== debiant (32 bit Chinese:China report) ===

comsvcs:
property.c:130: Test failed: Got hr 0x80040154.
property.c:134: Test failed: Got hr 0x80040154.
Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00403b4d).

Report validation errors:
comsvcs:property crashed (c0000005)

=== debiant (32 bit WoW report) ===

comsvcs:
property.c:130: Test failed: Got hr 0x80040154.
property.c:134: Test failed: Got hr 0x80040154.
Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00403b4d).

Report validation errors:
comsvcs:property crashed (c0000005)

=== debiant (64 bit WoW report) ===

comsvcs:
property.c:130: Test failed: Got hr 0x80040154.
property.c:134: Test failed: Got hr 0x80040154.
Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x00403b4d).

Report validation errors:
comsvcs:property crashed (c0000005)

[Nikolay Sivov]

On 8/30/20 6:04 PM, Jactry Zeng wrote:
> @@ -37,10 +44,44 @@ struct property_group
>      LONG isolation, release;
>      struct list entry;
>      BSTR name;
> +    struct shared_property *properties;
> +    size_t capacity, count;
> +    CRITICAL_SECTION cs;
>  };
> According to docs they have two parallel lists/arrays, one indexed,
> another named, that don't interact. The one you're introducing does not
> have to use an array.