Re: [PATCH 4/6] urlmon: Add some error handling to the http protocol.

[Jacek Caban]

Hi David,

On 12/22/10 12:50 AM, David Hedberg wrote:
> dlls/urlmon/http.c      |   99 +++++++++++++++++++++++++++++++++++++++++++++--
>   dlls/urlmon/tests/url.c |   25 ++++++++++++
>   2 files changed, 120 insertions(+), 4 deletions(-)
>
> diff --git a/dlls/urlmon/http.c b/dlls/urlmon/http.c
> index cfa2fc6..1ed5e15 100644
> --- a/dlls/urlmon/http.c
> +++ b/dlls/urlmon/http.c
> @@ -80,6 +80,92 @@ static LPWSTR query_http_info(HttpProtocol *This, DWORD option)
>       return ret;
>   }
>
> +static HRESULT handle_http_error(HttpProtocol *This, DWORD error)
> +{
> - IWindowForBindingUI *wfb_ui;
> - IHttpSecurity *http_security;
> - BOOL security_problem;
> - HRESULT hres;
> -
> - switch(error) {
> - case ERROR_INTERNET_SEC_CERT_DATE_INVALID:
> - case ERROR_INTERNET_SEC_CERT_CN_INVALID:
> - case ERROR_INTERNET_HTTP_TO_HTTPS_ON_REDIR:
> - case ERROR_INTERNET_HTTPS_TO_HTTP_ON_REDIR:
> - case ERROR_HTTP_REDIRECT_NEEDS_CONFIRMATION:
> - case ERROR_INTERNET_INVALID_CA:
> - case ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED:
> -
>    security_problem = TRUE;
> -
>    break;
> - default:
> -
>    security_problem = FALSE;
> - }
> -
> - if(security_problem) {
> -
>    hres = IUnknown_QueryService((IUnknown*)This->base.protocol_sink,

You shouldn't need the cast here.
> +&IID_IHttpSecurity,&IID_IHttpSecurity, (void **)&http_security);
> -
>    if(SUCCEEDED(hres)) {
> -
>        hres = IHttpSecurity_OnSecurityProblem(http_security, error);
> -
>        IHttpSecurity_Release(http_security);
> -
> -
>        if(hres == S_OK || hres == E_ABORT)
> -
>            return E_ABORT;
> -
>        if(hres == RPC_E_RETRY)
> -
>            return RPC_E_RETRY;
> -
>        if(hres != S_FALSE)
> -
>            return INET_E_SECURITY_PROBLEM;
> -
>    }
> - }
> -
> - hres = IUnknown_QueryService((IUnknown*)This->base.protocol_sink,
>
> +&IID_IWindowForBindingUI,&IID_IWindowForBindingUI, (void **)&wfb_ui);

Same here.
> - if(SUCCEEDED(hres)) {
> -
>    HWND hwnd;
> -
>    const IID *iid_reason;
> -
> -
>    if(security_problem)
> -
>        iid_reason =&IID_IHttpSecurity;
> -
>    else if(error == ERROR_INTERNET_INCORRECT_PASSWORD)
> -
>        iid_reason =&IID_IAuthenticate;
> -
>    else
> -
>        iid_reason =&IID_IWindowForBindingUI;
> -
> -
>    hres = IWindowForBindingUI_GetWindow(wfb_ui, iid_reason,&hwnd);
> -
>    if(SUCCEEDED(hres)&&  hwnd)
> -
>    {
> -
>        DWORD res;
> -
> -
>        res = InternetErrorDlg(hwnd, This->base.request, error,
> -
>                               FLAGS_ERROR_UI_FLAGS_CHANGE_OPTIONS | FLAGS_ERROR_UI_FLAGS_GENERATE_DATA,
> -
>                               NULL);
> -
> -
>        if(res == ERROR_INTERNET_FORCE_RETRY || res == ERROR_SUCCESS)
> -
>            hres = RPC_E_RETRY;
> -
>        else
> -
>            hres = E_FAIL;
> -
>    }
> -
>    IWindowForBindingUI_Release(wfb_ui);
> - }
> -
> - if(hres == RPC_E_RETRY)
> -
>    return hres;
> -
> - switch(error)
> - {
> - case ERROR_INTERNET_SEC_CERT_DATE_INVALID:
> - case ERROR_INTERNET_SEC_CERT_CN_INVALID:
> - case ERROR_INTERNET_INVALID_CA:
> - case ERROR_INTERNET_CLIENT_AUTH_CERT_NEEDED:
> -
>    return INET_E_INVALID_CERTIFICATE;
> - case ERROR_INTERNET_HTTP_TO_HTTPS_ON_REDIR:
> - case ERROR_INTERNET_HTTPS_TO_HTTP_ON_REDIR:
> - case ERROR_HTTP_REDIRECT_NEEDS_CONFIRMATION:
> -
>    return INET_E_REDIRECT_FAILED;
> - default:
> -
>    return INET_E_DOWNLOAD_FAILURE;
> - }
>
> +}
> - static ULONG send_http_request(HttpProtocol *This)
> {
>     INTERNET_BUFFERSW send_buffer = {sizeof(INTERNET_BUFFERSW)};
>
> @@ -282,13 +368,18 @@ static HRESULT HttpProtocol_open_request(Protocol *prot, IUri *uri, DWORD reques
>       if(!res)
>           WARN("InternetSetOption(INTERNET_OPTION_HTTP_DECODING) failed: %08x\n", GetLastError());
> - error = send_http_request(This);
> - do {
> -
>    error = send_http_request(This);
>
> - if(error == ERROR_IO_PENDING || error == ERROR_SUCCESS)
> -
>    return S_OK;
> -
>    if(error == ERROR_IO_PENDING || error == ERROR_SUCCESS)
> -
>        return S_OK;
> -
> -
>    hres = handle_http_error(This, error);
> -
> -
>
> } while(hres == RPC_E_RETRY);
>
> WARN("HttpSendRequest failed: %d\n", error);
> - return INET_E_DOWNLOAD_FAILURE;
> -
>
> return hres;
> }
>
> static HRESULT HttpProtocol_end_request(Protocol *protocol)
>
> diff --git a/dlls/urlmon/tests/url.c b/dlls/urlmon/tests/url.c
> index 875294b..c2cf152 100644
> --- a/dlls/urlmon/tests/url.c
> +++ b/dlls/urlmon/tests/url.c
> @@ -91,10 +91,12 @@ DEFINE_EXPECT(QueryInterface_IInternetBindInfo);
>   DEFINE_EXPECT(QueryInterface_IAuthenticate);
>   DEFINE_EXPECT(QueryInterface_IInternetProtocol);
>   DEFINE_EXPECT(QueryInterface_IWindowForBindingUI);
> +DEFINE_EXPECT(QueryInterface_IHttpSecurity);
>   DEFINE_EXPECT(QueryService_IAuthenticate);
>   DEFINE_EXPECT(QueryService_IInternetProtocol);
>   DEFINE_EXPECT(QueryService_IInternetBindInfo);
>   DEFINE_EXPECT(QueryService_IWindowForBindingUI);
> +DEFINE_EXPECT(QueryService_IHttpSecurity);
>   DEFINE_EXPECT(BeginningTransaction);
>   DEFINE_EXPECT(OnResponse);
>   DEFINE_EXPECT(QueryInterface_IHttpNegotiate2);
> @@ -627,6 +629,7 @@ static HRESULT WINAPI Protocol_Start(IInternetProtocol *iface, LPCWSTR szUrl,
>           IServiceProvider *service_provider;
>           IHttpNegotiate *http_negotiate;
>           IHttpNegotiate2 *http_negotiate2;
> -
>    IHttpSecurity *http_security;
>     LPWSTR ua = (LPWSTR)0xdeadbeef, accept_mimes[256];
>     LPWSTR additional_headers = (LPWSTR)0xdeadbeef;
>     BYTE sec_id[100];
>
> @@ -708,6 +711,18 @@ static HRESULT WINAPI Protocol_Start(IInternetProtocol *iface, LPCWSTR szUrl,
>           ok(hres == E_FAIL, "GetRootSecurityId failed: %08x, expected E_FAIL\n", hres);
>           ok(size == no_callback ? 512 : 13, "size=%d\n", size);
> -
>    if(!no_callback) {
> -
>        SET_EXPECT(QueryService_IHttpSecurity);
> -
>        SET_EXPECT(QueryInterface_IHttpSecurity);
> -
>    }
> -
>    hres = IServiceProvider_QueryService(service_provider,&IID_IHttpSecurity,
>
> +&IID_IHttpSecurity, (void**)&http_security);
> -
>    ok(hres == E_NOINTERFACE, "QueryService failed: 0x%08x\n", hres);
> -
>    if(!no_callback) {
> -
>        CHECK_CALLED(QueryService_IHttpSecurity);
> -
>        CHECK_CALLED(QueryInterface_IHttpSecurity);
> -
>    }
> -
>     IServiceProvider_Release(service_provider);
>
>     IInternetProtocolSink_AddRef(pOIProtSink);
>
> @@ -1212,6 +1227,11 @@ static HRESULT WINAPI ServiceProvider_QueryService(IServiceProvider *iface,
>           return E_NOTIMPL;
>       }
> - if(IsEqualGUID(&IID_IHttpSecurity, guidService)) {
> -
>    CHECK_EXPECT(QueryService_IHttpSecurity);
> -
>    return E_NOTIMPL;
> - }
> -
> ok(0, "unexpected service %s\n", debugstr_guid(guidService));
> return E_NOINTERFACE;
> }
>
> @@ -1292,6 +1312,11 @@ static HRESULT WINAPI statusclb_QueryInterface(IBindStatusCallbackEx *iface, REF
>           CHECK_EXPECT2(QueryInterface_IWindowForBindingUI);
>           return E_NOINTERFACE;
>       }
> - else if(IsEqualGUID(&IID_IHttpSecurity, riid))
> - {
> -
>    CHECK_EXPECT2(QueryInterface_IHttpSecurity);
> -
>    return E_NOINTERFACE;
> - }
>  else
>  {
>      ok(0, "unexpected interface %s\n", debugstr_guid(riid));

Please add some real tests. At least a connection to an invalid 
certificate should be easy to achieve.

Cheers,
     Jacek

[PATCH 4/6] urlmon: Add some error handling to the http protocol.

[David Hedberg]

Hi Jacek,

On Wed, Dec 22, 2010 at 13:07, Jacek Caban  wrote:
> > - if(security_problem) {
> > - hres = IUnknown_QueryService((IUnknown*)This->base.protocol_sink,
>
> You shouldn't need the cast here.

I get warnings if I leave the casts out though, and I can't see any
obvious problem with the declarations. Any ideas?
> Please add some real tests. At least a connection to an invalid certificate
> should be easy to achieve.

I'll look into adding some better tests.

Thanks,
David