Bug 36156 - XHR 'progress' event code assumes wrongly that expectedLength >= 0
Summary: XHR 'progress' event code assumes wrongly that expectedLength >= 0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Hans Muller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-15 22:55 PDT by Julien Chaffraix
Modified: 2011-11-30 22:38 PST (History)
5 users (show)

See Also:


Attachments
Patch (5.52 KB, patch)
2011-11-29 16:35 PST, Hans Muller
no flags Details | Formatted Diff | Diff
Patch (5.40 KB, patch)
2011-11-30 07:40 PST, Hans Muller
no flags Details | Formatted Diff | Diff
Patch (5.55 KB, patch)
2011-11-30 13:19 PST, Hans Muller
no flags Details | Formatted Diff | Diff
Patch (5.62 KB, patch)
2011-11-30 16:51 PST, Hans Muller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2010-03-15 22:55:48 PDT
The code casts the expected content length from the network stack without checking for negative length. This could cause some badness as some network stacks (such as CoreFoundation) just return -1 when they do not know the content length.

See https://bugs.webkit.org/show_bug.cgi?id=18654#c10 for the context of this bug.
Comment 1 Alexey Proskuryakov 2010-03-16 00:12:10 PDT
Answering a comment from bug 18654:

> PHP automatically fills the "Content-Length" so it is unlikely to be hit at
> this point. We will need some as-is magic to avoid sending the header.

I'm not sure about PHP, but it's certainly easy to send chunked responses with GGIs (any "slow" script in resources subdirectories does that). That is the normal use case for ling-lived requests of "comet"-style applications.
Comment 2 Glenn Maynard 2011-03-17 12:20:48 PDT
I take it this is why I'm seeing e.total == 4294967295 in XHR onprogress events from chunked sources (on 32-bit systems).  http://zewt.org/~glenn/test-webkit-xhr-progress-chunked/

According to Progress Events, "maximum" should be left unchanged when the length of the HTTP entity body is unknown, which means it retains the initial value of zero.  FF4 follows this behavior.  http://dev.w3.org/2006/webapi/progress/#firing-events-using-the-progressevent-in
Comment 3 Hans Muller 2011-11-22 12:15:52 PST
The problem is in void XMLHttpRequest::didReceiveData(const char* data, int len), here:

    if (!m_error) {
        long long expectedLength = m_response.expectedContentLength();
        m_receivedLength += len;

        if (m_async) {
            bool lengthComputable = expectedLength && m_receivedLength <= expectedLength;
            m_progressEventThrottle.dispatchProgressEvent(lengthComputable, m_receivedLength, expectedLength);
        }
        ...
     }

According to the W3C (Candidate Recommendation) ProgressEvents spec, the event's total field should be 0 if the content length can't be computed.  This happens, for example, when HTTP chunked transfer encoding is used, as in Glenn's PHP test case.

On OSX, when the content length can't be computed, m_response.expectedContentLength is -1 (this is the expected behavior from NSURLResponse).  It's being assigned to the dispatchProgressEvent() method's -unsigned- long long "total" parameter which just yields a nonsensically large value.

A defensive fix to the problem is to avoid passing any negative value as the dispatchProgressEvent's total parameter and to always use 0 when lengthComputable is false.

        if (m_async) {
            bool lengthComputable = expectedLength > 0 && m_receivedLength <= expectedLength;
            unsigned long long total = lengthComputable ? expectedLength : 0;
            m_progressEventThrottle.dispatchProgressEvent(lengthComputable, m_receivedLength, total);
        }


This produces the correct results for the Glenn's test case.  I'm working on a patch, a regression test, and checking the fix on Windows.
Comment 4 Hans Muller 2011-11-29 16:35:50 PST
Created attachment 117071 [details]
Patch
Comment 5 Alexey Proskuryakov 2011-11-29 16:44:48 PST
Comment on attachment 117071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117071&action=review

> LayoutTests/http/tests/xmlhttprequest/chunked-progress-event-expectedLength.html:26
> +        console.log(e.loaded + ", " + e.total + ", " + e.lengthComputable);

Did you intend to keep this line? It should probably use just log() for consistency.
Comment 6 Alexey Proskuryakov 2011-11-29 17:09:51 PST
Comment on attachment 117071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117071&action=review

Looks good. A few more trivial comments below.

> Source/WebCore/xml/XMLHttpRequest.cpp:1087
> -
> +        

Some of us prefer no trailing whitespace, and others don't care. Please don't just add it to lines that were not modified.

> LayoutTests/http/tests/xmlhttprequest/chunked-progress-event-expectedLength.html:33
> +	}
> +    }

There are tabs in this file. Please replace them with spaces.
Comment 7 Hans Muller 2011-11-29 17:21:06 PST
Comment on attachment 117071 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117071&action=review

>> Source/WebCore/xml/XMLHttpRequest.cpp:1087
>> +        
> 
> Some of us prefer no trailing whitespace, and others don't care. Please don't just add it to lines that were not modified.

I will remove that..

>> LayoutTests/http/tests/xmlhttprequest/chunked-progress-event-expectedLength.html:26
>> +        console.log(e.loaded + ", " + e.total + ", " + e.lengthComputable);
> 
> Did you intend to keep this line? It should probably use just log() for consistency.

I hadn't intended to include that line in the test.   I will remove it.

>> LayoutTests/http/tests/xmlhttprequest/chunked-progress-event-expectedLength.html:33
>> +    }
> 
> There are tabs in this file. Please replace them with spaces.

OK.
Comment 8 Hans Muller 2011-11-30 07:40:05 PST
Created attachment 117192 [details]
Patch
Comment 9 Alexey Proskuryakov 2011-11-30 10:51:07 PST
Comment on attachment 117192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117192&action=review

r- for the test that takes too long.

> Source/WebCore/ChangeLog:6
> +        Reviewed by Alexey Proskuryakov.

You shouldn't have put this in ChangeLog yet, since I never said r+ before.

> LayoutTests/http/tests/xmlhttprequest/chunked-progress-event-expectedLength.html:31
> +        if (e.loaded == 4 && e.total == 0 && !e.lengthComputable)
> +        {
> +            log("PASS");
> +            if (window.layoutTestController)
> +                layoutTestController.notifyDone();
> +        }

Nit: it is helpful to have detailed output for failure case, not just missing "PASS".

> LayoutTests/http/tests/xmlhttprequest/resources/chunked-transfer.php:7
> +sleep(0.5);
> +printf("4\r\n<a/>\r\n");
> +flush();
> +sleep(0.5);

Ugh.. I just noticed that this test takes a long time. Is that really needed? We generally don't want tests to run for a whole second.

Can't we check lengthComputable before state reaches 4?
Comment 10 Hans Muller 2011-11-30 12:51:17 PST
Comment on attachment 117192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117192&action=review

>> Source/WebCore/ChangeLog:6
>> +        Reviewed by Alexey Proskuryakov.
> 
> You shouldn't have put this in ChangeLog yet, since I never said r+ before.

Sorry, I'm new to the process.  I'll restore the "OOPS" boilerplate.

>> LayoutTests/http/tests/xmlhttprequest/chunked-progress-event-expectedLength.html:31
>> +        }
> 
> Nit: it is helpful to have detailed output for failure case, not just missing "PASS".

OK, the test now prints a message and the erroneous ProgressEvent total on failure.

>> LayoutTests/http/tests/xmlhttprequest/resources/chunked-transfer.php:7
>> +sleep(0.5);
> 
> Ugh.. I just noticed that this test takes a long time. Is that really needed? We generally don't want tests to run for a whole second.
> 
> Can't we check lengthComputable before state reaches 4?

Yes.  I've removed the sleep() calls.
Comment 11 Hans Muller 2011-11-30 13:19:02 PST
Created attachment 117260 [details]
Patch
Comment 12 Alexey Proskuryakov 2011-11-30 13:26:46 PST
Comment on attachment 117260 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117260&action=review

> LayoutTests/http/tests/xmlhttprequest/chunked-progress-event-expectedLength.html:34
> +        else if (e.total != 0 && !e.lengthComputable)
> +        {
> +            log("FAIL: XMLHttpRequestProgressEvent lengthComputable=false but total is non-zero: " + e.total);

This failure message is not necessarily true. It will also be printed if lengthComputable is true.

Also, this case also needs notifyDone().
Comment 13 Hans Muller 2011-11-30 16:51:39 PST
Created attachment 117296 [details]
Patch
Comment 14 Alexey Proskuryakov 2011-11-30 17:07:53 PST
Comment on attachment 117296 [details]
Patch

Please set cq? flag if you want to use commit queue (or you could ask someone to land this manually for you).
Comment 15 WebKit Review Bot 2011-11-30 22:38:18 PST
Comment on attachment 117296 [details]
Patch

Clearing flags on attachment: 117296

Committed r101612: <http://trac.webkit.org/changeset/101612>
Comment 16 WebKit Review Bot 2011-11-30 22:38:24 PST
All reviewed patches have been landed.  Closing bug.