On Mon Oct 10 21:33:57 2022 +0000, Huw Davies wrote:
> This looks correct to me, however afaict, Windows also sets
> `stubMessage.pRpcChannelBuffer` to `NULL` when it releases it, so it
> would be good to have a test for that, as well as it added to the implementation.
> It would be clearer to split this into two commits: the first commit
> would contain just the tests with appropriate `todo_wine`s. A second
> commit would then add the implementation changes and remove the
> `todo_wine`s. That way it's clear what the new changes are fixing.
Ok, easy to do
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/957#note_10232
Huw Davies (@huw) commented about dlls/rpcrt4/tests/cstub.c:
> + /* stubMessage doesn't add its own refcounts on proxy_if1 or proxy_buffer,
> + * so it's possible these are freed out from under it.
> + * E.g. an event sink might unadvise upon receiving the event it was waiting for;
> + * this unadvise could be reentrant to Invoke because SendReceive pumps STA messages.
> + * The source would then erase that conection point entry and Release the proxy. */
> + IRpcProxyBuffer_Disconnect(proxy_buffer);
> + ok(test_chanbuf.RefCount == 1, "got %ld\n", test_chanbuf.RefCount);
> + IRpcProxyBuffer_Release(proxy_buffer);
> + refs = IUnknown_Release(proxy_if1);
> + ok(refs == 0, "got %ld\n", refs);
> + ok(test_chanbuf.RefCount == 1, "got %ld\n", test_chanbuf.RefCount);
> +
> + /* NdrProxyFreeBuffer must not dereference the the now-freed proxy_if1,
> + * yet should still free the remaining reference on test_chanbuf */
> + NdrProxyFreeBuffer(proxy_if1, &stubMessage);
> + ok(test_chanbuf.RefCount == 0, "got %ld\n", test_chanbuf.RefCount);
This looks correct to me, however afaict, Windows also sets `stubMessage.pRpcChannelBuffer` to `NULL` when it releases it, so it would be good to have a test for that, as well as it added to the implementation.
It would be clearer to split this into two commits: the first commit would contain just the tests with appropriate `todo_wine`s. A second commit would then add the implementation changes and remove the `todo_wine`s. That way it's clear what the new changes are fixing.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/957#note_10221