Re: wininet: Don't start the next chunk if the read is satisfied.

[Jacek Caban]

Hi Hans,

On 09/04/13 09:22, Hans Leidekker wrote:
> dlls/wininet/http.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c
> index 1832b4f..521e4aa 100644
> --- a/dlls/wininet/http.c
> +++ b/dlls/wininet/http.c
> @@ -2795,7 +2795,7 @@ static DWORD chunked_read(data_stream_t *stream, http_request_t *req, BYTE *buf,
>      chunked_stream_t *chunked_stream = (chunked_stream_t*)stream;
>      DWORD read_bytes = 0, ret_read = 0, res = ERROR_SUCCESS;
> - if(chunked_stream->chunk_size == ~0u) {
> - if(!chunked_stream->chunk_size || chunked_stream->chunk_size == ~0u) {
>     res = start_next_chunk(chunked_stream, req);
>     if(res != ERROR_SUCCESS)
>         return res;
>
> @@ -2835,7 +2835,7 @@ static DWORD chunked_read(data_stream_t *stream, http_request_t *req, BYTE *buf,
>          chunked_stream->chunk_size -= read_bytes;
>          size -= read_bytes;
>          ret_read += read_bytes;
> -
>    if(!chunked_stream->chunk_size) {
> -
>    if(size && !chunked_stream->chunk_size) {
>        assert(read_mode != READMODE_NOBLOCK);
>        res = start_next_chunk(chunked_stream, req);
>        if(res != ERROR_SUCCESS)

This patch breaks two assumptions:
- chunk_size outside chunked_read means the end of the stream
- READMODE_SYNC should read as much data as possible

There is longer story about the way it's implemented the way it is.
Native IE in the past considered zero-length reporting data in
INTERNET_STATUS_REQUEST_COMPLETE to be an error. If you stop reading in
the end of chunk and the next chunk is the last one, then you will send
such report asynchronously. That's why we read the whole chunk only if
we can start reading the next one (and we can't if we don't want to
block). Now I think that the failure could possibly be caused by a
combination of returning zero size and some other bug and the behaviour
that I described could just hide the problem. Commit
851866e22a731141da7e3cbd2550c67c59968959 fixed a problem that could
potentially be the other one.

This means that I'm not entirely sure that we really shouldn't report
zero size data. This is illogical to begin with, but we even have tests
for that:
ok(!on_async, "Returned zero size in response to request complete\n");
I think we should try harder to have a test reporting zero size data
(this may be tricky to write one for Wine tests, but if we could just
observe such occasional notifications, that would be enough). And this
would need more testing with native IE.

This all means that intention of the patch may be right, but it needs
more work.

Thanks,
Jacek

wininet: Don't start the next chunk if the read is satisfied.

[Hans Leidekker]

On Wed, 2013-09-04 at 13:11 +0200, Jacek Caban wrote:
> On 09/04/13 09:22, Hans Leidekker wrote:
> > dlls/wininet/http.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c
> > index 1832b4f..521e4aa 100644
> > --- a/dlls/wininet/http.c
> > +++ b/dlls/wininet/http.c
> > @@ -2795,7 +2795,7 @@ static DWORD chunked_read(data_stream_t *stream, http_request_t *req, BYTE *buf,
> >      chunked_stream_t *chunked_stream = (chunked_stream_t*)stream;
> >      DWORD read_bytes = 0, ret_read = 0, res = ERROR_SUCCESS;
> > - if(chunked_stream->chunk_size == ~0u) {
> > - if(!chunked_stream->chunk_size || chunked_stream->chunk_size == ~0u) {
> >     res = start_next_chunk(chunked_stream, req);
> >     if(res != ERROR_SUCCESS)
> >         return res;
> >
> > @@ -2835,7 +2835,7 @@ static DWORD chunked_read(data_stream_t *stream, http_request_t *req, BYTE *buf,
> >          chunked_stream->chunk_size -= read_bytes;
> >          size -= read_bytes;
> >          ret_read += read_bytes;
> > -
> >    if(!chunked_stream->chunk_size) {
> > -
> >    if(size && !chunked_stream->chunk_size) {
> >        assert(read_mode != READMODE_NOBLOCK);
> >        res = start_next_chunk(chunked_stream, req);
> >        if(res != ERROR_SUCCESS)
>
> This patch breaks two assumptions:
> - chunk_size outside chunked_read means the end of the stream

That's strange, considering that there could always be another chunk. We can't
know if we're at the end of the stream in chunked mode, so I wonder if we should
always try the next chunk (and block) if we're being asked to read more than the
current chunk gives us.

The bug I'm seeing is when the current chunk is exactly the size of the read
request (or has that many bytes left). In that case the current code still tries
to start the next chunk and potentially blocks.
> - READMODE_SYNC should read as much data as possible

But no more than asked for? I don't see how my patch breaks this assumption.

wininet: Don't start the next chunk if the read is satisfied.

[Jacek Caban]

On 09/04/13 14:38, Hans Leidekker wrote:
> On Wed, 2013-09-04 at 13:11 +0200, Jacek Caban wrote:
> > On 09/04/13 09:22, Hans Leidekker wrote:
> > > dlls/wininet/http.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/dlls/wininet/http.c b/dlls/wininet/http.c
> > > index 1832b4f..521e4aa 100644
> > > --- a/dlls/wininet/http.c
> > > +++ b/dlls/wininet/http.c
> > > @@ -2795,7 +2795,7 @@ static DWORD chunked_read(data_stream_t *stream, http_request_t *req, BYTE *buf,
> > >      chunked_stream_t *chunked_stream = (chunked_stream_t*)stream;
> > >      DWORD read_bytes = 0, ret_read = 0, res = ERROR_SUCCESS;
> > > - if(chunked_stream->chunk_size == ~0u) {
> > > - if(!chunked_stream->chunk_size || chunked_stream->chunk_size == ~0u) {
> > >     res = start_next_chunk(chunked_stream, req);
> > >     if(res != ERROR_SUCCESS)
> > >         return res;
> > >
> > > @@ -2835,7 +2835,7 @@ static DWORD chunked_read(data_stream_t *stream, http_request_t *req, BYTE *buf,
> > >          chunked_stream->chunk_size -= read_bytes;
> > >          size -= read_bytes;
> > >          ret_read += read_bytes;
> > > -
> > >    if(!chunked_stream->chunk_size) {
> > > -
> > >    if(size && !chunked_stream->chunk_size) {
> > >        assert(read_mode != READMODE_NOBLOCK);
> > >        res = start_next_chunk(chunked_stream, req);
> > >        if(res != ERROR_SUCCESS)
> >
> > This patch breaks two assumptions:
> > - chunk_size outside chunked_read means the end of the stream

Ops, I meant to write chunk_size==0, which means the end of the stream.
> That's strange, considering that there could always be another chunk. We can't
> know if we're at the end of the stream in chunked mode, so I wonder if we should
> always try the next chunk (and block) if we're being asked to read more than the
> current chunk gives us.
> The bug I'm seeing is when the current chunk is exactly the size of the read
> request (or has that many bytes left). In that case the current code still tries
> to start the next chunk and potentially blocks.

See chunked_end_of_data. It will return TRUE after the call you
mentioned, no matter if there are more chunks to read.

Obviously we could change chunked_end_of_data and the whole assumptions,
but we need better understanding of desired behaviour here. Consider
this sequence:

- chunked_read reads the whole chunk
- app calls InternetQueryDataAvailable, which has no data buffered, so
it schedules async read
- async read reads zero-size chunk (indicating the end of the stream)
- we call INTERNET_REQUEST_COMPLETE, reporting 0 bytes to read

The last call, as I explained previously, has been problematic in the
past and according to all tests we have, it never happens with navitve
wininet (possibly we simply need more testing to see this happening).
This needs to be investigated, otherwise we risk breaking native IE.
> > - READMODE_SYNC should read as much data as possible
>
> But no more than asked for? I don't see how my patch breaks this assumption.

You're right, I misread that part, sorry.

Jacek