Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
39203004:
[Net] Fix error handling on wrong file in UploadData (Closed)
|
Created:
7 years, 2 months ago by tzik Modified:
7 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org, manoranjanr, kinuko+watch, tommi (sloooow) - chröme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Net] Fix error handling on wrong file in UploadData
* Handle file error as a failure rather than handling it as empty file.
* Change Chrome Frame error handling for URLRequest failure, from DCHECK hit to ignoring failing input file.
BUG=310548, 317432
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234436
Patch Set 1 #Patch Set 2 : fix UploadFileElementReaderSyncTest.WrongPath expectation #Patch Set 3 : test fix #Patch Set 4 : comment fix #Patch Set 5 : +SPDY test #Patch Set 6 : win test fix #
Total comments: 11
Patch Set 7 : #
Total comments: 6
Patch Set 8 : #Patch Set 9 : #
Total comments: 6
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : chrome_frame_fix #Patch Set 13 : fix test expectation for chrome_frame #Patch Set 14 : #Patch Set 15 : buildfix #Patch Set 16 : expectation fix #
Total comments: 10
Patch Set 17 : #Patch Set 18 : #
Total comments: 2
Patch Set 19 : #Messages
Total messages: 35 (0 generated)
PTL.
Passing hot potato to Matt :)
On 2013/10/24 13:17:03, willchan wrote: > Passing hot potato to Matt :) Hmm, is it intended behavior? A test comment says. // If we try to upload an unreadable file, the network stack should report // the file size as zero and upload zero bytes for that file. https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw...
On 2013/10/24 13:55:08, tzik wrote: > On 2013/10/24 13:17:03, willchan wrote: > > Passing hot potato to Matt :) > > Hmm, is it intended behavior? You mean passing hot potatoes to me? Yes, I have the scars to prove it! > A test comment says. > // If we try to upload an unreadable file, the network stack should report > // the file size as zero and upload zero bytes for that file. > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... I'd like to see if we can get some history of the behavior before landing this - it is clearly expected behavior from the old comment, but I'd like to figure out if there's a reason for it. I'm supportive of the the CL, but there may be real reasons for the behavior. Probably also a good idea to look at what other browsers do. I'd also like unit tests for this: Both an upload unittest of some sort, and some at a higher level - one making sure we both return the error and don't try to reuse the socket. We should also have a SPDY test for this. If we already have a SPDY test checking the case where InitInternal fails, I'm not as concerned about the SPDY test, but should probably still have an HTTP one.
After applying the patch, I can still reproduce the issue with http://www.speedyshare.com/
On 2013/10/24 14:37:25, mmenke wrote: > On 2013/10/24 13:55:08, tzik wrote: > > On 2013/10/24 13:17:03, willchan wrote: > > > Passing hot potato to Matt :) > > > > Hmm, is it intended behavior? > > You mean passing hot potatoes to me? Yes, I have the scars to prove it! > I hope I can make it cool! > > A test comment says. > > // If we try to upload an unreadable file, the network stack should report > > // the file size as zero and upload zero bytes for that file. > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... > > I'd like to see if we can get some history of the behavior before landing this - > it is clearly expected behavior from the old comment, but I'd like to figure out > if there's a reason for it. I'm supportive of the the CL, but there may be real > reasons for the behavior. Probably also a good idea to look at what other > browsers do. > Referring related issue, this is introduced as a bugfix. Chromium seems to have failed on this not-readable file. http://crbug.com/30850 For other browser behavior: Internet Explorer 10 on Windows 7 sends the file content event after the permission is dropped. Firefox 24 on Windows 7 and Gentoo Linux doesn't send request to the server, the request job is aborted after connection. And Chromium with this patch behaves the same as Firefox. According to the issue, at that time Firefox used to send empty file on non-readable file. > I'd also like unit tests for this: Both an upload unittest of some sort, and > some at a higher level - one making sure we both return the error and don't try > to reuse the socket. We should also have a SPDY test for this. If we already > have a SPDY test checking the case where InitInternal fails, I'm not as > concerned about the SPDY test, but should probably still have an HTTP one. Let me look into.
On 2013/10/24 15:13:01, Michał Górny wrote: > After applying the patch, I can still reproduce the issue with > http://www.speedyshare.com/ This CL does not affect the call path for the site. The site uses FileReader to read file locally and send it as XHR. And ignores errors on file reading around line 1042 of view-source:http://www17.speedyshare.com/upload_page.php
On 2013/10/25 02:28:29, tzik wrote: > On 2013/10/24 14:37:25, mmenke wrote: > > On 2013/10/24 13:55:08, tzik wrote: > > > On 2013/10/24 13:17:03, willchan wrote: > > > > Passing hot potato to Matt :) > > > > > > Hmm, is it intended behavior? > > > > You mean passing hot potatoes to me? Yes, I have the scars to prove it! > > > I hope I can make it cool! > > > > A test comment says. > > > // If we try to upload an unreadable file, the network stack should report > > > // the file size as zero and upload zero bytes for that file. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... > > > > I'd like to see if we can get some history of the behavior before landing this > - > > it is clearly expected behavior from the old comment, but I'd like to figure > out > > if there's a reason for it. I'm supportive of the the CL, but there may be > real > > reasons for the behavior. Probably also a good idea to look at what other > > browsers do. > > > Referring related issue, this is introduced as a bugfix. > Chromium seems to have failed on this not-readable file. > http://crbug.com/30850 > > For other browser behavior: > Internet Explorer 10 on Windows 7 sends the file content event after the > permission is dropped. > Firefox 24 on Windows 7 and Gentoo Linux doesn't send request to the server, the > request job is aborted after connection. > And Chromium with this patch behaves the same as Firefox. > > According to the issue, at that time Firefox used to send empty file on > non-readable file. Thanks for looking up the bug. Sounds like no compelling reason to keep the current behavior, given that this is no longer what FF does, and not what IE does. > > I'd also like unit tests for this: Both an upload unittest of some sort, and > > some at a higher level - one making sure we both return the error and don't > try > > to reuse the socket. We should also have a SPDY test for this. If we already > > have a SPDY test checking the case where InitInternal fails, I'm not as > > concerned about the SPDY test, but should probably still have an HTTP one. > > Let me look into. I hadn't noticed the test changes when I wrote that comment. Think we have the upload tests covered, since we both test failing to open a file and the general case of an UploadDataStream failing during init. We could add a test where open succeeds but seek fails, if that's easy to do, but I'm not terribly concerned about that case. We do have HttpNetworkTransactionTest.UploadUnreadableFile, which is failing now (As it should). So it looks like once that test is fixed (And another similar one), my only remaining concern is a SPDY test for this.
On 2013/10/25 15:50:21, mmenke wrote: > On 2013/10/25 02:28:29, tzik wrote: > > On 2013/10/24 14:37:25, mmenke wrote: > > > On 2013/10/24 13:55:08, tzik wrote: > > > > On 2013/10/24 13:17:03, willchan wrote: > > > > > Passing hot potato to Matt :) > > > > > > > > Hmm, is it intended behavior? > > > > > > You mean passing hot potatoes to me? Yes, I have the scars to prove it! > > > > > I hope I can make it cool! > > > > > > A test comment says. > > > > // If we try to upload an unreadable file, the network stack should report > > > > // the file size as zero and upload zero bytes for that file. > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... > > > > > > I'd like to see if we can get some history of the behavior before landing > this > > - > > > it is clearly expected behavior from the old comment, but I'd like to figure > > out > > > if there's a reason for it. I'm supportive of the the CL, but there may be > > real > > > reasons for the behavior. Probably also a good idea to look at what other > > > browsers do. > > > > > Referring related issue, this is introduced as a bugfix. > > Chromium seems to have failed on this not-readable file. > > http://crbug.com/30850 > > > > For other browser behavior: > > Internet Explorer 10 on Windows 7 sends the file content event after the > > permission is dropped. > > Firefox 24 on Windows 7 and Gentoo Linux doesn't send request to the server, > the > > request job is aborted after connection. > > And Chromium with this patch behaves the same as Firefox. > > > > According to the issue, at that time Firefox used to send empty file on > > non-readable file. > > Thanks for looking up the bug. Sounds like no compelling reason to keep the > current behavior, given that this is no longer what FF does, and not what IE > does. > > > > I'd also like unit tests for this: Both an upload unittest of some sort, > and > > > some at a higher level - one making sure we both return the error and don't > > try > > > to reuse the socket. We should also have a SPDY test for this. If we > already > > > have a SPDY test checking the case where InitInternal fails, I'm not as > > > concerned about the SPDY test, but should probably still have an HTTP one. > > > > Let me look into. > > I hadn't noticed the test changes when I wrote that comment. Think we have the > upload tests covered, since we both test failing to open a file and the general > case of an UploadDataStream failing during init. We could add a test where open > succeeds but seek fails, if that's easy to do, but I'm not terribly concerned > about that case. > > We do have HttpNetworkTransactionTest.UploadUnreadableFile, which is failing now > (As it should). > > So it looks like once that test is fixed (And another similar one), my only > remaining concern is a SPDY test for this. OK, I fixed the failed tests and added a SPDY test! Please take another look, the potato is now frozen.
On 2013/10/28 12:00:56, tzik wrote: > On 2013/10/25 15:50:21, mmenke wrote: > > On 2013/10/25 02:28:29, tzik wrote: > > > On 2013/10/24 14:37:25, mmenke wrote: > > > > On 2013/10/24 13:55:08, tzik wrote: > > > > > On 2013/10/24 13:17:03, willchan wrote: > > > > > > Passing hot potato to Matt :) > > > > > > > > > > Hmm, is it intended behavior? > > > > > > > > You mean passing hot potatoes to me? Yes, I have the scars to prove it! > > > > > > > I hope I can make it cool! > > > > > > > > A test comment says. > > > > > // If we try to upload an unreadable file, the network stack should > report > > > > > // the file size as zero and upload zero bytes for that file. > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/http/http_netw... > > > > > > > > I'd like to see if we can get some history of the behavior before landing > > this > > > - > > > > it is clearly expected behavior from the old comment, but I'd like to > figure > > > out > > > > if there's a reason for it. I'm supportive of the the CL, but there may > be > > > real > > > > reasons for the behavior. Probably also a good idea to look at what other > > > > browsers do. > > > > > > > Referring related issue, this is introduced as a bugfix. > > > Chromium seems to have failed on this not-readable file. > > > http://crbug.com/30850 > > > > > > For other browser behavior: > > > Internet Explorer 10 on Windows 7 sends the file content event after the > > > permission is dropped. > > > Firefox 24 on Windows 7 and Gentoo Linux doesn't send request to the server, > > the > > > request job is aborted after connection. > > > And Chromium with this patch behaves the same as Firefox. > > > > > > According to the issue, at that time Firefox used to send empty file on > > > non-readable file. > > > > Thanks for looking up the bug. Sounds like no compelling reason to keep the > > current behavior, given that this is no longer what FF does, and not what IE > > does. > > > > > > I'd also like unit tests for this: Both an upload unittest of some sort, > > and > > > > some at a higher level - one making sure we both return the error and > don't > > > try > > > > to reuse the socket. We should also have a SPDY test for this. If we > > already > > > > have a SPDY test checking the case where InitInternal fails, I'm not as > > > > concerned about the SPDY test, but should probably still have an HTTP one. > > > > > > Let me look into. > > > > I hadn't noticed the test changes when I wrote that comment. Think we have > the > > upload tests covered, since we both test failing to open a file and the > general > > case of an UploadDataStream failing during init. We could add a test where > open > > succeeds but seek fails, if that's easy to do, but I'm not terribly concerned > > about that case. > > > > We do have HttpNetworkTransactionTest.UploadUnreadableFile, which is failing > now > > (As it should). > > > > So it looks like once that test is fixed (And another similar one), my only > > remaining concern is a SPDY test for this. > > OK, I fixed the failed tests and added a SPDY test! > Please take another look, the potato is now frozen. I'm heavily loaded with reviews and have an interview today as well, so may not get to this today.
Some drive-by comments... https://codereview.chromium.org/39203004/diff/390001/net/base/upload_file_ele... File net/base/upload_file_element_reader.cc (right): https://codereview.chromium.org/39203004/diff/390001/net/base/upload_file_ele... net/base/upload_file_element_reader.cc:38: // If the file can't be opened, we'll just upload an empty file. This comment is obsolete. https://codereview.chromium.org/39203004/diff/390001/net/base/upload_file_ele... net/base/upload_file_element_reader.cc:52: if (file_util::GetFileSize(path, &length) && Shouldn't we return an error when GetFileSize fails? The same goes for GetFileInfo below. https://codereview.chromium.org/39203004/diff/390001/net/spdy/spdy_network_tr... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/390001/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_unittest.cc:1821: scoped_ptr<SpdyFrame> body(spdy_util_.ConstructSpdyBodyFrame(1, true)); Do we need |req| and |body|? https://codereview.chromium.org/39203004/diff/390001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/39203004/diff/390001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:4531: EXPECT_EQ(0, d.bytes_received()); How about having checks for r.status().status() and r.status().error()?
https://codereview.chromium.org/39203004/diff/390001/net/base/upload_file_ele... File net/base/upload_file_element_reader.cc (right): https://codereview.chromium.org/39203004/diff/390001/net/base/upload_file_ele... net/base/upload_file_element_reader.cc:38: // If the file can't be opened, we'll just upload an empty file. On 2013/10/29 04:08:59, hashimoto wrote: > This comment is obsolete. Done. https://codereview.chromium.org/39203004/diff/390001/net/base/upload_file_ele... net/base/upload_file_element_reader.cc:52: if (file_util::GetFileSize(path, &length) && On 2013/10/29 04:08:59, hashimoto wrote: > Shouldn't we return an error when GetFileSize fails? > The same goes for GetFileInfo below. Done. https://codereview.chromium.org/39203004/diff/390001/net/spdy/spdy_network_tr... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/390001/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_unittest.cc:1821: scoped_ptr<SpdyFrame> body(spdy_util_.ConstructSpdyBodyFrame(1, true)); On 2013/10/29 04:08:59, hashimoto wrote: > Do we need |req| and |body|? Hmm? Let me reconsider... This test doesn't look meaningful.
https://codereview.chromium.org/39203004/diff/390001/net/spdy/spdy_network_tr... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/390001/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_unittest.cc:1821: scoped_ptr<SpdyFrame> body(spdy_util_.ConstructSpdyBodyFrame(1, true)); On 2013/10/29 05:48:02, tzik wrote: > On 2013/10/29 04:08:59, hashimoto wrote: > > Do we need |req| and |body|? > > Hmm? Let me reconsider... This test doesn't look meaningful. > OK, I updated the test. https://codereview.chromium.org/39203004/diff/390001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/39203004/diff/390001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:4531: EXPECT_EQ(0, d.bytes_received()); On 2013/10/29 04:08:59, hashimoto wrote: > How about having checks for r.status().status() and r.status().error()? The request should be aborted before it set status(). So status().status() and status().error() are SUCCESS/OK as they were initialized. I'm not sure we should check them.
https://codereview.chromium.org/39203004/diff/390001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/39203004/diff/390001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:4531: EXPECT_EQ(0, d.bytes_received()); On 2013/10/29 06:06:19, tzik wrote: > On 2013/10/29 04:08:59, hashimoto wrote: > > How about having checks for r.status().status() and r.status().error()? > > The request should be aborted before it set status(). So status().status() and > status().error() are SUCCESS/OK as they were initialized. > I'm not sure we should check them. Really? In my local environment, FAILED/ERR_FILE_NOT_FOUND is set with the calling stack below. [7868:7868:1029/154135:3022819135420:ERROR:url_request.h(690)] [0x7f575a0b0a2e] base::debug::StackTrace::StackTrace() [0x7f5759c404e6] net::URLRequest::set_status() [0x7f5759c3f9c4] net::URLRequestJob::NotifyStartError() [0x7f5759c31e67] net::URLRequestHttpJob::OnStartCompleted() [0x7f5759c3a639] base::internal::RunnableAdapter<>::Run() [0x7f5759c3c334] base::internal::InvokeHelper<>::MakeItSo() [0x7f5759c3c2c2] base::internal::Invoker<>::Run() [0x7f5759694256] base::Callback<>::Run() [0x7f5759917260] net::HttpCache::Transaction::DoCallback() [0x7f5759917356] net::HttpCache::Transaction::HandleResult() [0x7f57599144b5] net::HttpCache::Transaction::DoLoop() [0x7f5759912c3b] net::HttpCache::Transaction::OnIOComplete() [0x7f5759923669] base::internal::RunnableAdapter<>::Run() [0x7f5759923568] base::internal::InvokeHelper<>::MakeItSo() [0x7f57599234ed] base::internal::Invoker<>::Run() [0x7f5759694256] base::Callback<>::Run() [0x7f575992ec22] net::HttpNetworkTransaction::DoCallback() [0x7f575992b39b] net::HttpNetworkTransaction::OnIOComplete() [0x7f5759935db9] base::internal::RunnableAdapter<>::Run() [0x7f5759935d14] base::internal::InvokeHelper<>::MakeItSo() [0x7f5759935ca2] base::internal::Invoker<>::Run() [0x7f5759694256] base::Callback<>::Run() [0x7f575971048b] net::UploadDataStream::ResumePendingInit() [0x7f57597121e3] base::internal::RunnableAdapter<>::Run() [0x7f5759712116] base::internal::InvokeHelper<>::MakeItSo() [0x7f575971206b] base::internal::Invoker<>::Run() [0x7f5759694256] base::Callback<>::Run() [0x7f57597148cb] net::UploadFileElementReader::OnInitCompleted() https://codereview.chromium.org/39203004/diff/540001/net/base/upload_file_ele... File net/base/upload_file_element_reader.cc (right): https://codereview.chromium.org/39203004/diff/540001/net/base/upload_file_ele... net/base/upload_file_element_reader.cc:70: if (file_util::GetFileInfo(path, &info) && Also this GetFileInfo()? https://codereview.chromium.org/39203004/diff/540001/net/spdy/spdy_network_tr... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/540001/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_unittest.cc:1835: TestCompletionCallback callback; I'm not familiar with the code in this file, but is there any reason we cannot use RunDefaultTest() instead of directly calling Start()? Is it OK to omit the CloseCurrentSessions() call in FinishDefaultTest()? https://codereview.chromium.org/39203004/diff/540001/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_unittest.cc:1842: helper.VerifyDataNotConsumed(); How about doing some verification against helper.output()?
https://codereview.chromium.org/39203004/diff/670001/net/base/upload_file_ele... File net/base/upload_file_element_reader.cc (right): https://codereview.chromium.org/39203004/diff/670001/net/base/upload_file_ele... net/base/upload_file_element_reader.cc:72: return ERR_FILE_NOT_FOUND; I suggest not setting out_content_length or out_file_stream when we return either of these errors - I prefer to have the invariant "We either set them to be non-NULL, or return an error". https://codereview.chromium.org/39203004/diff/670001/net/http/http_network_tr... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/670001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:7740: "Content-Length: 0\r\n\r\n"), Do we need all this stuff? I thought we ended up not writing anything to the socket in this case? If that's the case, can get rid of all this stuff and pass in 0's and NULLs to the StaticSocketDataProvider (Still need it the StaticSocketDataProvider , I believe, since we still establish a connection). Same goes for the SPDY tests. https://codereview.chromium.org/39203004/diff/670001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:7810: temp_file_contents.length()), Again, don't think we need the second block of data. The socket should actually be closed. Same goes for the reads.
Sorry for my slow update, but I'm still fixing a trybot failure on chrome_frame_net_tests. Debugging chrome frame is too painful... https://codereview.chromium.org/39203004/diff/390001/net/url_request/url_requ... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/39203004/diff/390001/net/url_request/url_requ... net/url_request/url_request_unittest.cc:4531: EXPECT_EQ(0, d.bytes_received()); On 2013/10/29 06:55:26, hashimoto wrote: > On 2013/10/29 06:06:19, tzik wrote: > > On 2013/10/29 04:08:59, hashimoto wrote: > > > How about having checks for r.status().status() and r.status().error()? > > > > The request should be aborted before it set status(). So status().status() and > > status().error() are SUCCESS/OK as they were initialized. > > I'm not sure we should check them. > > Really? In my local environment, FAILED/ERR_FILE_NOT_FOUND is set with the > calling stack below. > [7868:7868:1029/154135:3022819135420:ERROR:url_request.h(690)] [0x7f575a0b0a2e] > base::debug::StackTrace::StackTrace() > [0x7f5759c404e6] net::URLRequest::set_status() > [0x7f5759c3f9c4] net::URLRequestJob::NotifyStartError() > [0x7f5759c31e67] net::URLRequestHttpJob::OnStartCompleted() > [0x7f5759c3a639] base::internal::RunnableAdapter<>::Run() > [0x7f5759c3c334] base::internal::InvokeHelper<>::MakeItSo() > [0x7f5759c3c2c2] base::internal::Invoker<>::Run() > [0x7f5759694256] base::Callback<>::Run() > [0x7f5759917260] net::HttpCache::Transaction::DoCallback() > [0x7f5759917356] net::HttpCache::Transaction::HandleResult() > [0x7f57599144b5] net::HttpCache::Transaction::DoLoop() > [0x7f5759912c3b] net::HttpCache::Transaction::OnIOComplete() > [0x7f5759923669] base::internal::RunnableAdapter<>::Run() > [0x7f5759923568] base::internal::InvokeHelper<>::MakeItSo() > [0x7f57599234ed] base::internal::Invoker<>::Run() > [0x7f5759694256] base::Callback<>::Run() > [0x7f575992ec22] net::HttpNetworkTransaction::DoCallback() > [0x7f575992b39b] net::HttpNetworkTransaction::OnIOComplete() > [0x7f5759935db9] base::internal::RunnableAdapter<>::Run() > [0x7f5759935d14] base::internal::InvokeHelper<>::MakeItSo() > [0x7f5759935ca2] base::internal::Invoker<>::Run() > [0x7f5759694256] base::Callback<>::Run() > [0x7f575971048b] net::UploadDataStream::ResumePendingInit() > [0x7f57597121e3] base::internal::RunnableAdapter<>::Run() > [0x7f5759712116] base::internal::InvokeHelper<>::MakeItSo() > [0x7f575971206b] base::internal::Invoker<>::Run() > [0x7f5759694256] base::Callback<>::Run() > [0x7f57597148cb] net::UploadFileElementReader::OnInitCompleted() Sorry, right it is. My build might be from another branch when I tried it. Fixed. https://codereview.chromium.org/39203004/diff/540001/net/base/upload_file_ele... File net/base/upload_file_element_reader.cc (right): https://codereview.chromium.org/39203004/diff/540001/net/base/upload_file_ele... net/base/upload_file_element_reader.cc:70: if (file_util::GetFileInfo(path, &info) && On 2013/10/29 06:55:26, hashimoto wrote: > Also this GetFileInfo()? Done. https://codereview.chromium.org/39203004/diff/540001/net/spdy/spdy_network_tr... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/540001/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_unittest.cc:1842: helper.VerifyDataNotConsumed(); On 2013/10/29 06:55:26, hashimoto wrote: > How about doing some verification against helper.output()? Done.
On 2013/10/31 12:19:28, tzik wrote: > Sorry for my slow update, but I'm still fixing a trybot failure on > chrome_frame_net_tests. > > Debugging chrome frame is too painful... Support for ChromeFrame ends fairly soon (January?). It may be fine to disable the test on ChromeFrame. However, I'd like to know exactly why it fails before doing so, to make sure we aren't papering over a real bug in the ChromeFrame case.
On 2013/10/31 14:53:02, mmenke wrote: > On 2013/10/31 12:19:28, tzik wrote: > > Sorry for my slow update, but I'm still fixing a trybot failure on > > chrome_frame_net_tests. > > > > Debugging chrome frame is too painful... > > Support for ChromeFrame ends fairly soon (January?). It may be fine to disable > the test on ChromeFrame. However, I'd like to know exactly why it fails before > doing so, to make sure we aren't papering over a real bug in the ChromeFrame > case. I think Chrome Frame tests are failing because UrlmonUploadDataStream, a data reader class used by CF, lacks the capability to handle initialization errors. (see https://code.google.com/p/chromium/codesearch#chromium/src/chrome_frame/urlmo...) Considering the CF's limited life expectancy, implementing a correct error handling would not be worth doing. How about implementing quick hacks like: 1. In UrlmonUploadDataStream::Initialize, replace the failing UploadDataStream with an empty UploadDataStream. 2. In PluginUrlRequest::Initialize, behave as if there was no upload data provided when UploadDataStream::Initialize fails. Thoughts?
On 2013/11/05 06:04:42, hashimoto wrote: > On 2013/10/31 14:53:02, mmenke wrote: > > On 2013/10/31 12:19:28, tzik wrote: > > > Sorry for my slow update, but I'm still fixing a trybot failure on > > > chrome_frame_net_tests. > > > > > > Debugging chrome frame is too painful... > > > > Support for ChromeFrame ends fairly soon (January?). It may be fine to > disable > > the test on ChromeFrame. However, I'd like to know exactly why it fails > before > > doing so, to make sure we aren't papering over a real bug in the ChromeFrame > > case. > > I think Chrome Frame tests are failing because UrlmonUploadDataStream, a data > reader class used by CF, lacks the capability to handle initialization errors. > (see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome_frame/urlmo...) > > Considering the CF's limited life expectancy, implementing a correct error > handling would not be worth doing. > How about implementing quick hacks like: > 1. In UrlmonUploadDataStream::Initialize, replace the failing UploadDataStream > with an empty UploadDataStream. > 2. In PluginUrlRequest::Initialize, behave as if there was no upload data > provided when UploadDataStream::Initialize fails. > Thoughts? Sorry, forgot about this. I'll look at those tomorrow.
Now, tests turned saner. Please take another look. Adding tommi as a chrome_frame OWNER. tommi: I'm making a bit dirty workaround for chrome_frame in this CL. Is it acceptable? On 2013/11/05 06:04:42, hashimoto wrote: >Considering the CF's limited life expectancy, implementing a correct error >handling would not be worth doing. >How about implementing quick hacks like: >1. In UrlmonUploadDataStream::Initialize, replace the failing UploadDataStream >with an empty UploadDataStream. >2. In PluginUrlRequest::Initialize, behave as if there was no upload data >provided when UploadDataStream::Initialize fails. >Thoughts? Thanks for investigation. I tried to fix like 2. that seems working. https://codereview.chromium.org/39203004/diff/540001/net/spdy/spdy_network_tr... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/540001/net/spdy/spdy_network_tr... net/spdy/spdy_network_transaction_unittest.cc:1835: TestCompletionCallback callback; On 2013/10/29 06:55:26, hashimoto wrote: > I'm not familiar with the code in this file, but is there any reason we cannot > use RunDefaultTest() instead of directly calling Start()? > Is it OK to omit the CloseCurrentSessions() call in FinishDefaultTest()? Ah, I though it expects the transaction complete successful. Done. https://codereview.chromium.org/39203004/diff/670001/net/base/upload_file_ele... File net/base/upload_file_element_reader.cc (right): https://codereview.chromium.org/39203004/diff/670001/net/base/upload_file_ele... net/base/upload_file_element_reader.cc:72: return ERR_FILE_NOT_FOUND; On 2013/10/29 15:29:31, mmenke wrote: > I suggest not setting out_content_length or out_file_stream when we return > either of these errors - I prefer to have the invariant "We either set them to > be non-NULL, or return an error". Done. https://codereview.chromium.org/39203004/diff/670001/net/http/http_network_tr... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/670001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:7740: "Content-Length: 0\r\n\r\n"), On 2013/10/29 15:29:31, mmenke wrote: > Do we need all this stuff? I thought we ended up not writing anything to the > socket in this case? If that's the case, can get rid of all this stuff and pass > in 0's and NULLs to the StaticSocketDataProvider (Still need it the > StaticSocketDataProvider , I believe, since we still establish a connection). > > Same goes for the SPDY tests. Done. https://codereview.chromium.org/39203004/diff/670001/net/http/http_network_tr... net/http/http_network_transaction_unittest.cc:7810: temp_file_contents.length()), On 2013/10/29 15:29:31, mmenke wrote: > Again, don't think we need the second block of data. The socket should actually > be closed. Same goes for the reads. Well, since the request should be aborted before sending any request to the server, IMO, this test is no longer valid. Let me drop this.
Just one comment. Otherwise, looks great! https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... net/spdy/spdy_network_transaction_unittest.cc:1822: CreateMockWrite(*req), I believe this can just be NULL, too? Since we can't get the file size, we won't be able to send the request.
https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... net/spdy/spdy_network_transaction_unittest.cc:1822: CreateMockWrite(*req), On 2013/11/07 20:29:00, mmenke wrote: > I believe this can just be NULL, too? Since we can't get the file size, we > won't be able to send the request. (To make it NULL, best to just get rid of writes completely, and pass NULL, 0 into the DelayedSocketData) https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... net/spdy/spdy_network_transaction_unittest.cc:1828: DelayedSocketData data(2, reads, arraysize(reads), writes, arraysize(writes)); I believe this can just be a StaticSocketDataProvider now.
https://codereview.chromium.org/39203004/diff/1220001/chrome_frame/plugin_url... File chrome_frame/plugin_url_request.cc (right): https://codereview.chromium.org/39203004/diff/1220001/chrome_frame/plugin_url... chrome_frame/plugin_url_request.cc:49: return true; Is it OK to return without setting enable_frame_busting_? https://codereview.chromium.org/39203004/diff/1220001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/39203004/diff/1220001/net/url_request/url_req... net/url_request/url_request_unittest.cc:4709: #if defined(CHROME_FRAME_NET_TESTS) How about having a TODO to remove this "if defined"? (or a comment to describe the reason for this difference)
https://codereview.chromium.org/39203004/diff/1220001/chrome_frame/plugin_url... File chrome_frame/plugin_url_request.cc (right): https://codereview.chromium.org/39203004/diff/1220001/chrome_frame/plugin_url... chrome_frame/plugin_url_request.cc:49: return true; On 2013/11/08 04:59:08, hashimoto wrote: > Is it OK to return without setting enable_frame_busting_? Done. https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... net/spdy/spdy_network_transaction_unittest.cc:1822: CreateMockWrite(*req), On 2013/11/07 20:29:00, mmenke wrote: > I believe this can just be NULL, too? Since we can't get the file size, we > won't be able to send the request. Done. https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... net/spdy/spdy_network_transaction_unittest.cc:1822: CreateMockWrite(*req), On 2013/11/07 21:55:51, mmenke wrote: > On 2013/11/07 20:29:00, mmenke wrote: > > I believe this can just be NULL, too? Since we can't get the file size, we > > won't be able to send the request. > > (To make it NULL, best to just get rid of writes completely, and pass NULL, 0 > into the DelayedSocketData) Passing empty writes and using StaticSocketDataProvider seems to cause hang. Can I just replace req to empty MockWrite and keep others as is? https://codereview.chromium.org/39203004/diff/1220001/net/url_request/url_req... File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/39203004/diff/1220001/net/url_request/url_req... net/url_request/url_request_unittest.cc:4709: #if defined(CHROME_FRAME_NET_TESTS) On 2013/11/08 04:59:08, hashimoto wrote: > How about having a TODO to remove this "if defined"? (or a comment to describe > the reason for this difference) Done. I filed a bug and added a comment with a link.
Did you forget to address Matt's comment in spdy_network_transaction_unittest.cc? https://codereview.chromium.org/39203004/diff/1450001/chrome_frame/plugin_url... File chrome_frame/plugin_url_request.cc (right): https://codereview.chromium.org/39203004/diff/1450001/chrome_frame/plugin_url... chrome_frame/plugin_url_request.cc:49: enable_frame_busting_ = enable_frame_busting; Can't lines 49 & 61 moved to above, to avoid code duplication?
https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... File net/spdy/spdy_network_transaction_unittest.cc (right): https://codereview.chromium.org/39203004/diff/1220001/net/spdy/spdy_network_t... net/spdy/spdy_network_transaction_unittest.cc:1828: DelayedSocketData data(2, reads, arraysize(reads), writes, arraysize(writes)); On 2013/11/07 21:55:51, mmenke wrote: > I believe this can just be a StaticSocketDataProvider now. Using it causes hang around line 213. (see reply above) https://codereview.chromium.org/39203004/diff/1450001/chrome_frame/plugin_url... File chrome_frame/plugin_url_request.cc (right): https://codereview.chromium.org/39203004/diff/1450001/chrome_frame/plugin_url... chrome_frame/plugin_url_request.cc:49: enable_frame_busting_ = enable_frame_busting; On 2013/11/11 04:27:53, hashimoto wrote: > Can't lines 49 & 61 moved to above, to avoid code duplication? Done.
lgtm, thanks
I'm going to have to bow out due to time restrictions. Greg, can you take a look please?
To be clear: the changes in chrome_frame are to more or less maintain the old behavior? If this is the case, then LGTM. Thanks.
Should update the comment to reflect the ChromeFrame changes. I'll look at what's happening with the SPDY test later today, but expect the test is fine as-is.
On 2013/11/11 16:28:38, mmenke wrote: > Should update the comment to reflect the ChromeFrame changes. I'll look at > what's happening with the SPDY test later today, but expect the test is fine > as-is. LGTM, other than updating the description to mention ChromeFrame.
On 2013/11/11 14:53:41, grt wrote: > To be clear: the changes in chrome_frame are to more or less maintain the old > behavior? If this is the case, then LGTM. Thanks. This doesn't preserve the old behavior but is a partial fix or workaround for Chrome Frame. I believe this doesn't break anything.
On 2013/11/11 19:28:31, mmenke wrote: > On 2013/11/11 16:28:38, mmenke wrote: > > Should update the comment to reflect the ChromeFrame changes. I'll look at > > what's happening with the SPDY test later today, but expect the test is fine > > as-is. > > LGTM, other than updating the description to mention ChromeFrame. Done! I updated the description.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/39203004/1570001
Message was sent while issue was closed.
Change committed as 234436 |
