[PATCH 2/2] urlmon: If ProtocolCF_CreateInstance doesn't support aggregation retry without it.
This patch fixes regression caused by 097811f2513e457ebf4afb1d2d21ab90b8684325, and reported in the bug 47190. Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru> --- dlls/urlmon/bindprot.c | 17 +++++++++++++---- dlls/urlmon/tests/url.c | 3 --- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/dlls/urlmon/bindprot.c b/dlls/urlmon/bindprot.c index 42d3d21925..4334d2336b 100644 --- a/dlls/urlmon/bindprot.c +++ b/dlls/urlmon/bindprot.c @@ -509,13 +509,22 @@ static HRESULT WINAPI BindProtocol_StartEx(IInternetProtocolEx *iface, IUri *pUr hres = IClassFactory_CreateInstance(cf, (IUnknown*)&This->IInternetBindInfo_iface, &IID_IUnknown, (void**)&protocol_unk); + if(SUCCEEDED(hres)) + hres = IUnknown_QueryInterface(protocol_unk, &IID_IInternetProtocol, (void**)&protocol); + else if (hres == CLASS_E_NOAGGREGATION) + { + hres = IClassFactory_CreateInstance(cf, NULL, &IID_IInternetProtocol, (void**)&protocol); + if(SUCCEEDED(hres)) + { + protocol_unk = (IUnknown*)protocol; + IUnknown_AddRef(protocol_unk); + } + } IClassFactory_Release(cf); - if(FAILED(hres)) - return hres; - hres = IUnknown_QueryInterface(protocol_unk, &IID_IInternetProtocol, (void**)&protocol); if(FAILED(hres)) { - IUnknown_Release(protocol_unk); + if (protocol_unk) + IUnknown_Release(protocol_unk); return hres; } } diff --git a/dlls/urlmon/tests/url.c b/dlls/urlmon/tests/url.c index 84dedc07f4..5f3199ba37 100644 --- a/dlls/urlmon/tests/url.c +++ b/dlls/urlmon/tests/url.c @@ -3109,11 +3109,8 @@ static void test_BindToStorage(int protocol, DWORD flags, DWORD t) ok(hres == MK_S_ASYNCHRONOUS, "IMoniker_BindToStorage failed: %08x\n", hres); else if(no_callback) { if(emulate_protocol) - todo_wine_if(no_aggregation) ok( WaitForSingleObject(complete_event2, 3000) == WAIT_OBJECT_0, "wait timed out\n" ); - todo_wine_if(no_aggregation) ok(hres == S_OK, "IMoniker_BindToStorage failed: %08x\n", hres); - todo_wine_if(no_aggregation) ok(unk != NULL, "unk == NULL\n"); }else if(!(bindf & BINDF_ASYNCHRONOUS) && tymed == TYMED_FILE) { ok(hres == S_OK, "IMoniker_BindToStorage failed: %08x\n", hres); -- 2.20.1
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=52488 Your paranoid android. === w7u (32 bit report) === urlmon: url.c:3712: Test failed: expected OnProgress_DOWNLOADINGDATA === w864 (32 bit report) === urlmon: url.c:1673: Test failed: unexpected call OnProgress_FINDINGRESOURCE === w1064v1507 (32 bit report) === urlmon: url.c:3712: Test failed: expected OnProgress_DOWNLOADINGDATA === w864 (64 bit report) === urlmon: url.c:1671: Test failed: unexpected call OnProgress_FINDINGRESOURCE
Hi Dmitry, Those changes look mostly good, but we should probably not forward QI calls in BindProtocol_QueryInterface if the protocol doesn't support aggregation. Also I don't mind tests from patch 1, but it would be probably easier to extend test_CreateBinding(). It would also make a testing the above comment trivial. Thanks, Jacek
Jacek Caban <jacek(a)codeweavers.com> wrote:
Those changes look mostly good, but we should probably not forward QI calls in BindProtocol_QueryInterface if the protocol doesn't support aggregation.
Also I don't mind tests from patch 1, but it would be probably easier to extend test_CreateBinding(). It would also make a testing the above comment trivial.
Thanks for the review. Perhaps the patches could be accepted in current state, and I'll have a look at improving the tests according to your comments? -- Dmitry.
On 5/20/19 1:41 PM, Dmitry Timoshkov wrote:
Jacek Caban <jacek(a)codeweavers.com> wrote:
Those changes look mostly good, but we should probably not forward QI calls in BindProtocol_QueryInterface if the protocol doesn't support aggregation.
Also I don't mind tests from patch 1, but it would be probably easier to extend test_CreateBinding(). It would also make a testing the above comment trivial. Thanks for the review. Perhaps the patches could be accepted in current state, and I'll have a look at improving the tests according to your comments?
QueryInterface problem that I mentioned is a COM rule violation (see 3rd rule in [1]). Unless we have a prove that we should do otherwise, QueryInterface should be fixed in the same commit as StartEx. Otherwise we'd be trading one bug for another. Thanks, Jacek [1] https://docs.microsoft.com/en-us/windows/desktop/com/rules-for-implementing-...
Jacek Caban <jacek(a)codeweavers.com> wrote:
Those changes look mostly good, but we should probably not forward QI calls in BindProtocol_QueryInterface if the protocol doesn't support aggregation.
Also I don't mind tests from patch 1, but it would be probably easier to extend test_CreateBinding(). It would also make a testing the above comment trivial.
I've moved the tests to test_CreateBinding(), however making them pass requires more knowledge about interaction between the object and outer interface, I'd appreciate your help with it, I miss an outer reference count decrease somewhere, and couldn't figure this out yet. -- Dmitry.
Hi Dmitry, On 5/21/19 12:33 PM, Dmitry Timoshkov wrote:
Jacek Caban <jacek(a)codeweavers.com> wrote:
Those changes look mostly good, but we should probably not forward QI calls in BindProtocol_QueryInterface if the protocol doesn't support aggregation.
Also I don't mind tests from patch 1, but it would be probably easier to extend test_CreateBinding(). It would also make a testing the above comment trivial. I've moved the tests to test_CreateBinding(), however making them pass requires more knowledge about interaction between the object and outer interface, I'd appreciate your help with it, I miss an outer reference count decrease somewhere, and couldn't figure this out yet.
Please test the attached patch with your application. Jacek
Jacek Caban <jacek(a)codeweavers.com> wrote:
Jacek Caban <jacek(a)codeweavers.com> wrote:
Those changes look mostly good, but we should probably not forward QI calls in BindProtocol_QueryInterface if the protocol doesn't support aggregation.
Also I don't mind tests from patch 1, but it would be probably easier to extend test_CreateBinding(). It would also make a testing the above comment trivial. I've moved the tests to test_CreateBinding(), however making them pass requires more knowledge about interaction between the object and outer interface, I'd appreciate your help with it, I miss an outer reference count decrease somewhere, and couldn't figure this out yet.
Please test the attached patch with your application.
Thanks, as expected your patch fixes the regression, but the tests I've just sent crash. -- Dmitry.
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=52694 Your paranoid android. === debian9 (32 bit report) === urlmon: protocol.c:3879: Test succeeded inside todo block: Start failed: 00000000 protocol.c:2309: Test failed: unexpected call Protocol_destructor protocol.c:3966: Test failed: expected Continue Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x7e41f34b). Report errors: urlmon:protocol crashed (c0000005) === debian9 (32 bit Chinese:China report) === urlmon: protocol.c:3879: Test succeeded inside todo block: Start failed: 00000000 protocol.c:2309: Test failed: unexpected call Protocol_destructor protocol.c:3961: Test failed: protocol_outer_ref = 1208696 protocol.c:3966: Test failed: expected Continue Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x7e41f34b). Report errors: urlmon:protocol crashed (c0000005) === debian9 (32 bit WoW report) === urlmon: protocol.c:3879: Test succeeded inside todo block: Start failed: 00000000 protocol.c:2309: Test failed: unexpected call Protocol_destructor protocol.c:3966: Test failed: expected Continue Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x7e41f34b). Report errors: urlmon:protocol crashed (c0000005) === debian9 (64 bit WoW report) === urlmon: protocol.c:3879: Test succeeded inside todo block: Start failed: 00000000 protocol.c:2309: Test failed: unexpected call Protocol_destructor protocol.c:3966: Test failed: expected Continue Unhandled exception: page fault on read access to 0x00000000 in 32-bit code (0x7e41f34b). Report errors: urlmon:protocol crashed (c0000005)
participants (3)
-
Dmitry Timoshkov -
Jacek Caban -
Marvin