Re: [PATCH] wininet: Don't try to read data when the server response is HTTP_STATUS_NOT_MODIFIED

[Jacek Caban]

Hi Young,

The fix itself seems right, but the test needs more work. Since we close
connection after each request in server_thread, the test passes even
without your fix. You could probably just include some garbage data
after response so that we can see that it's not used.

Also see some minor comments bellow.

On 08.02.2017 01:38, Young Chung wrote:
> diff --git a/dlls/wininet/tests/http.c b/dlls/wininet/tests/http.c
> index fb64e3c..8cdeb8e 100644
> --- a/dlls/wininet/tests/http.c
> +++ b/dlls/wininet/tests/http.c
> @@ -2017,6 +2017,10 @@ static const char notokmsg[] =
>  "Server: winetest\r\n"
>  "\r\n";
>
> +static const char notmodified[] =
> +"HTTP/1.1 304 Not Modified\r\n"
> +"\r\n";
> -
>
> static const char noauthmsg[] =
>  "HTTP/1.1 401 Unauthorized\r\n"
>  "Server: winetest\r\n"
> @@ -2295,6 +2299,11 @@ static DWORD CALLBACK server_thread(LPVOID param)
>              WaitForSingleObject(conn_close_event, INFINITE);
>              trace("closing connection\n");
>          }
> -
>    if (strstr(buffer, "GET /test_not_modified"))
> -
>    {
> -
>        if (strstr(buffer, "If-Modified-Since:")) send(c, notmodified, sizeof notmodified - 1, 0);
> -
>        else send(c, notokmsg, sizeof(notokmsg) - 1, 0);
> -
>    }
>    if (strstr(buffer, "GET /test_cache_control_no_cache"))
>    {
>        static const char no_cache_response[] = "HTTP/1.1 200 OK\r\nCache-Control: no-cache\r\n\r\nsome content";
>
> @@ -3470,6 +3479,56 @@ static void test_conn_close(int port)
>      CloseHandle(hCompleteEvent);
>  }
>
> +static void test_not_modified(int port)
> +{
> - static const char ifmodified[] = {'I','f','-','M','o','d','i','f','i','e','d','-','S','i','n','c','e',':',' '};
> - HINTERNET session, connection, req;
> - DWORD res, status, size;
> - BOOL ret;
> - SYSTEMTIME st;
> - char today[sizeof(ifmodified) + INTERNET_RFC1123_BUFSIZE + 3], buffer[32];
> -
> - trace("Testing 304 not modified response...\n");
> -
> - memcpy(today, ifmodified, sizeof(ifmodified));
> - GetSystemTime(&st);
> - InternetTimeFromSystemTimeA(&st, INTERNET_RFC1123_FORMAT, &today[sizeof(ifmodified)], sizeof(today)-sizeof(ifmodified));
> -
> - session = InternetOpenA("", INTERNET_OPEN_TYPE_DIRECT, NULL, NULL, 0);
> - ok(session != NULL,"InternetOpen failed with error %u\n", GetLastError());
> -
> - connection = InternetConnectA(session, "localhost", port,
> -
>        NULL, NULL, INTERNET_SERVICE_HTTP, 0x0, 0xdeadbeef);
> - ok(connection != NULL,"InternetConnect failed with error %u\n", GetLastError());
> -
> - req = HttpOpenRequestA(connection, "GET", "/test_not_modified", NULL, NULL, NULL,
> -
>        INTERNET_FLAG_KEEP_CONNECTION | INTERNET_FLAG_RESYNCHRONIZE, 0xdeadbead);
> - ok(req != NULL, "HttpOpenRequest failed: %u\n", GetLastError());

Please use open_simple_request helper here.
> -
> - res = HttpSendRequestA(req, today, -1, NULL, 0);
> - ok(res, "HttpSendRequest failed: %u\n", GetLastError());
> -
> - buffer[0] = 0;
> - size = sizeof(buffer);
> - ret = HttpQueryInfoA(req, HTTP_QUERY_IF_MODIFIED_SINCE | HTTP_QUERY_FLAG_REQUEST_HEADERS, &buffer, &size, NULL);
> - ok(ret, "HttpQueryInfoA(HTTP_QUERY_IF_MODIFIED_SINCE) failed %u\n", GetLastError());
> -
> - status = 0xdeadbeef;
> - size = sizeof(status);
> - ret = HttpQueryInfoA(req, HTTP_QUERY_STATUS_CODE | HTTP_QUERY_FLAG_NUMBER, &status, &size, NULL);
> - ok(ret, "HttpQueryInfoA(HTTP_QUERY_STATUS_CODE) failed %u\n", GetLastError());
> - ok(status == 304, "got %u\n", status);

It would be nice to test HTTP_QUERY_CONTENT_LENGTH here as well.
> -
> - size = 0xdeadbeef;
> - res = InternetQueryDataAvailable(req, &size, 0, 0);
> - ok(res, "InternetQueryDataAvailable failed: %u\n", GetLastError());
> - ok(size == 0, "size != 0\n");
> -
> - InternetCloseHandle(req);
> - InternetCloseHandle(connection);
> - InternetCloseHandle(session);
>
> +}

Use close_request().

Thanks,
Jacek