Re: [PATCH 3/3] urlmon: Add some error handling to the http protocol. (try 2)

[Jacek Caban]

Hi David,

The patch looks much better now, but needs a bit more. BTW, this would 
be better tested on protocol level (like we do in protocol.c) than 
moniker binding, but this will do too.

On 1/3/11 3:47 AM, David Hedberg wrote:
> Try 2: Better tests, among other things.
> ---
>   dlls/urlmon/http.c      |  173 +++++++++++++++++++++++++++++++++++++-
>   dlls/urlmon/tests/url.c |  211 +++++++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 373 insertions(+), 11 deletions(-)
>
> @@ -2546,11 +2670,19 @@ static void test_BindToStorage(int protocol, DWORD flags, DWORD t)
>               SET_EXPECT(QueryInterface_IHttpNegotiate);
>               SET_EXPECT(QueryInterface_IWindowForBindingUI);
>               SET_EXPECT(QueryService_IWindowForBindingUI);
> +            SET_EXPECT(GetWindow_IWindowForBindingUI);
>               SET_EXPECT(BeginningTransaction);
>               SET_EXPECT(QueryInterface_IHttpNegotiate2);
>               SET_EXPECT(GetRootSecurityId);
>               SET_EXPECT(OnProgress_FINDINGRESOURCE);
>               SET_EXPECT(OnProgress_CONNECTING);
> +            if(flags&  BINDTEST_INVALID_CN) {
> +                SET_EXPECT(QueryInterface_IHttpSecurity);
> +                SET_EXPECT(QueryService_IHttpSecurity);
> +                SET_EXPECT(OnSecurityProblem);
> +                if(SUCCEEDED(onsecurityproblem_hres))
> +                    SET_EXPECT(GetWindow_IHttpSecurity);
> +            }
>           }
>           if(!no_callback) {
>               if(test_protocol == HTTP_TEST || test_protocol == HTTPS_TEST || test_protocol == FTP_TEST
> @@ -2600,6 +2732,47 @@ static void test_BindToStorage(int protocol, DWORD flags, DWORD t)
>           ok(hres == INET_E_DATA_NOT_AVAILABLE,
>              "IMoniker_BindToStorage failed: %08x, expected INET_E_DATA_NOT_AVAILABLE\n", hres);
>           ok(unk == NULL, "istr should be NULL\n");
> +    }else if(flags&  BINDTEST_INVALID_CN) {
> +        if(invalid_cn_accepted) {
> +            todo_wine CHECK_NOT_CALLED(QueryInterface_IHttpSecurity);
> +            todo_wine CHECK_NOT_CALLED(QueryService_IHttpSecurity);
> +            todo_wine CHECK_NOT_CALLED(OnSecurityProblem);
> +        }else {
> +            CHECK_CALLED(QueryInterface_IHttpSecurity);
> +            CHECK_CALLED(QueryService_IHttpSecurity);
> +            CHECK_CALLED(OnSecurityProblem);
> +            if(onsecurityproblem_hres != S_OK || security_problem == ERROR_INTERNET_SEC_CERT_ERRORS) {
> +                CHECK_NOT_CALLED(QueryInterface_IInternetBindInfo);
> +                CHECK_NOT_CALLED(QueryService_IInternetBindInfo);
> +                CHECK_CALLED(QueryInterface_IHttpNegotiate);
> +                CHECK_CALLED(BeginningTransaction);
> +                CHECK_CALLED(QueryInterface_IHttpNegotiate2);
> +                CHECK_CALLED(GetRootSecurityId);
> +                CLEAR_CALLED(GetWindow_IWindowForBindingUI);
> +                CLEAR_CALLED(QueryInterface_IWindowForBindingUI);
> +                CLEAR_CALLED(QueryService_IWindowForBindingUI);
> +                if(onsecurityproblem_hres == S_FALSE) {
> +                    if(security_problem == ERROR_INTERNET_SEC_CERT_ERRORS)
> +                        CLEAR_CALLED(OnProgress_FINDINGRESOURCE);
> +                    else
> +                        CHECK_CALLED(OnProgress_FINDINGRESOURCE);
> +                    CHECK_CALLED(GetWindow_IHttpSecurity);
> +                }else {
> +                    todo_wine CHECK_NOT_CALLED(OnProgress_FINDINGRESOURCE);
> +                    CHECK_NOT_CALLED(GetWindow_IHttpSecurity);
> +                }
> +            }
> +        }
> +        if(binding_hres != S_OK) {
> +            ok(hres == binding_hres, "Got %08x\n", hres);
> +            ok(unk == NULL, "Got %p\n", unk);
> +        }else if(invalid_cn_accepted) {
> +            todo_wine ok(hres == S_OK, "IMoniker_BindToStorage failed: %08x\n", hres);
> +            todo_wine ok(unk != NULL, "unk == NULL\n");
> +        }else {
> +            ok(hres == S_OK, "IMoniker_BindToStorage failed: %08x\n", hres);
> +            ok(unk != NULL, "unk == NULL\n");
> +        }

This is making tests even harder to maintain than they are now. I guess 
you're duplicating checking called function here because of returning on 
failure a few lines later. It's the return that shouldn't be there. You 
can change it to

if(FAILED(hres) && !(flags & BINDTEST_INVALID_CN))
     return;

if you don't want to deal with existing problems in error handling.

Jacek