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