Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/#review52842 --- This review has been submitted with commit 3d857d2171d4182954a2f4e90a2bc9a862b5c560 by Andrea Iacovitti to branch master. - Commit Hook On Feb. 17, 2014, 5:29 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 17, 2014, 5:29 p.m.) Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/#review50079 --- This review has been submitted with commit 24d780debd65e5c2e957a0ab3dd1ba5b9885a42b by Dawit Alemayehu to branch KDE/4.12. - Commit Hook On Feb. 16, 2014, 7:10 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 16, 2014, 7:10 p.m.) Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 17, 2014, 5:29 p.m.) Status -- This change has been marked as submitted. Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
On Feb. 16, 2014, 12:38 a.m., Andrea Iacovitti wrote: khtml/ecma/xmlhttprequest.cpp, line 512 https://git.reviewboard.kde.org/r/115689/diff/2/?file=244040#file244040line512 This does not seems to fix the POST-POST redirection with no content on 307 response issue, instead it has the side effect that POST method is rewritten to GET in 307 redirection.Also it doesn't make possible to do an XHR PUT request with content (!_body.isEmpty()) as actually a POST request is sent to the origin server and not a PUT. We use KIO::http_post (regardless of method) whenever we need to send a request that includes content, even for methods that do not have a defined semantics for entity-body (e.g. DELETE). So setting custom method it's always needed. The reason a POST method is rewritten to GET in a 307 redirection has to do with how we originally chose to deal with POST redirection in KIO::TransferJob. Right now all POST redirections are converted to GET regardless of the HTTP status code in TransferJob::slotFinished in job.cpp. See the CMD_SPECIAL section of the switch statement in that function. As far as then issue with the PUT request and an entity-body, kio_http was designed only to send the entity-body to the server for POST and other webdav requests. Anything that is done to skirt around that to me is a hack by definition. What would be the purpose of allowing entity-body for methods that are not supposed to have such thing, e.g. DELETE? Anyhow, I do not want to cause regression ; so I will roll back this patch to the way things were. However, those POST-POST redirection tests it seems to pass are wrong because it never sends the entity-body to the redirected resource. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/#review49889 --- On Feb. 14, 2014, 4 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 14, 2014, 4 p.m.) Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 16, 2014, 7:10 p.m.) Review request for kdelibs and Andrea Iacovitti. Changes --- Always send the CustomHTTPmethod meta data. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs (updated) - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/#review49994 --- Ship it! - Andrea Iacovitti On Feb. 16, 2014, 7:10 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 16, 2014, 7:10 p.m.) Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115689/#review49890
---
Because of CustomHTTPMethod i think changes are also needed in kio_http's POST
to GET redirection code:
--- kdelibs/kioslave/http/http.cpp
+++ kdelibs/kioslave/http/http.cpp
@@ -3099,16 +3099,17 @@
// this way of doing things! Thus, we are forced to do the same
// thing here. Otherwise, we loose compatibility and might not be
// able to correctly retrieve sites that redirect.
+const QByteArray method = m_request.methodString();
switch (m_request.responseCode) {
case 301: // Moved Permanently
setMetaData(QLatin1String(permanent-redirect),
QLatin1String(true));
-if (m_request.method == HTTP_POST) {
+if (method == POST ) {
m_request.method = HTTP_GET; // FORCE a GET
setMetaData(QLatin1String(redirect-to-get),
QLatin1String(true));
}
break;
case 302: // Found
-if (m_request.method == HTTP_POST) {
+if (method == POST ) {
m_request.method = HTTP_GET; // FORCE a GET
setMetaData(QLatin1String(redirect-to-get),
QLatin1String(true));
}
- Andrea Iacovitti
On Feb. 14, 2014, 4 p.m., Dawit Alemayehu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115689/
---
(Updated Feb. 14, 2014, 4 p.m.)
Review request for kdelibs and Andrea Iacovitti.
Bugs: 331007
http://bugs.kde.org/show_bug.cgi?id=331007
Repository: kdelibs
Description
---
Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests
without a noticable delay, i.e. use KIO::mimeType instead of KIO::get.
Otherwise, the request will wait for the content which is not returned when
doing a stat operation like HEAD.
Diffs
-
khtml/ecma/xmlhttprequest.cpp b3707e7
kio/kio/job.cpp abb3dfd
Diff: https://git.reviewboard.kde.org/r/115689/diff/
Testing
---
Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/
Thanks,
Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 14, 2014, 4 p.m.) Review request for kdelibs and Andrea Iacovitti. Changes --- Updated the patch because I discovered more issues with the way khtml does its xmlhttp requests. Andrea please review and see if you have any objections. As stated in my comments, setting a custom HTTP header blindly has unintended consequences. It pretends to correctly redirect a POST request to another POST request for 307/308 HTTP response codes when it really does not. IOW, it simply changes the method from GET to POST, because of the presence of the custom HTTP method, but does NOT send any content to the redirected resource like a real POST-POST redirection should. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs (updated) - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/kio/job.cpp abb3dfd Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 12, 2014, 9:56 a.m.) Review request for kdelibs and Andrea Iacovitti. Changes --- Uploaded the patch. Please note that the relevant patch to this pull request is the one that changes khtml/ecma/xmlhttprequest.cpp and adds CMD_STAT to TransferJob::slotFinished's switch statement. Unfortunately, this patch depends on another one that is currently in review, https://git.reviewboard.kde.org/r/115651/, hence the posted patch here contains the changes from that review request as well. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs (updated) - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/DESIGN.metadata 1351119 kio/kio/accessmanager.cpp 7a806e8 kio/kio/job.cpp 13107c2 kioslave/http/http.cpp b13eed1 Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
Am 12.02.2014 10:56, schrieb Dawit Alemayehu: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 12, 2014, 9:56 a.m.) Review request for kdelibs and Andrea Iacovitti. Changes --- Uploaded the patch. It looks to me as if the patch also contains the stuff for RR 115651. Eike
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/#review49671 --- Ship it! Looks good from my side. I added some comments in https://git.reviewboard.kde.org/r/115651 about http.cpp's changes. - Andrea Iacovitti On Feb. 12, 2014, 9:56 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 12, 2014, 9:56 a.m.) Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - khtml/ecma/xmlhttprequest.cpp b3707e7 kio/DESIGN.metadata 1351119 kio/kio/accessmanager.cpp 7a806e8 kio/kio/job.cpp 13107c2 kioslave/http/http.cpp b13eed1 Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
Re: Review Request 115689: Fix khtml/ecma/xmlhttprequest.cpp to properly handle HEAD requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/#review49621 --- no diff? - Bernd Buschinski On Feb. 12, 2014, 4:29 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115689/ --- (Updated Feb. 12, 2014, 4:29 a.m.) Review request for kdelibs and Andrea Iacovitti. Bugs: 331007 http://bugs.kde.org/show_bug.cgi?id=331007 Repository: kdelibs Description --- Fix khtml/ecma/xmlttprequest.cpp such that it correctly handles HEAD requests without a noticable delay, i.e. use KIO::mimeType instead of KIO::get. Otherwise, the request will wait for the content which is not returned when doing a stat operation like HEAD. Diffs - Diff: https://git.reviewboard.kde.org/r/115689/diff/ Testing --- Tested HEAD redirection with http://greenbytes.de/tech/tc/httpredirects/ Thanks, Dawit Alemayehu
