Re: [2/2] winhttp: Add custom implementation of IWinHttpRequest::Invoke(DISPID_HTTPREQUEST_OPTION).
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
Hi Jacek, thanks for the review, Jacek Caban <jacek(a)codeweavers.com> 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. -- Dmitry.
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 <jacek(a)codeweavers.com> 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
Jacek Caban <jacek(a)codeweavers.com> 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. -- Dmitry.
Hi Jacek, Any further comments on this? Is there anything else I can provide to justify adding custom WinHttpRequest::Invoke implementation? -- Dmitry.
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
Jacek Caban <jacek(a)codeweavers.com> 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. -- Dmitry.
Hi Jacek,
Jacek Caban <jacek(a)codeweavers.com> 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. -- Dmitry.
Hi Dmitry, On 09/14/15 17:11, Dmitry Timoshkov wrote:
Hi Jacek,
Jacek Caban <jacek(a)codeweavers.com> 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
Hi Jacek, Jacek Caban <jacek(a)codeweavers.com> 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. -- Dmitry.
participants (2)
-
Dmitry Timoshkov -
Jacek Caban