Signed-off-by: Alistair Leslie-Hughes leslie_alistair@hotmail.com --- dlls/dmime/performance.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/dlls/dmime/performance.c b/dlls/dmime/performance.c index 9dbb469f388..a63bd3b4517 100644 --- a/dlls/dmime/performance.c +++ b/dlls/dmime/performance.c @@ -479,7 +479,9 @@ static HRESULT WINAPI IDirectMusicPerformance8Impl_FreePMsg(IDirectMusicPerforma DMUS_ItemRemoveFromQueue( This, pItem ); LeaveCriticalSection(&This->safe);
- /** TODO: see if we should Release the pItem->pMsg->punkUser and others Interfaces */ + if (pPMSG->punkUser) + IUnknown_Release(pPMSG->punkUser); + HeapFree(GetProcessHeap(), 0, pItem); return S_OK; }
Hello Alistair,
while this patch is technically correct http://doc.51windows.net/Directx9_SDK/htm/dmuspmsg.htm I'm wondering why it was triggered by bug #25728 (comments 34 and 35).
I don't see a FreePMsg in the log https://bugs.winehq.org/attachment.cgi?id=65980 I also don't see a FreePMsg in any of the logs I have. Nor in the attachments of all directx-dmusic bugs.
Also we don't set punkUser anywhere in the whole Wine code
wine$ git grep -n punkUser dlls/dmime/performance.c:482: if (pPMSG->punkUser) dlls/dmime/performance.c:483: IUnknown_Release(pPMSG->punkUser); dlls/dmime/tests/performance.c:447: ok(msg->punkUser != NULL, "Unexpected NULL pointer\n"); dlls/dmime/tests/performance.c:448: if (msg->punkUser) { dlls/dmime/tests/performance.c:452: hr = IUnknown_QueryInterface(msg->punkUser, &IID_IDirectMusicSegmentState8, (void**)&segmentstate); include/dmusici.h:363: IUnknown* punkUser;
While we seem to have a test that checks for non-NULL punkUser, that test doesn't runs on Wine. Changing to ok(msg->punkUser == NULL, ...) still passes on Wine. That test does run on Windows though https://testbot.winehq.org/JobDetails.pl?Key=62304
So this patch is a no-op for now iff we allocate zero all the messages. Else we might crash on uninitialized memory. So it does sound like a "Deferred" to me.
bye michael
On 12/18/19 6:17 AM, Alistair Leslie-Hughes wrote:
Signed-off-by: Alistair Leslie-Hughes leslie_alistair@hotmail.com
dlls/dmime/performance.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/dlls/dmime/performance.c b/dlls/dmime/performance.c index 9dbb469f388..a63bd3b4517 100644 --- a/dlls/dmime/performance.c +++ b/dlls/dmime/performance.c @@ -479,7 +479,9 @@ static HRESULT WINAPI IDirectMusicPerformance8Impl_FreePMsg(IDirectMusicPerforma DMUS_ItemRemoveFromQueue( This, pItem ); LeaveCriticalSection(&This->safe);
- /** TODO: see if we should Release the pItem->pMsg->punkUser and others Interfaces */
- if (pPMSG->punkUser)
- IUnknown_Release(pPMSG->punkUser);
- HeapFree(GetProcessHeap(), 0, pItem); return S_OK;
}
Hi Michael,
I have another patch that causes this function to be called (faking a set notification). So, Yes it's currently a noop but it's small and not going to effect anything.
I happy for it to be differed for required.
Alistair.
On 19/12/19 7:55 am, Michael Stefaniuc wrote:
Hello Alistair,
while this patch is technically correct https://nam02.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdoc.51windo... I'm wondering why it was triggered by bug #25728 (comments 34 and 35).
I don't see a FreePMsg in the log https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.wineh... I also don't see a FreePMsg in any of the logs I have. Nor in the attachments of all directx-dmusic bugs.
Also we don't set punkUser anywhere in the whole Wine code
wine$ git grep -n punkUser dlls/dmime/performance.c:482: if (pPMSG->punkUser) dlls/dmime/performance.c:483: IUnknown_Release(pPMSG->punkUser); dlls/dmime/tests/performance.c:447: ok(msg->punkUser != NULL, "Unexpected NULL pointer\n"); dlls/dmime/tests/performance.c:448: if (msg->punkUser) { dlls/dmime/tests/performance.c:452: hr = IUnknown_QueryInterface(msg->punkUser, &IID_IDirectMusicSegmentState8, (void**)&segmentstate); include/dmusici.h:363: IUnknown* punkUser;
While we seem to have a test that checks for non-NULL punkUser, that test doesn't runs on Wine. Changing to ok(msg->punkUser == NULL, ...) still passes on Wine. That test does run on Windows though https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftestbot.wi...
So this patch is a no-op for now iff we allocate zero all the messages. Else we might crash on uninitialized memory. So it does sound like a "Deferred" to me.
bye michael
On 12/18/19 6:17 AM, Alistair Leslie-Hughes wrote:
Signed-off-by: Alistair Leslie-Hughes leslie_alistair@hotmail.com
dlls/dmime/performance.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/dlls/dmime/performance.c b/dlls/dmime/performance.c index 9dbb469f388..a63bd3b4517 100644 --- a/dlls/dmime/performance.c +++ b/dlls/dmime/performance.c @@ -479,7 +479,9 @@ static HRESULT WINAPI IDirectMusicPerformance8Impl_FreePMsg(IDirectMusicPerforma DMUS_ItemRemoveFromQueue( This, pItem ); LeaveCriticalSection(&This->safe);
- /** TODO: see if we should Release the pItem->pMsg->punkUser and others Interfaces */
- if (pPMSG->punkUser)
- IUnknown_Release(pPMSG->punkUser);
- HeapFree(GetProcessHeap(), 0, pItem); return S_OK; }
Hello Alistair,
On 12/19/19 7:59 AM, Alistair Leslie-Hughes wrote:
I have another patch that causes this function to be called (faking a set notification). So, Yes it's currently a noop but it's small and not going to effect anything.
if you need it for a patch that fixes a bug than sure, please resubmit it as patch series with that patch during the freeze.
I've would have signed-off on the patch after reading the documentation I've linked. But the timing wasn't right for a low risk but zero benefit during the freeze patch.
I happy for it to be differed for required.
I've had to create a 'dmdefer' branch anyway for my two outstanding patches. So I've added your patch to that too.
thanks bye michael
Alistair.
On 19/12/19 7:55 am, Michael Stefaniuc wrote:
Hello Alistair,
while this patch is technically correct https://nam02.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdoc.51windo... I'm wondering why it was triggered by bug #25728 (comments 34 and 35).
I don't see a FreePMsg in the log https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.wineh... I also don't see a FreePMsg in any of the logs I have. Nor in the attachments of all directx-dmusic bugs.
Also we don't set punkUser anywhere in the whole Wine code
wine$ git grep -n punkUser dlls/dmime/performance.c:482: if (pPMSG->punkUser) dlls/dmime/performance.c:483: IUnknown_Release(pPMSG->punkUser); dlls/dmime/tests/performance.c:447: ok(msg->punkUser != NULL, "Unexpected NULL pointer\n"); dlls/dmime/tests/performance.c:448: if (msg->punkUser) { dlls/dmime/tests/performance.c:452: hr = IUnknown_QueryInterface(msg->punkUser, &IID_IDirectMusicSegmentState8, (void**)&segmentstate); include/dmusici.h:363: IUnknown* punkUser;
While we seem to have a test that checks for non-NULL punkUser, that test doesn't runs on Wine. Changing to ok(msg->punkUser == NULL, ...) still passes on Wine. That test does run on Windows though https://nam02.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftestbot.wi...
So this patch is a no-op for now iff we allocate zero all the messages. Else we might crash on uninitialized memory. So it does sound like a "Deferred" to me.
bye michael
On 12/18/19 6:17 AM, Alistair Leslie-Hughes wrote:
Signed-off-by: Alistair Leslie-Hughes leslie_alistair@hotmail.com
dlls/dmime/performance.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/dlls/dmime/performance.c b/dlls/dmime/performance.c index 9dbb469f388..a63bd3b4517 100644 --- a/dlls/dmime/performance.c +++ b/dlls/dmime/performance.c @@ -479,7 +479,9 @@ static HRESULT WINAPI IDirectMusicPerformance8Impl_FreePMsg(IDirectMusicPerforma DMUS_ItemRemoveFromQueue( This, pItem ); LeaveCriticalSection(&This->safe);
- /** TODO: see if we should Release the pItem->pMsg->punkUser and others Interfaces */
- if (pPMSG->punkUser)
- IUnknown_Release(pPMSG->punkUser);
- HeapFree(GetProcessHeap(), 0, pItem); return S_OK; }
-
Alistair Leslie-Hughes -
Michael Stefaniuc