Bug 36156

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.

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

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.

Created attachment 117071 [details] Patch

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 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 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.

Created attachment 117192 [details] Patch

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 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.

Created attachment 117260 [details] Patch

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().

Created attachment 117296 [details] Patch

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 on attachment 117296 [details] Patch Clearing flags on attachment: 117296 Committed r101612: <http://trac.webkit.org/changeset/101612>

All reviewed patches have been landed. Closing bug.