Re: [2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Jacek Caban]

Hi Dmitry,

On 09/02/15 07:44, Dmitry Timoshkov wrote:
> - if (member == DISPID_HTTPREQUEST_OPTION)

Why can't we use standard, ITypeInfo-based implementation here?
> - {
> -
>    VARIANT ret_value, option;
> -
>    UINT err_pos;
> -
> -
>    if (!result) result = &ret_value;
> -
>    if (!arg_err) arg_err = &err_pos;
> -
> -
>    VariantInit( &option );
> -
>    VariantInit( result );
> -
> -
>    if (flags == DISPATCH_PROPERTYPUT)
> -
>    {
> -
>        hr = DispGetParam( params, 0, VT_I4, &option, arg_err );
> -
>        if (FAILED(hr)) return hr;
> -
> -
>        hr = IWinHttpRequest_put_Option( &request->IWinHttpRequest_iface, V_I4( &option ), params->rgvarg[0] );
> -
>        if (FAILED(hr))
> -
>            WARN("put_Option(%d) failed: %x\n", V_I4( &option ), hr);
> -
>        return hr;
> -
>    }
> -
>    else if (flags & (DISPATCH_PROPERTYGET | DISPATCH_METHOD))
> -
>    {
> -
>        hr = DispGetParam( params, 0, VT_I4, &option, arg_err );
> -
>        if (FAILED(hr)) return hr;
> -
> -
>        hr = IWinHttpRequest_get_Option( &request->IWinHttpRequest_iface, V_I4( &option ), result );
> -
>        if (FAILED(hr))
> -
>            WARN("get_Option(%d) failed: %x\n", V_I4( &option ), hr);
> -
>        return hr;
> -
>    }
> -
> -
>    return S_OK;

I don't think silently returning S_OK is right for not handled flags.

Thanks,
Jacek

[2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Dmitry Timoshkov]

Hi Jacek,

thanks for the review,

Jacek Caban  wrote:
> On 09/02/15 07:44, Dmitry Timoshkov wrote:
> > - if (member == DISPID_HTTPREQUEST_OPTION)
>
> Why can't we use standard, ITypeInfo-based implementation here?

The tests (1/2 and existing ones) show that ITypeInfo->Invoke doesn't work
for a lot of cases when native IWinHttpRequest::Invoke succeeds. In fact,
after a lot of attempts I failed to force ITypeInfo->Invoke work at all
under Windows using same code that winhttp request (and many other places
in Wine) uses to load a typelib from winhttp.dll and forward Invoke to
ITypeInfo implementation.
> > - {
> > -
> >    VARIANT ret_value, option;
> > -
> >    UINT err_pos;
> > -
> > -
> >    if (!result) result = &ret_value;
> > -
> >    if (!arg_err) arg_err = &err_pos;
> > -
> > -
> >    VariantInit( &option );
> > -
> >    VariantInit( result );
> > -
> > -
> >    if (flags == DISPATCH_PROPERTYPUT)
> > -
> >    {
> > -
> >        hr = DispGetParam( params, 0, VT_I4, &option, arg_err );
> > -
> >        if (FAILED(hr)) return hr;
> > -
> > -
> >        hr = IWinHttpRequest_put_Option( &request->IWinHttpRequest_iface, V_I4( &option ), params->rgvarg[0] );
> > -
> >        if (FAILED(hr))
> > -
> >            WARN("put_Option(%d) failed: %x\n", V_I4( &option ), hr);
> > -
> >        return hr;
> > -
> >    }
> > -
> >    else if (flags & (DISPATCH_PROPERTYGET | DISPATCH_METHOD))
> > -
> >    {
> > -
> >        hr = DispGetParam( params, 0, VT_I4, &option, arg_err );
> > -
> >        if (FAILED(hr)) return hr;
> > -
> > -
> >        hr = IWinHttpRequest_get_Option( &request->IWinHttpRequest_iface, V_I4( &option ), result );
> > -
> >        if (FAILED(hr))
> > -
> >            WARN("get_Option(%d) failed: %x\n", V_I4( &option ), hr);
> > -
> >        return hr;
> > -
> >    }
> > -
> > -
> >    return S_OK;
>
> I don't think silently returning S_OK is right for not handled flags.

Do you have some particular flags in mind? I tried to follow the test results,
although probably the tests don't cover every case.

[2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Jacek Caban]

Hi Dmitry,

On 09/02/15 11:49, Dmitry Timoshkov wrote:
> Hi Jacek,
>
> thanks for the review,

I'm sorry for the delay, I wanted to take a closer look at those tests.
> Jacek Caban  wrote:
> > On 09/02/15 07:44, Dmitry Timoshkov wrote:
> > > - if (member == DISPID_HTTPREQUEST_OPTION)
> >
> > Why can't we use standard, ITypeInfo-based implementation here?
>
> The tests (1/2 and existing ones) show that ITypeInfo->Invoke doesn't work
> for a lot of cases when native IWinHttpRequest::Invoke succeeds. In fact,
> after a lot of attempts I failed to force ITypeInfo->Invoke work at all
> under Windows using same code that winhttp request (and many other places
> in Wine) uses to load a typelib from winhttp.dll and forward Invoke to
> ITypeInfo implementation.

It seems to me that it's a typelib problem. I think that Invoke
implementation can't find how to handle enum arguments. Did you try if
building the typelib with midl helps? My guess is that the typelib
produced by widl is somehow broken.
> > > - {
> > > -
> > >    VARIANT ret_value, option;
> > > -
> > >    UINT err_pos;
> > > -
> > > -
> > >    if (!result) result = &ret_value;
> > > -
> > >    if (!arg_err) arg_err = &err_pos;
> > > -
> > > -
> > >    VariantInit( &option );
> > > -
> > >    VariantInit( result );
> > > -
> > > -
> > >    if (flags == DISPATCH_PROPERTYPUT)
> > > -
> > >    {
> > > -
> > >        hr = DispGetParam( params, 0, VT_I4, &option, arg_err );
> > > -
> > >        if (FAILED(hr)) return hr;
> > > -
> > > -
> > >        hr = IWinHttpRequest_put_Option( &request->IWinHttpRequest_iface, V_I4( &option ), params->rgvarg[0] );
> > > -
> > >        if (FAILED(hr))
> > > -
> > >            WARN("put_Option(%d) failed: %x\n", V_I4( &option ), hr);
> > > -
> > >        return hr;
> > > -
> > >    }
> > > -
> > >    else if (flags & (DISPATCH_PROPERTYGET | DISPATCH_METHOD))
> > > -
> > >    {
> > > -
> > >        hr = DispGetParam( params, 0, VT_I4, &option, arg_err );
> > > -
> > >        if (FAILED(hr)) return hr;
> > > -
> > > -
> > >        hr = IWinHttpRequest_get_Option( &request->IWinHttpRequest_iface, V_I4( &option ), result );
> > > -
> > >        if (FAILED(hr))
> > > -
> > >            WARN("get_Option(%d) failed: %x\n", V_I4( &option ), hr);
> > > -
> > >        return hr;
> > > -
> > >    }
> > > -
> > > -
> > >    return S_OK;
> >
> > I don't think silently returning S_OK is right for not handled flags.
>
> Do you have some particular flags in mind? I tried to follow the test results,
> although probably the tests don't cover every case.

DISPATCH_PROPERTYPUTREF mostly, maybe some invalid combination. Having a
FIXME like you did in the other version of the patch looks good to me.

Thanks,
Jacek

[2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > > On 09/02/15 07:44, Dmitry Timoshkov wrote:
> > > > - if (member == DISPID_HTTPREQUEST_OPTION)
> > >
> > > Why can't we use standard, ITypeInfo-based implementation here?
> >
> > The tests (1/2 and existing ones) show that ITypeInfo->Invoke doesn't work
> > for a lot of cases when native IWinHttpRequest::Invoke succeeds. In fact,
> > after a lot of attempts I failed to force ITypeInfo->Invoke work at all
> > under Windows using same code that winhttp request (and many other places
> > in Wine) uses to load a typelib from winhttp.dll and forward Invoke to
> > ITypeInfo implementation.
>
> It seems to me that it's a typelib problem. I think that Invoke
> implementation can't find how to handle enum arguments. Did you try if
> building the typelib with midl helps? My guess is that the typelib
> produced by widl is somehow broken.

I tried almost all things that can think of like extracting a binary
typelib from an XP's winhttp.dll and replacing by it a Wine generated
one in the Wine built-in winhttp.dll, extracting with oleview.exe typelib
from XP's winhttp.dll and compiling it with widl and idl and using the
result again in the Wine built-in dll. After that I've written a test under
Windows that does the same as Wine's winhttp's IDispatch implementation:
loads a typelib from winhttp.dll, and uses the resulting ITypeInfo for
experimenting with ITypeInfo_Invoke: noone of the Invoke calls succeed.
So I went to a conclusion that winhttp.dll in Windows most likely uses
custom Invoke implementation, especially taking into account that it
can successfully handle some invalid flags/args usage demonstrated by
the tests.

[2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Dmitry Timoshkov]

Hi Jacek,

Any further comments on this? Is there anything else I can provide to
justify adding custom WinHttpRequest::Invoke implementation?

[2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Jacek Caban]

Hi Dmitry,

On 09/10/15 09:23, Dmitry Timoshkov wrote:
> Hi Jacek,
>
> Any further comments on this? Is there anything else I can provide to
> justify adding custom WinHttpRequest::Invoke implementation?

To be honest, I'm not sure. I tried following test myself:

- Copy native winhttp.dll with a different name (like nwinhttp.dll)
- Change location of typelib in registry to point to native DLL
- Run Wine tests with builtin winhttp.dll

That resulted in a few TODO tests succeeded. It proves that there is a
problem in widl. Sadly, there were still a few failures that are not
present with your patch. It may mean both that our oleaut32 has more
bugs or that we indeed should use custom implementation. It would be
nice to have widl fixed, but due to ambiguous results of my testing, I
can't guarantee that we wouldn't end up with custom Invoke
implementation anyway.

Thanks,
Jacek

[2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Dmitry Timoshkov]

Jacek Caban  wrote:
> > Any further comments on this? Is there anything else I can provide to
> > justify adding custom WinHttpRequest::Invoke implementation?
>
> To be honest, I'm not sure. I tried following test myself:
> - Copy native winhttp.dll with a different name (like nwinhttp.dll)
> - Change location of typelib in registry to point to native DLL
> - Run Wine tests with builtin winhttp.dll
>
> That resulted in a few TODO tests succeeded. It proves that there is a
> problem in widl. Sadly, there were still a few failures that are not
> present with your patch. It may mean both that our oleaut32 has more
> bugs or that we indeed should use custom implementation. It would be
> nice to have widl fixed, but due to ambiguous results of my testing, I
> can't guarantee that we wouldn't end up with custom Invoke
> implementation anyway.

Follwing your test with native winhttp.dll I did the following: copied
winhttp.dll to windows/system32, added assert(0) at the beginning of
dlls/oleaut32/typelib.c,ITypeInfo_fnInvoke() and ran the winhttp tests
like this:
WINEDLLOVERRIDES=winhttp=n wine winhttp_test.exe.so winhttp.c
and assert() never tiggers, while with winhttp=b it immediately fires
an exception.

[2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Dmitry Timoshkov]

Hi Jacek,
> Jacek Caban  wrote:
> > > Any further comments on this? Is there anything else I can provide to
> > > justify adding custom WinHttpRequest::Invoke implementation?
> >
> > To be honest, I'm not sure. I tried following test myself:
> > - Copy native winhttp.dll with a different name (like nwinhttp.dll)
> > - Change location of typelib in registry to point to native DLL
> > - Run Wine tests with builtin winhttp.dll
> >
> > That resulted in a few TODO tests succeeded. It proves that there is a
> > problem in widl. Sadly, there were still a few failures that are not
> > present with your patch. It may mean both that our oleaut32 has more
> > bugs or that we indeed should use custom implementation. It would be
> > nice to have widl fixed, but due to ambiguous results of my testing, I
> > can't guarantee that we wouldn't end up with custom Invoke
> > implementation anyway.
>
> Follwing your test with native winhttp.dll I did the following: copied
> winhttp.dll to windows/system32, added assert(0) at the beginning of
> dlls/oleaut32/typelib.c,ITypeInfo_fnInvoke() and ran the winhttp tests
> like this:
> WINEDLLOVERRIDES=winhttp=n wine winhttp_test.exe.so winhttp.c
> and assert() never tiggers, while with winhttp=b it immediately fires
> an exception.

Is the test above enough to persuade you that winhttp in Windows has its
own Invoke implementation? Even if you suspect that there might be some
problem with the typelib or the way oleaut32 interprets it, I'd suggest
to accept my patch since currently it's the only way to make Invoke in
winhttp work (and as my tests show probably even under Windows), that
shouldn't prevent you from investigating other problems if you still
plan to. What's your opinion on this?

Thanks.

[2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Jacek Caban]

Hi Dmitry,

On 09/14/15 17:11, Dmitry Timoshkov wrote:
> Hi Jacek,
> > Jacek Caban  wrote:
> > > > Any further comments on this? Is there anything else I can provide to
> > > > justify adding custom WinHttpRequest::Invoke implementation?
> > >
> > > To be honest, I'm not sure. I tried following test myself:
> > > - Copy native winhttp.dll with a different name (like nwinhttp.dll)
> > > - Change location of typelib in registry to point to native DLL
> > > - Run Wine tests with builtin winhttp.dll
> > >
> > > That resulted in a few TODO tests succeeded. It proves that there is a
> > > problem in widl. Sadly, there were still a few failures that are not
> > > present with your patch. It may mean both that our oleaut32 has more
> > > bugs or that we indeed should use custom implementation. It would be
> > > nice to have widl fixed, but due to ambiguous results of my testing, I
> > > can't guarantee that we wouldn't end up with custom Invoke
> > > implementation anyway.
> >
> > Follwing your test with native winhttp.dll I did the following: copied
> > winhttp.dll to windows/system32, added assert(0) at the beginning of
> > dlls/oleaut32/typelib.c,ITypeInfo_fnInvoke() and ran the winhttp tests
> > like this:
> > WINEDLLOVERRIDES=winhttp=n wine winhttp_test.exe.so winhttp.c
> > and assert() never tiggers, while with winhttp=b it immediately fires
> > an exception.
>
> Is the test above enough to persuade you that winhttp in Windows has its
> own Invoke implementation?

Your test proves native implementation detail, I'd prefer to talk about
external behaviour instead. However, I believe you've made your point,
winhttp probably differs from default ITypeInfo implementation one way
or another, so custom implementation is a valid change.
> Even if you suspect that there might be some
> problem with the typelib or the way oleaut32 interprets it, I'd suggest
> to accept my patch since currently it's the only way to make Invoke in
> winhttp work (and as my tests show probably even under Windows), that
> shouldn't prevent you from investigating other problems if you still
> plan to. What's your opinion on this?

My personal opinion is that I don't like the patch. If it was me working
on this, I'd check if widl fixes would cover the need of your real
application (which is easy to test even without actually fixing widl)
and if it does, I'd concentrate on that. It would probably fix more bugs
than just one specific winhttp function.

However, in terms of your patch correctness, I believe it is correct, so
I think it is acceptable.

Jacek

[2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).

[Dmitry Timoshkov]

Hi Jacek,

Jacek Caban  wrote:
> > Even if you suspect that there might be some
> > problem with the typelib or the way oleaut32 interprets it, I'd suggest
> > to accept my patch since currently it's the only way to make Invoke in
> > winhttp work (and as my tests show probably even under Windows), that
> > shouldn't prevent you from investigating other problems if you still
> > plan to. What's your opinion on this?
>
> My personal opinion is that I don't like the patch. If it was me working
> on this, I'd check if widl fixes would cover the need of your real
> application (which is easy to test even without actually fixing widl)
> and if it does, I'd concentrate on that. It would probably fix more bugs
> than just one specific winhttp function.

I have tested that already (and mentioned in my previous explanations
https://www.winehq.org/pipermail/wine-devel/2015-September/109076.html):
fixing widl wouldn't fix neither the test cases nor make the application
that I have here work.
> However, in terms of your patch correctness, I believe it is correct, so
> I think it is acceptable.

Thanks.