Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2007013003:
Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. (Closed)
|
Created:
4 years, 6 months ago by maksims (do not use this acc) Modified:
4 years, 6 months ago Reviewers:
mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce error handling in HttpStreamParser on
UploadDataStream::Read() failure.
HttpStreamParser was written with the assumption
UploadDataStream::Read() never fails, which the
UploadDataStream API currently guarantees. Instead,
on read errors, UploadDataStream zero pads the
response, resulting in sending corrupted data. We're
working on changing the UploadDataStream API so that
Read will return errors on failures.
This cl introduces error handling in HttpStreamParser
class after UploadDataStream::Read() starts returning
errors if read fails.
There are several reviews for 3 streams:
SpdyHttpStream:
https://codereview.chromium.org/2022053002/
HttpStreamParser
https://codereview.chromium.org/2007013003/ <------ current
QuicHttpStream
https://codereview.chromium.org/2035643002/
And the main change in UploadDataStream:
https://codereview.chromium.org/2030353002/
BUG=517615
Committed: https://crrev.com/52fcdc5e13fb7a0334ead130d73a4e41d1b83549
Cr-Commit-Position: refs/heads/master@{#401550}
Patch Set 1 #
Total comments: 31
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #Patch Set 4 : added upload_data_stream.cc/h in order to be able to land the cl #
Messages
Total messages: 33 (22 generated)
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
This looks really good to me, just a bunch of nits. https://codereview.chromium.org/2007013003/diff/1/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2007013003/diff/1/net/base/net_error_list.h#n... net/base/net_error_list.h:116: NET_ERROR(FILE_READ_FAIL, -28) We may want to add this when you modify the ElementsUploadDataStream (Or whatever it's called) to return read errors, but don't think we need it now. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:296: return result; I think we can actually just keep the DCHECK here, instead - if it's in memory, reads can't fail. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:320: result = DoLoop(OK); Should keep these line break, for readability. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:549: if (result == ERR_FILE_READ_FAIL) This should be "if (result < 0)" - if we see any read failure, we should fail, it doesn't matter which one it is. Also note that we can't get ERR_IO_PENDING here. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... File net/http/http_stream_parser_unittest.cc (right): https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:49: const bool kAsynchronously = true; I'd suggest using an enum, and making the enum a public member of ElementsUploadDataStreamError. Could even make it an enum class, which is more defensive - protects against accidentally using it where you want a real bool. The downside of enum classes is they're wordy, but think the benefits outweigh the cost, in this case. So could do something like: enum class FailureMode { SYNC, ASYNC }; And then, outside of ElementsUploadDataStreamError, it would be referenced as, for example, ElementsUploadDataStreamError::FailureMode::SYNC. (You may know all this, just mentioning it because this C++0x11 stuff is fairly new to me, at least) https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:68: class ElementsUploadDataStreamError : public ElementsUploadDataStream { This should inherit from UploadDataStream, instead of ElementsUploadDataStream. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:68: class ElementsUploadDataStreamError : public ElementsUploadDataStream { Also, this is an UploadDataStream with a read error, rather than the error itself. Think a name like "UploadDataStreamWithReadError" or "ReadErrorUploadDataStream" would be clearer. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:73: bool async = false) There's a preference not to use default values, in net, to make code clearer. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:80: return ERR_IO_PENDING; For the async case, suggest instead: base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( base::Bind(&ElementsUploadDataStreamError::CompleteRead, weak_factory_.GetWeakPtr())); return ERR_IO_PENDING; That will make it automatically run CompleteRead asynchronously, and CompleteRead can then be private. To see how to set up weak_factory_, can just do a search for "weak_factory_" in the Chrome repo. It basically makes is to if the object is deleted before the bound method runs, the callback will be cancelled, instead of causing a crash. Doesn't really matter for the tests you're adding, since they don't delete the object without waiting for the callback, but think it's best to be safe. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:81: return ERR_FILE_READ_FAIL; This can be any error, per my suggestion not to add this error code. Could just use ERR_FAILED, for instance, which is used in a lot of obscure cases, not worth their own error code. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:84: void CompleteRead(int result) { Don't think this should take an argument, since ReadInternal returns a fixed error code. Best to be consistent. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:89: bool IsInMemory() const override { return false; } nit: This should be up with the other override method. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:90: bool async_; nit: const https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:90: bool async_; nit: DISALLOW_COPY_AND_ASSIGN(ElementsUploadDataStream); https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:130: callback.callback())); This works, and is correct, but I suggest instead: int result = parser.SendRequest(...); EXPECT_EQ(ERR_..., callback.GetResult(result)); This works if parser returns a result asynchronously for any reason, and can use it for the next test as well, if you follow my suggestions above. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:168: DLOG(ERROR) << "TUT"; Remove debugging code.
Description was changed from ========== first version BUG= ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. It was believed UploadDataStream::Read() never failed. But when internal read methods fail, the buffer is zero padded and zeros are uploaded without any error indication. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. It was believed UploadDataStream::Read() never failed. But when internal read methods fail, the buffer is zero padded and zeros are uploaded without any error indication. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. It was believed UploadDataStream::Read() never failed. But when internal read methods fail, the buffer is zero padded and zeros are uploaded without any error indication. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. BUG=517615 ==========
Patchset #2 (id:20001) has been deleted
Please review. https://codereview.chromium.org/2007013003/diff/1/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/2007013003/diff/1/net/base/net_error_list.h#n... net/base/net_error_list.h:116: NET_ERROR(FILE_READ_FAIL, -28) On 2016/05/24 17:13:47, mmenke wrote: > We may want to add this when you modify the ElementsUploadDataStream (Or > whatever it's called) to return read errors, but don't think we need it now. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:296: return result; On 2016/05/24 17:13:47, mmenke wrote: > I think we can actually just keep the DCHECK here, instead - if it's in memory, > reads can't fail. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:320: result = DoLoop(OK); On 2016/05/24 17:13:47, mmenke wrote: > Should keep these line break, for readability. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:549: if (result == ERR_FILE_READ_FAIL) On 2016/05/24 17:13:47, mmenke wrote: > This should be "if (result < 0)" - if we see any read failure, we should fail, > it doesn't matter which one it is. Also note that we can't get ERR_IO_PENDING > here. Done. Or should it be DCHECK_NE(result, ERR_IO_PENDING) ? https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... File net/http/http_stream_parser_unittest.cc (right): https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:49: const bool kAsynchronously = true; On 2016/05/24 17:13:47, mmenke wrote: > I'd suggest using an enum, and making the enum a public member of > ElementsUploadDataStreamError. > > Could even make it an enum class, which is more defensive - protects against > accidentally using it where you want a real bool. The downside of enum classes > is they're wordy, but think the benefits outweigh the cost, in this case. So > could do something like: > > enum class FailureMode { > SYNC, > ASYNC > }; > > And then, outside of ElementsUploadDataStreamError, it would be referenced as, > for example, ElementsUploadDataStreamError::FailureMode::SYNC. (You may know > all this, just mentioning it because this C++0x11 stuff is fairly new to me, at > least) Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:68: class ElementsUploadDataStreamError : public ElementsUploadDataStream { On 2016/05/24 17:13:47, mmenke wrote: > This should inherit from UploadDataStream, instead of ElementsUploadDataStream. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:68: class ElementsUploadDataStreamError : public ElementsUploadDataStream { On 2016/05/24 17:13:47, mmenke wrote: > Also, this is an UploadDataStream with a read error, rather than the error > itself. Think a name like "UploadDataStreamWithReadError" or > "ReadErrorUploadDataStream" would be clearer. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:73: bool async = false) On 2016/05/24 17:13:48, mmenke wrote: > There's a preference not to use default values, in net, to make code clearer. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:80: return ERR_IO_PENDING; On 2016/05/24 17:13:47, mmenke wrote: > For the async case, suggest instead: > > base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( > base::Bind(&ElementsUploadDataStreamError::CompleteRead, > weak_factory_.GetWeakPtr())); > return ERR_IO_PENDING; > > That will make it automatically run CompleteRead asynchronously, and > CompleteRead can then be private. To see how to set up weak_factory_, can just > do a search for "weak_factory_" in the Chrome repo. It basically makes is to if > the object is deleted before the bound method runs, the callback will be > cancelled, instead of causing a crash. Doesn't really matter for the tests > you're adding, since they don't delete the object without waiting for the > callback, but think it's best to be safe. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:81: return ERR_FILE_READ_FAIL; On 2016/05/24 17:13:48, mmenke wrote: > This can be any error, per my suggestion not to add this error code. Could just > use ERR_FAILED, for instance, which is used in a lot of obscure cases, not worth > their own error code. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:84: void CompleteRead(int result) { On 2016/05/24 17:13:47, mmenke wrote: > Don't think this should take an argument, since ReadInternal returns a fixed > error code. Best to be consistent. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:89: bool IsInMemory() const override { return false; } On 2016/05/24 17:13:47, mmenke wrote: > nit: This should be up with the other override method. Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:90: bool async_; On 2016/05/24 17:13:47, mmenke wrote: > nit: const Done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser_unittest.cc:130: callback.callback())); On 2016/05/24 17:13:47, mmenke wrote: > This works, and is correct, but I suggest instead: > > int result = parser.SendRequest(...); > EXPECT_EQ(ERR_..., callback.GetResult(result)); > > This works if parser returns a result asynchronously for any reason, and can use > it for the next test as well, if you follow my suggestions above. Done.
Want to do a final quick pass after you respond to these, out of paranoia, and since landing this one day later won't block your other CLs, but think we're done. https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser.cc File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2007013003/diff/1/net/http/http_stream_parser... net/http/http_stream_parser.cc:549: if (result == ERR_FILE_READ_FAIL) On 2016/05/25 07:13:38, maksims wrote: > On 2016/05/24 17:13:47, mmenke wrote: > > This should be "if (result < 0)" - if we see any read failure, we should fail, > > it doesn't matter which one it is. Also note that we can't get ERR_IO_PENDING > > here. > > Done. Or should it be DCHECK_NE(result, ERR_IO_PENDING) ? We already have that DCHECK in HttpStreamParser::DoLoop, before calling this, and other comparable methods in this file don't, so probably best just to keep things as-is. However, that wouldn't be unreasonable here. https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... net/http/http_stream_parser.cc:295: DCHECK_GT(consumed, 0); You removed a comment here. Please bring it back. Actually, could improve it to say "Read() must succeed synchronously if not chunked and in memory." https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... File net/http/http_stream_parser_unittest.cc (right): https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... net/http/http_stream_parser_unittest.cc:70: ReadErrorUploadDataStream(int64_t identifier, FailureMode mode) optional: May want to just replace with with: explicit ReadErrorUploadDataStream(FailureMode mode) and hard-code the identifier of 0 below, just to make call sites simpler. The explicit is used on 1 argument constructors, so ReadErrorUploadDataStream blah = FailureMode::SYNC doesn't compile. https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... net/http/http_stream_parser_unittest.cc:72: async_(static_cast<bool>(mode)), Should make async a FailureMode instead, and compare it explicitly against FailureMode::SYNC or FailureMode::ASYNC in ReadInternal. Generally, should avoid cast operations unless they're absolutely necessary necessary. https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... net/http/http_stream_parser_unittest.cc:80: int ReadInternal(IOBuffer* buf, int buf_len) override { nit: Add comment "// UploadDataStream implementation:"
please take a look https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... File net/http/http_stream_parser.cc (right): https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... net/http/http_stream_parser.cc:295: DCHECK_GT(consumed, 0); On 2016/05/25 15:13:31, mmenke wrote: > You removed a comment here. Please bring it back. > > Actually, could improve it to say "Read() must succeed synchronously if not > chunked and in memory." Done. https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... File net/http/http_stream_parser_unittest.cc (right): https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... net/http/http_stream_parser_unittest.cc:70: ReadErrorUploadDataStream(int64_t identifier, FailureMode mode) On 2016/05/25 15:13:31, mmenke wrote: > optional: May want to just replace with with: > > explicit ReadErrorUploadDataStream(FailureMode mode) > > and hard-code the identifier of 0 below, just to make call sites simpler. > > The explicit is used on 1 argument constructors, so ReadErrorUploadDataStream > blah = FailureMode::SYNC doesn't compile. Yes, sure. That's a good point. Protecting self from type casting. https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... net/http/http_stream_parser_unittest.cc:72: async_(static_cast<bool>(mode)), On 2016/05/25 15:13:31, mmenke wrote: > Should make async a FailureMode instead, and compare it explicitly against > FailureMode::SYNC or FailureMode::ASYNC in ReadInternal. Generally, should > avoid cast operations unless they're absolutely necessary necessary. Done. https://codereview.chromium.org/2007013003/diff/40001/net/http/http_stream_pa... net/http/http_stream_parser_unittest.cc:80: int ReadInternal(IOBuffer* buf, int buf_len) override { On 2016/05/25 15:13:31, mmenke wrote: > nit: Add comment "// UploadDataStream implementation:" Done.
LGTM! Good job on the tests, especially. Could you update the second paragraph in the description, though. Replace "It was believed UploadDataStream::Read() never failed. But when internal read methods fail, the buffer is zero padded and zeros are uploaded without any error indication." Which isn't quite correct, with something like: "HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures."
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. It was believed UploadDataStream::Read() never failed. But when internal read methods fail, the buffer is zero padded and zeros are uploaded without any error indication. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ <------ current HttpStreamParser https://codereview.chromium.org/2007013003/ QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/........../ - not ready And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ==========
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/2035643002/ And main change in UploadDataStream: https://codereview.chromium.org/........../ - not ready BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ==========
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007013003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2007013003/#ps100001 (title: "added upload_data_stream.cc/h in order to be able to land the cl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007013003/100001
The CQ bit was unchecked by maksim.sisov@intel.com
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2007013003/#ps120001 (title: "added upload_data_stream.cc/h in order to be able to land the cl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007013003/120001
Message was sent while issue was closed.
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 ========== to ========== Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure. HttpStreamParser was written with the assumption UploadDataStream::Read() never fails, which the UploadDataStream API currently guarantees. Instead, on read errors, UploadDataStream zero pads the response, resulting in sending corrupted data. We're working on changing the UploadDataStream API so that Read will return errors on failures. This cl introduces error handling in HttpStreamParser class after UploadDataStream::Read() starts returning errors if read fails. There are several reviews for 3 streams: SpdyHttpStream: https://codereview.chromium.org/2022053002/ HttpStreamParser https://codereview.chromium.org/2007013003/ <------ current QuicHttpStream https://codereview.chromium.org/2035643002/ And the main change in UploadDataStream: https://codereview.chromium.org/2030353002/ BUG=517615 Committed: https://crrev.com/52fcdc5e13fb7a0334ead130d73a4e41d1b83549 Cr-Commit-Position: refs/heads/master@{#401550} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/52fcdc5e13fb7a0334ead130d73a4e41d1b83549 Cr-Commit-Position: refs/heads/master@{#401550} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
