[PATCH 0/2] MR4925: winhttp: Get output synchronously whenever possible in WinHttpQueryDataAvailable / WinHttpReadData.

[Paul Gofman (＠gofman)]

It looks like when the data is available but the recursion limit is reached Windows (after 7 when WinHttpQueryDataAvailable is concerned) fills the output immediately and only queues completion callback. I hope the updated test should be stable as we should have the server response read during WinHttpSendRequest already (like on Windows).

Fixes market window in Black Desert Online.
The game depends on WinHttpReadData returning the result synchronously in WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE callback (and doesn't mind WINHTTP_CALLBACK_STATUS_READ_COMPLETE at all). By the time it reaches there through previous callbacks the recursion limit doesn't allow full synchronous return (and after playing with test_recursion() I think at least Windows 10 has the same recursion limit as we do currently). But as the test shows the output of WinHttpReadData / WinHttpQueryDataAvailable is filled in immediately, probably only the callback should be queued asynchronously to break the recursion.

[PATCH 1/2] winhttp: Always return result at once if available in WinHttpQueryDataAvailable().

[Paul Gofman]

From: Paul Gofman

---
 dlls/winhttp/request.c            | 59 ++++++++++++++++++++++++++++---
 dlls/winhttp/tests/notification.c |  7 +++-
 dlls/winhttp/winhttp_private.h    | 13 +++++++
 3 files changed, 73 insertions(+), 6 deletions(-)

diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c
index 84af8e58988..74278195fc3 100644
--- a/dlls/winhttp/request.c
+++ b/dlls/winhttp/request.c
@@ -257,6 +257,16 @@ static BOOL cancel_queue( struct queue *queue )
     return cancelled;
 }

+static void task_send_callback( void *ctx, BOOL abort )
+{
+    struct send_callback *s = ctx;
+
+    if (abort) return;
+
+    TRACE( "running %p\n", ctx );
+    send_callback( s->task_hdr.obj, s->status, s->info, s->buflen );
+}
+
 static void free_header( struct header *header )
 {
     free( header->field );
@@ -3052,9 +3062,10 @@ static DWORD query_data_ready( struct request *request )
     return count;
 }

-static BOOL skip_async_queue( struct request *request )
+static BOOL skip_async_queue( struct request *request, BOOL *data_available )
 {
-    return request->hdr.recursion_count < 3 && (end_of_read_data( request ) || query_data_ready( request ));
+    *data_available = end_of_read_data( request ) || query_data_ready( request );
+    return request->hdr.recursion_count < 3 && *data_available;
 }

static DWORD query_data_available( struct request *request, DWORD *available, BOOL async )
@@ -3106,6 +3117,7 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available )
     DWORD ret;
     struct request *request;
     BOOL async;
+    BOOL data_available = FALSE;

TRACE("%p, %p\n", hrequest, available);

@@ -3121,7 +3133,44 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available )
         return FALSE;
     }

-    if ((async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) && !skip_async_queue( request ))
+    if (!(async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) || skip_async_queue( request, &data_available ))
+    {
+        ret = query_data_available( request, available, async );
+    }
+    else if (data_available)
+    {
+        /* Data available but recursion limit reached, only queue callback. */
+        struct send_callback *s;
+
+        if (!(s = malloc( sizeof(*s) )))
+        {
+            release_object( &request->hdr );
+            SetLastError( ERROR_OUTOFMEMORY );
+            return FALSE;
+        }
+
+        if (!(ret = query_data_available( request, &s->count, FALSE )))
+        {
+            if (available) *available = s->count;
+            s->status = WINHTTP_CALLBACK_STATUS_DATA_AVAILABLE;
+            s->info = &s->count;
+            s->buflen = sizeof(s->count);
+        }
+        else
+        {
+            s->result.dwResult = API_QUERY_DATA_AVAILABLE;
+            s->result.dwError  = ret;
+            s->status = WINHTTP_CALLBACK_STATUS_REQUEST_ERROR;
+            s->info = &s->result;
+            s->buflen = sizeof(s->result);
+        }
+
+        if ((ret = queue_task( &request->queue, task_send_callback, &s->task_hdr, &request->hdr )))
+            free( s );
+        else
+            ret = ERROR_IO_PENDING;
+    }
+    else
     {
         struct query_data *q;

@@ -3139,7 +3188,6 @@ BOOL WINAPI WinHttpQueryDataAvailable( HINTERNET hrequest, LPDWORD available )
         else
             ret = ERROR_IO_PENDING;
     }
-    else ret = query_data_available( request, available, async );

release_object( &request->hdr );
     SetLastError( ret );
@@ -3165,6 +3213,7 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW
     DWORD ret;
     struct request *request;
     BOOL async;
+    BOOL data_available;

TRACE( "%p, %p, %lu, %p\n", hrequest, buffer, to_read, read );

@@ -3180,7 +3229,7 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW
         return FALSE;
     }

-    if ((async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) && !skip_async_queue( request ))
+    if ((async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) && !skip_async_queue( request, &data_available ))
     {
         struct read_data *r;

diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c
index 78319f0df5c..40db323fb5e 100644
--- a/dlls/winhttp/tests/notification.c
+++ b/dlls/winhttp/tests/notification.c
@@ -1863,6 +1863,7 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex
                                               DWORD status, void *buffer, DWORD buflen )
 {
     struct test_recursion_context *context = (struct test_recursion_context *)context_ptr;
+    static DWORD len;
     DWORD err;
     BOOL ret;
     BYTE b;
@@ -1939,10 +1940,14 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex
             context->max_recursion_read = max( context->max_recursion_read, context->recursion_count );
             context->read_from_callback = TRUE;
             InterlockedIncrement( &context->recursion_count );
-            ret = WinHttpQueryDataAvailable( context->request, NULL );
+            len = 0xdeadbeef;
+            /* Use static variable len here so write to it doesn't destroy the stack on old Windows which
+             * doesn't set the value at once. */
+            ret = WinHttpQueryDataAvailable( context->request, &len );
             err = GetLastError();
             ok( ret, "failed to query data available, GetLastError() %lu\n", err );
             ok( err == ERROR_SUCCESS || err == ERROR_IO_PENDING, "got %lu\n", err );
+            ok( len != 0xdeadbeef || broken( len == 0xdeadbeef ) /* Win7 */, "got %lu.\n", len );
             if (err == ERROR_SUCCESS) context->have_sync_callback = TRUE;
             InterlockedDecrement( &context->recursion_count );
             break;
diff --git a/dlls/winhttp/winhttp_private.h b/dlls/winhttp/winhttp_private.h
index 0bf1c5bc6b0..80ce2614ca1 100644
--- a/dlls/winhttp/winhttp_private.h
+++ b/dlls/winhttp/winhttp_private.h
@@ -312,6 +312,19 @@ struct task_header
     volatile LONG completion_sent;
 };

+struct send_callback
+{
+    struct task_header task_hdr;
+    DWORD status;
+    void *info;
+    DWORD buflen;
+    union
+    {
+        WINHTTP_ASYNC_RESULT result;
+        DWORD count;
+    };
+};
+
 struct send_request
 {
     struct task_header task_hdr;

[PATCH 2/2] winhttp: Always return result at once if available in WinHttpReadData().

[Paul Gofman]

From: Paul Gofman

---
 dlls/winhttp/request.c            | 42 ++++++++++++++++++++++++++++---
 dlls/winhttp/tests/notification.c |  6 ++++-
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c
index 74278195fc3..14e5b493f77 100644
--- a/dlls/winhttp/request.c
+++ b/dlls/winhttp/request.c
@@ -3213,7 +3213,7 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW
     DWORD ret;
     struct request *request;
     BOOL async;
-    BOOL data_available;
+    BOOL data_available = FALSE;

TRACE( "%p, %p, %lu, %p\n", hrequest, buffer, to_read, read );

@@ -3229,7 +3229,44 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW
         return FALSE;
     }

-    if ((async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) && !skip_async_queue( request, &data_available ))
+    if (!(async = request->connect->hdr.flags & WINHTTP_FLAG_ASYNC) || skip_async_queue( request, &data_available ))
+    {
+        ret = read_data( request, buffer, to_read, read, async );
+    }
+    else if (data_available)
+    {
+        /* Data available but recursion limit reached, only queue callback. */
+        struct send_callback *s;
+
+        if (!(s = malloc( sizeof(*s) )))
+        {
+            release_object( &request->hdr );
+            SetLastError( ERROR_OUTOFMEMORY );
+            return FALSE;
+        }
+
+        if (!(ret = read_data( request, buffer, to_read, &s->count, FALSE )))
+        {
+            if (read) *read = s->count;
+            s->status = WINHTTP_CALLBACK_STATUS_READ_COMPLETE;
+            s->info = buffer;
+            s->buflen = s->count;
+        }
+        else
+        {
+            s->result.dwResult = API_READ_DATA;
+            s->result.dwError  = ret;
+            s->status = WINHTTP_CALLBACK_STATUS_REQUEST_ERROR;
+            s->info = &s->result;
+            s->buflen = sizeof(s->result);
+        }
+
+        if ((ret = queue_task( &request->queue, task_send_callback, &s->task_hdr, &request->hdr )))
+            free( s );
+        else
+            ret = ERROR_IO_PENDING;
+    }
+    else
     {
         struct read_data *r;

@@ -3248,7 +3285,6 @@ BOOL WINAPI WinHttpReadData( HINTERNET hrequest, void *buffer, DWORD to_read, DW
         else
             ret = ERROR_IO_PENDING;
     }
-    else ret = read_data( request, buffer, to_read, read, async );

release_object( &request->hdr );
     SetLastError( ret );
diff --git a/dlls/winhttp/tests/notification.c b/dlls/winhttp/tests/notification.c
index 40db323fb5e..ed70ed906f0 100644
--- a/dlls/winhttp/tests/notification.c
+++ b/dlls/winhttp/tests/notification.c
@@ -1921,10 +1921,14 @@ static void CALLBACK test_recursion_callback( HINTERNET handle, DWORD_PTR contex
                 "got %lu, thread %#lx\n", context->recursion_count, GetCurrentThreadId() );
             context->max_recursion_query = max( context->max_recursion_query, context->recursion_count );
             InterlockedIncrement( &context->recursion_count );
-            ret = WinHttpReadData( context->request, &b, 1, NULL );
+            b = 0xff;
+            len = 0xdeadbeef;
+            ret = WinHttpReadData( context->request, &b, 1, &len );
             err = GetLastError();
             ok( ret, "failed to read data, GetLastError() %lu\n", err );
             ok( err == ERROR_SUCCESS || err == ERROR_IO_PENDING, "got %lu\n", err );
+            ok( b != 0xff, "got %#x.\n", b );
+            ok( len == 1, "got %lu.\n", len );
             if (err == ERROR_SUCCESS) context->have_sync_callback = TRUE;
             InterlockedDecrement( &context->recursion_count );
             break;

[PATCH 2/2] winhttp: Always return result at once if available in WinHttpReadData().

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=142311

Your paranoid android.

=== build (build log) ===

error: patch failed: dlls/winhttp/request.c:3213
Task: Patch failed to apply

=== debian11 (build log) ===

error: patch failed: dlls/winhttp/request.c:3213
Task: Patch failed to apply

=== debian11b (build log) ===

error: patch failed: dlls/winhttp/request.c:3213
Task: Patch failed to apply

[Hans Leidekker (＠hans)]

Hans Leidekker (@hans) commented about dlls/winhttp/request.c:
>  return count;
>
> }
>
> -static BOOL skip_async_queue( struct request *request )
> +static BOOL skip_async_queue( struct request *request, BOOL *data_available )
>  {
> - return request->hdr.recursion_count < 3 && (end_of_read_data( request ) || query_data_ready( request ));
> - *data_available = end_of_read_data( request ) || query_data_ready( request );

It's counterintuitive that *data_available is set to true when end_of_read_data() returns true. Perhaps we can find a better name, or pull that condition out of skip_async_queue()?