[PATCH 2/2] urlmon: If ProtocolCF_CreateInstance doesn't support aggregation retry without it.

[Dmitry Timoshkov]

This patch fixes regression caused by 097811f2513e457ebf4afb1d2d21ab90b8684325,
and reported in the bug 47190.

Signed-off-by: Dmitry Timoshkov 
---
 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);

[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=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

[Jacek Caban]

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

[Dmitry Timoshkov]

Jacek Caban  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?

[Jacek Caban]

On 5/20/19 1:41 PM, Dmitry Timoshkov wrote:
> Jacek Caban  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-...

[Dmitry Timoshkov]

Jacek Caban  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.

[Jacek Caban]

Hi Dmitry,

On 5/21/19 12:33 PM, Dmitry Timoshkov wrote:
> Jacek Caban  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

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > Jacek Caban  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.

[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=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)