Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2334773002:
Add net::GzipSourceStream (Closed)
|
Created:
4 years, 3 months ago by xunjieli Modified:
4 years, 2 months ago Reviewers:
Randy Smith (Not in Mondays) CC:
chromium-reviews, cbentzel+watch_chromium.org, mmenke Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd net::GzipSourceStream
This CL adds a net::GzipSourceStream which implements
net::FilterSourceStream to do gzip decoding.
This is a part of the efforts to convert net::Filter
into a pull-based interface. See the linked bug for
more details.
BUG=474859
Committed: https://crrev.com/89be8ae4a98355e81372cc35dab5e125913c5e3f
Cr-Commit-Position: refs/heads/master@{#421314}
Patch Set 1 #Patch Set 2 : self review sync-ed to r417929 #
Total comments: 20
Patch Set 3 : address comments and rebased onto CL 2338043002 #
Total comments: 26
Patch Set 4 : Addressed Randy's comments and synced to r419441 #
Total comments: 14
Patch Set 5 : Address Randy's comments (synced to 92104a0503b2862f54d60473b59dd1ae145eb22b) #
Total comments: 14
Patch Set 6 : Address Randy's comments #
Dependent Patchsets: Messages
Total messages: 39 (24 generated)
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org, rdsmith@chromium.org
Randy and Matt: PTAL. Thanks.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add net::GzipSourceStream This CL adds a net::GzipSourceStream which implements net::FilterSourceStream to do gzip decoding. BUG=474859 ========== to ========== Add net::GzipSourceStream This CL adds a net::GzipSourceStream which implements net::FilterSourceStream to do gzip decoding. This is a part of the efforts to convert net::Filter into a pull-based interface. See the linked bug for more details. BUG=474859 ==========
First pass. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:99: // synchronously. If this happens, return right here. Couldn't everything after line 97 here be replaced by something like: DCHECK(bytes_read != 0 || 0 == input_buffer->BytesRemaining()); return bytes_read; ? https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:104: DCHECK_EQ(0, input_buffer->BytesRemaining()); This won't necessarily be true if Decompress() returns an error? https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:215: bool GzipSourceStream::ShouldFallbackToPlain(DrainableIOBuffer* input_buffer) { Could we not pass a DrainableIOBuffer to this function? That type implies the function could actually drain data, which it won't. char* or void*? https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:215: bool GzipSourceStream::ShouldFallbackToPlain(DrainableIOBuffer* input_buffer) { I'm also concerned that the interface contract for this function says that it'll return "false" if there isn't enough information in DrainableIOBuffer to decide whether or not to fallback, but the code that calls it assumes that false always means that we should not fall back. We only need a single byte for the decision we're making; maybe pass in that byte instead of a char* and require the calling code to know that we need at least a byte? https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:49: bool Init(); Documentation as to the return value of Init()? https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:66: // Returns how many bytes are drained from |input_buffer|. I don't understand based on the comment what this function does? (I assume it copies directly from input to output, but it should say that.) https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:79: // have been seen to be sure. This method pulls its input from |buffer_|. I presume this can be called multiple times with multiple input_buffers? And that in the case of returning false for a complete, valid header, it doesn't drain any more bytes? https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:94: GzipSourceStreamMode mode_; nit, suggestion: I prefer whitespace before explanatory comments, to make it easier to visually group a comment with the code it applies to. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:94: GzipSourceStreamMode mode_; It looks to me as if this is set in the constructor, not in Init(). If that's true, can it be made const?
Thanks for the review! PTAL. I have uploaded a new patch to address the comments. I also rebased the CL on top of the two requested changes on the original FilterSourceStream CL. The two requested changes are at: https://codereview.chromium.org/2338043002/ https://codereview.chromium.org/2331193005/ https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:99: // synchronously. If this happens, return right here. On 2016/09/12 20:54:00, Randy Smith - Not in Fridays wrote: > Couldn't everything after line 97 here be replaced by something like: > > DCHECK(bytes_read != 0 || 0 == input_buffer->BytesRemaining()); > return bytes_read; > > ? Done. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:104: DCHECK_EQ(0, input_buffer->BytesRemaining()); On 2016/09/12 20:54:00, Randy Smith - Not in Fridays wrote: > This won't necessarily be true if Decompress() returns an error? Done. Yes, we might not drain all bytes if there is an error. I have changed the assertion according to your earlier suggestion which should take care of the error case. Thanks for catching that. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:215: bool GzipSourceStream::ShouldFallbackToPlain(DrainableIOBuffer* input_buffer) { On 2016/09/12 20:54:01, Randy Smith - Not in Fridays wrote: > Could we not pass a DrainableIOBuffer to this function? That type implies the > function could actually drain data, which it won't. char* or void*? Done. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:215: bool GzipSourceStream::ShouldFallbackToPlain(DrainableIOBuffer* input_buffer) { On 2016/09/12 20:54:01, Randy Smith - Not in Fridays wrote: > I'm also concerned that the interface contract for this function says that it'll > return "false" if there isn't enough information in DrainableIOBuffer to decide > whether or not to fallback, but the code that calls it assumes that false always > means that we should not fall back. We only need a single byte for the decision > we're making; maybe pass in that byte instead of a char* and require the calling > code to know that we need at least a byte? Done. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:49: bool Init(); On 2016/09/12 20:54:01, Randy Smith - Not in Fridays wrote: > Documentation as to the return value of Init()? Done. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:66: // Returns how many bytes are drained from |input_buffer|. On 2016/09/12 20:54:01, Randy Smith - Not in Fridays wrote: > I don't understand based on the comment what this function does? (I assume it > copies directly from input to output, but it should say that.) Done. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:79: // have been seen to be sure. This method pulls its input from |buffer_|. On 2016/09/12 20:54:01, Randy Smith - Not in Fridays wrote: > I presume this can be called multiple times with multiple input_buffers? And > that in the case of returning false for a complete, valid header, it doesn't > drain any more bytes? Done. Yes, I believe you are right. I will add this to the doc. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:94: GzipSourceStreamMode mode_; On 2016/09/12 20:54:01, Randy Smith - Not in Fridays wrote: > nit, suggestion: I prefer whitespace before explanatory comments, to make it > easier to visually group a comment with the code it applies to. Done. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:94: GzipSourceStreamMode mode_; On 2016/09/12 20:54:01, Randy Smith - Not in Fridays wrote: > It looks to me as if this is set in the constructor, not in Init(). If that's > true, can it be made const? Done.
Next pass. The biggest thing is wanting to explore ways of simplifying the state transitions in the class. Let me know what you think of my first try below. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:66: // Returns how many bytes are drained from |input_buffer|. On 2016/09/14 16:44:01, xunjieli wrote: > On 2016/09/12 20:54:01, Randy Smith - Not in Fridays wrote: > > I don't understand based on the comment what this function does? (I assume it > > copies directly from input to output, but it should say that.) > > Done. Not quite what I meant. The documentation is still documenting only the return value, not what the function *does*. If it purely copied just saying "Copies bytes from input_buffer to output_buffer, returning the number of bytes copied" would be fine. But my memory of the implementation is that the function is more complex and does something with gzip headers. What does the function do? https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:91: should_check_gzip_header_ = false; Suggestion: I'm finding myself a bit uncomfortable with all of these booleans that are checked (and set) in semi-random places in the code. Would it be cleaner to instead use a single state variable enum describing where/what mode we are in with regard to the input stream? I imagine: enum InputState { STREAM_START, GZIP_HEADER, COMPRESSED_BODY, GZIP_FOOTER, UNCOMPRESSED_BODY } With this pattern the tests would be conditionalized by where you were in the stream. The ShouldFallbackToPlain test would occur at stream start. That would transition to UNCOMPRESSED_BODY on fallback, and GZIP_HEADER or COMPRESSED_BODY based on the mode of the filter. The handling for COMPRESSED_BODY would transition out to GZIP_FOOTER or UNCOMPRESSED_BODY based on the mode, and GZIP_FOOTER to UNCOMPRESSEd_BODY when it was done. There could be a switch statement at the beginning of FilterData(), maybe wrapped in a while loop in case multiple state transitions could occur in a single call, with calls to sub-routines that are the right ones for each state. The handler for GZIP_FOOTER could make the determination as to whether the footer needed to be skipped before going to passthrough mode or not, which would be (to me) a cleaner place for it in the overall code than the PassThrough() function. I'm also open to other ideas about how to simplify the code, and would accept it in its current state. But it's just striking more as more complicated than is ideal for long-term maintenance so I'd like to explore ways to simplify it. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:137: // to splice it back in. It looks like this would happen anywhere in the stream, not just at the beginning; is that correct? Or does the zlib header show up at multiple points in the stream? https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:151: // in the wild any more? +1 (though it's fine if you don't want to do that in this CL). https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:160: return ERR_CONTENT_DECODING_FAILED; Do we have any documentation in the interface contract as to whether FilterData() can be called after it returns an error? I'm inclined to promise subclasses that it won't so be called if we haven't already. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:47: GzipSourceStream(std::unique_ptr<SourceStream> previous, nit, suggestion: "previous -> upstream"? https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:50: // Returns true if initialization is successful, false otherwise. Under what circumstances can initialization fail? More specifically, it doesn't look like there's any variations in consumer input to the stream to this point, and it doesn't read anything from the upstream stream as part of Init(). So why should the Init() return be constant for all calls? Digging down into the implementation, the reasons I see for Init() returning false are: * new failing. We don't bother to check this in any other chrome code; OOM errors are crashes. * inflateInit{,2} failing. The documentation on inflateInit says that it returns errors if OOM, the version is incompatible between header and linked library, or an invalid argument (e.g. null pointer) was passed. Based on that, I'd suggest documenting this as returning false in the case of a version mismatch and DHCECKing other than that, or alternatively DHCECKing everything--all these errors really sound like they're bugs in our code (or OOM), not something that's useful or actionable to the caller. (This is a suggestion, both because I'm not sure about it and because this problem (return value and lack of documentation of return value) predates this CL. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:104: // This variable is set only once by Init. nit: No longer set by Init, correct? And "const" pretty much means "only set in the constructor", so we could probably nuke the second sentence entirely. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream_unittest.cc (right): https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:31: void Init() { Init(false); } This is function overloading, which is discouraged by the style guild unless someone looking at the call site can get an idea of what's happening without figuring out which overload is being called. I don't think this qualifies, so I'm inclined to think you should just call "Init(false);" everywhere instead of Init(). https://google.github.io/styleguide/cppguide.html#Function_Overloading https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:71: void Compress(char* source, Document? Mostly I want to make sure *dest_len is documented as an input/output parameter. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:180: EXPECT_EQ("GZIP", gzip_stream()->Description()); Why test gzip_stream() description in a test that's targeting the deflate stream? https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:272: // gzip stream to falsely accept the corrupt data and pass it through untouched. nitty nit: I'd put this comment just before the assignment to gzipped_data()[1]; that's what it refers to, not really the test as a whole. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:316: Test for both missing gzip footer and present gzip footer?
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Thanks for the thorough review and thanks for being incredibly patient with this CL! I have uploaded a new patch. PTAL. https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2334773002/diff/20001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:66: // Returns how many bytes are drained from |input_buffer|. On 2016/09/14 22:25:23, Randy Smith - Not in Fridays wrote: > On 2016/09/14 16:44:01, xunjieli wrote: > > On 2016/09/12 20:54:01, Randy Smith - Not in Fridays wrote: > > > I don't understand based on the comment what this function does? (I assume > it > > > copies directly from input to output, but it should say that.) > > > > Done. > > Not quite what I meant. The documentation is still documenting only the return > value, not what the function *does*. If it purely copied just saying "Copies > bytes from input_buffer to output_buffer, returning the number of bytes copied" > would be fine. But my memory of the implementation is that the function is more > complex and does something with gzip headers. What does the function do? Done. It additionally drains footer bytes from the stream. I've adjusted the documentation. Let me know if that's clearer. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:91: should_check_gzip_header_ = false; On 2016/09/14 22:25:23, Randy Smith - Not in Fridays wrote: > Suggestion: I'm finding myself a bit uncomfortable with all of these booleans > that are checked (and set) in semi-random places in the code. Would it be > cleaner to instead use a single state variable enum describing where/what mode > we are in with regard to the input stream? I imagine: > > enum InputState { > STREAM_START, > GZIP_HEADER, > COMPRESSED_BODY, > GZIP_FOOTER, > UNCOMPRESSED_BODY > } > > With this pattern the tests would be conditionalized by where you were in the > stream. The ShouldFallbackToPlain test would occur at stream start. That would > transition to UNCOMPRESSED_BODY on fallback, and GZIP_HEADER or COMPRESSED_BODY > based on the mode of the filter. The handling for COMPRESSED_BODY would > transition out to GZIP_FOOTER or UNCOMPRESSED_BODY based on the mode, and > GZIP_FOOTER to UNCOMPRESSEd_BODY when it was done. There could be a switch > statement at the beginning of FilterData(), maybe wrapped in a while loop in > case multiple state transitions could occur in a single call, with calls to > sub-routines that are the right ones for each state. The handler for > GZIP_FOOTER could make the determination as to whether the footer needed to be > skipped before going to passthrough mode or not, which would be (to me) a > cleaner place for it in the overall code than the PassThrough() function. > > I'm also open to other ideas about how to simplify the code, and would accept it > in its current state. But it's just striking more as more complicated than is > ideal for long-term maintenance so I'd like to explore ways to simplify it. Done. Great idea! https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:137: // to splice it back in. On 2016/09/14 22:25:23, Randy Smith - Not in Fridays wrote: > It looks like this would happen anywhere in the stream, not just at the > beginning; is that correct? Or does the zlib header show up at multiple points > in the stream? I believe it should only show up in the beginning. But the current code (gzip_filter.cc) doesn't differentiate this case. I guess we can add another variable to keep track whether we are at the beginning or in the middle. However, on the other hand, since we are planning to remove this all together. We can leave it as it is and add UMA tracking in a follow-up CL. Let me know what you think. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:151: // in the wild any more? On 2016/09/14 22:25:23, Randy Smith - Not in Fridays wrote: > +1 (though it's fine if you don't want to do that in this CL). Acknowledged. I am planning to do it in a followup so I can loop in histograms owners separately and keep this one small. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:160: return ERR_CONTENT_DECODING_FAILED; On 2016/09/14 22:25:23, Randy Smith - Not in Fridays wrote: > Do we have any documentation in the interface contract as to whether > FilterData() can be called after it returns an error? I'm inclined to promise > subclasses that it won't so be called if we haven't already. The interface documentation says that ERR_CONTENT_DECODING_FAILED is unrecoverable error, so by that reasoning, filtering fails and no FilterData() will be called again. // If an unrecoverable error occurred, this should return // ERR_CONTENT_DECODING_FAILED or a more specific error code. Let me know if you'd like me to make it more explicit. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:47: GzipSourceStream(std::unique_ptr<SourceStream> previous, On 2016/09/14 22:25:24, Randy Smith - Not in Fridays wrote: > nit, suggestion: "previous -> upstream"? Done. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:50: // Returns true if initialization is successful, false otherwise. On 2016/09/14 22:25:23, Randy Smith - Not in Fridays wrote: > Under what circumstances can initialization fail? > > More specifically, it doesn't look like there's any variations in consumer input > to the stream to this point, and it doesn't read anything from the upstream > stream as part of Init(). So why should the Init() return be constant for all > calls? > > Digging down into the implementation, the reasons I see for Init() returning > false are: > > * new failing. We don't bother to check this in any other chrome code; OOM > errors are crashes. > * inflateInit{,2} failing. The documentation on inflateInit says that it > returns errors if OOM, the version is incompatible between header and linked > library, or an invalid argument (e.g. null pointer) was passed. > > Based on that, I'd suggest documenting this as returning false in the case of a > version mismatch and DHCECKing other than that, or alternatively DHCECKing > everything--all these errors really sound like they're bugs in our code (or > OOM), not something that's useful or actionable to the caller. > > (This is a suggestion, both because I'm not sure about it and because this > problem (return value and lack of documentation of return value) predates this > CL. Done. I am not sure if it's a good idea to DCHECK on version mismatch though. Will it be better to pass raw input to user (current behavior) than to crash the browser process? I guess we can land the DCHECK first, if this breaks people, we can remove the DCHECK? https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.h:104: // This variable is set only once by Init. On 2016/09/14 22:25:23, Randy Smith - Not in Fridays wrote: > nit: No longer set by Init, correct? And "const" pretty much means "only set in > the constructor", so we could probably nuke the second sentence entirely. Done. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream_unittest.cc (right): https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:31: void Init() { Init(false); } On 2016/09/14 22:25:24, Randy Smith - Not in Fridays wrote: > This is function overloading, which is discouraged by the style guild unless > someone looking at the call site can get an idea of what's happening without > figuring out which overload is being called. I don't think this qualifies, so > I'm inclined to think you should just call "Init(false);" everywhere instead of > Init(). > > https://google.github.io/styleguide/cppguide.html#Function_Overloading Done. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:71: void Compress(char* source, On 2016/09/14 22:25:24, Randy Smith - Not in Fridays wrote: > Document? Mostly I want to make sure *dest_len is documented as an input/output > parameter. Done. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:180: EXPECT_EQ("GZIP", gzip_stream()->Description()); On 2016/09/14 22:25:24, Randy Smith - Not in Fridays wrote: > Why test gzip_stream() description in a test that's targeting the deflate > stream? Done. Good catch! https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:272: // gzip stream to falsely accept the corrupt data and pass it through untouched. On 2016/09/14 22:25:24, Randy Smith - Not in Fridays wrote: > nitty nit: I'd put this comment just before the assignment to gzipped_data()[1]; > that's what it refers to, not really the test as a whole. Done. I think this is an old comment that no longer applies. I have adjusted the test accordingly. Thanks for bringing it up! https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream_unittest.cc:316: On 2016/09/14 22:25:24, Randy Smith - Not in Fridays wrote: > Test for both missing gzip footer and present gzip footer? Done. Great idea!
Next round of comments :-}. And thank you for *your* patience--I feel like i haven't been giving very quick turnaround time on this CL :-J. And it's also taking more time because I like pushing for further cleanups when I see them. But I hope the result will be good. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:137: // to splice it back in. On 2016/09/19 13:57:03, xunjieli wrote: > On 2016/09/14 22:25:23, Randy Smith - Not in Fridays wrote: > > It looks like this would happen anywhere in the stream, not just at the > > beginning; is that correct? Or does the zlib header show up at multiple > points > > in the stream? > > I believe it should only show up in the beginning. But the current code > (gzip_filter.cc) doesn't differentiate this case. I guess we can add another > variable to keep track whether we are at the beginning or in the middle. > > However, on the other hand, since we are planning to remove this all together. > We can leave it as it is and add UMA tracking in a follow-up CL. Let me know > what you think. Gotcha--I think I understand better now. I'm happy leaving this for a followup CL given that we're thinking about removing it. If it stays in the code, I'd like to explore if we can replace this variable with another state, given that there's only one place in the input stream that this can happen. (It's a bit tricky to write code that has this as another state--the code that's shared between that state and the normal decompress state has to be put into a subroutine that includes both setting the zlib_stream next/avail values, calling inflate, and return the result. At that point, you can have this state be "Try to deflate, if that fails stuff in the header (with error check) and transition to the ongoing decompress state". Which I think would be cleaner. But I'm ok with this if we really do have plans to remove this code in a followup CL.) https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:124: zlib_stream_.get()->avail_in = input_buffer_size - *consumed_bytes; Thought (i.e. not even a suggestion, just tossing it out in case you like the idea): You could have a char* variable at the top of the function initialized to input_buffer->data(), and then increment it and decrement input_buffer_size through this function, so that this section of code would always have simple variables to access the input buffer through. https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:159: return bytes_out; I'm confused. Above here there is code to (IIUC) handle the case where we loop a couple of times in STATE_COMPRESSED_BODY; we modified *consumed_bytes and then use it to figure out how far we've gotten in the input buffer. But this implies that the the zlib inflate function will accept all the incoming data in one call unless it reaches the end of the compressed data. What am I missing? (I'm also confused because it's my assumption that the inflate function will respect the limits on the output buffer, and my assumption is that if the output buffer is limited enough it may choose not to read the entire input buffer. But maybe it commits to always reading the entire input buffer and doing internal buffering if necessary?) https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:181: size_t footer_bytes_skipped = NumGzipFooterBytesToSkip(input_buffer_size); Why not implement this with a separate state? It would make this function simpler--it could just be "passthrough" without the magic two layers deep inside NumGzipFooterBytesToSkip() and it would be clear at the top level we're moving through the footer. And it would also give us an easy, separate function in which to validate the footer if we so choose (NumGzipFooterBytesToSkip() could be that currently, but the name doesn't sound like a validation function and has no way to return an error, so it's not ideal.) https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.h:71: // |input_buffer|. This decompressor will decompress a zlib stream (either Presumably it will also stop when it's filled |*output_buffer|? https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.h:82: // |input_buffer| without writing the footer bytes to |output_buffer|. The I don't find this documentation complete; can this happen anywhere in the stream, or only at the beginning? ( https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.h:93: // The function returns true on success and false otherwise. nit, suggestion: I don't think the comments needs to say both true on success and false otherwise; if you specify that it returns true on success (or false on failure) the other is implicit. Also, it's more useful if the comment describes what can lead to failure (like the Init() comment now does). https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.h:104: int* consumed_bytes); I don't see this function in the source file anymore?
Patchset #5 (id:160001) has been deleted
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review! The code looks much better after applying the suggestions. https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/40001/net/filter/gzip_source_... net/filter/gzip_source_stream.cc:137: // to splice it back in. On 2016/09/21 20:57:05, Randy Smith - Not in Mondays wrote: > On 2016/09/19 13:57:03, xunjieli wrote: > > On 2016/09/14 22:25:23, Randy Smith - Not in Fridays wrote: > > > It looks like this would happen anywhere in the stream, not just at the > > > beginning; is that correct? Or does the zlib header show up at multiple > > points > > > in the stream? > > > > I believe it should only show up in the beginning. But the current code > > (gzip_filter.cc) doesn't differentiate this case. I guess we can add another > > variable to keep track whether we are at the beginning or in the middle. > > > > However, on the other hand, since we are planning to remove this all together. > > We can leave it as it is and add UMA tracking in a follow-up CL. Let me know > > what you think. > > Gotcha--I think I understand better now. I'm happy leaving this for a followup > CL given that we're thinking about removing it. If it stays in the code, I'd > like to explore if we can replace this variable with another state, given that > there's only one place in the input stream that this can happen. > > (It's a bit tricky to write code that has this as another state--the code that's > shared between that state and the normal decompress state has to be put into a > subroutine that includes both setting the zlib_stream next/avail values, calling > inflate, and return the result. At that point, you can have this state be "Try > to deflate, if that fails stuff in the header (with error check) and transition > to the ongoing decompress state". Which I think would be cleaner. But I'm ok > with this if we really do have plans to remove this code in a followup CL.) Acknowledged. Thanks! I filed a bug and linked it here so we don't lose track. https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:124: zlib_stream_.get()->avail_in = input_buffer_size - *consumed_bytes; On 2016/09/21 20:57:05, Randy Smith - Not in Mondays wrote: > Thought (i.e. not even a suggestion, just tossing it out in case you like the > idea): You could have a char* variable at the top of the function initialized to > input_buffer->data(), and then increment it and decrement input_buffer_size > through this function, so that this section of code would always have simple > variables to access the input buffer through. Done. https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:159: return bytes_out; On 2016/09/21 20:57:06, Randy Smith - Not in Mondays wrote: > I'm confused. Above here there is code to (IIUC) handle the case where we loop > a couple of times in STATE_COMPRESSED_BODY; we modified *consumed_bytes and then > use it to figure out how far we've gotten in the input buffer. But this implies > that the the zlib inflate function will accept all the incoming data in one call > unless it reaches the end of the compressed data. What am I missing? > > (I'm also confused because it's my assumption that the inflate function will > respect the limits on the output buffer, and my assumption is that if the output > buffer is limited enough it may choose not to read the entire input buffer. But > maybe it commits to always reading the entire input buffer and doing internal > buffering if necessary?) > Done. Thanks for catching that! That's bug in my code. I have updated it and added a test where we use a 1 byte output buffer which will catch this bug. https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:181: size_t footer_bytes_skipped = NumGzipFooterBytesToSkip(input_buffer_size); On 2016/09/21 20:57:06, Randy Smith - Not in Mondays wrote: > Why not implement this with a separate state? It would make this function > simpler--it could just be "passthrough" without the magic two layers deep inside > NumGzipFooterBytesToSkip() and it would be clear at the top level we're moving > through the footer. > > And it would also give us an easy, separate function in which to validate the > footer if we so choose (NumGzipFooterBytesToSkip() could be that currently, but > the name doesn't sound like a validation function and has no way to return an > error, so it's not ideal.) Done. https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... File net/filter/gzip_source_stream.h (right): https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.h:71: // |input_buffer|. This decompressor will decompress a zlib stream (either On 2016/09/21 20:57:06, Randy Smith - Not in Mondays wrote: > Presumably it will also stop when it's filled |*output_buffer|? Done. Added to the doc. https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.h:82: // |input_buffer| without writing the footer bytes to |output_buffer|. The On 2016/09/21 20:57:06, Randy Smith - Not in Mondays wrote: > I don't find this documentation complete; can this happen anywhere in the > stream, or only at the beginning? ( Done. I have inlined this method. After applying your suggestion on the footer, this method is only two lines. https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.h:93: // The function returns true on success and false otherwise. On 2016/09/21 20:57:06, Randy Smith - Not in Mondays wrote: > nit, suggestion: I don't think the comments needs to say both true on success > and false otherwise; if you specify that it returns true on success (or false on > failure) the other is implicit. Also, it's more useful if the comment describes > what can lead to failure (like the Init() comment now does). Done. https://codereview.chromium.org/2334773002/diff/140001/net/filter/gzip_source... net/filter/gzip_source_stream.h:104: int* consumed_bytes); On 2016/09/21 20:57:06, Randy Smith - Not in Mondays wrote: > I don't see this function in the source file anymore? Done.
I'm going to defer to Randy on this one.
xunjieli@chromium.org changed reviewers: - mmenke@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Next round. I'm sorry about the multiple rounds, but I think we're getting pretty close and that the code will be fairly clean when we're done. https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:98: if (ShouldFallbackToPlain(input_data[0])) { Suggestion: I'll call out that this is assuming that input data is non-zero, which will always be true because of the combination of a) the conditional about input_buffer_size == 0 at the top of the function, and b) the fact that no other state enters STATE_START. I'm finding myself wondering if that's worth either a comment or a DCHECK, but I'm not sure. Up to you. https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:118: input_data_size -= bytes_consumed; If this drops input_data_size to zero, we want to return, correct? I think the best way to handle that is by pulling the "if (input_buffer_size == 0)" conditional inside the while loop and making it on input_data_size. Actually, isn't that always true for INCOMPLETE_HEADER, above? Does that suggest a hole in testing (i.e. that there's no test that triggers the INCOMPLETE_HEADER return from GzipHeader::ReadMore())? https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:130: zlib_stream_.get()->avail_out = output_buffer_size; Suggestion: The fact that {next,avail}_out are being set directly from the output buffer passed into the function is assuming that the STATE_COMPRESSED_BODY element in the switch statement will never be visited more than once for a single invocation of FilterData(). I believe that that assumption is both based on the fact that we never leave this block with a simple "break;"; we either return or set a new state and break. But I find myself a bit uncomfortable with that chain of assumptions (even though I think it's a valid chain). How would you feel about having a DCHECK at the top of this clause asserting that this is the only time for this function invocation the clause has been entered? (I'd imagine this would involve declaring a boolean variable in function scope, asserting on it here, and then changing its state.) https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:160: return ERR_CONTENT_DECODING_FAILED; nit, suggestion: Is there a reason this isn't up above before all the arithmetic around how much has been consumed and produced, which I sorta presume isn't valid if inflate() returned an error? https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:172: input_state_ = STATE_UNCOMPRESSED_BODY; NumGzipFooterBytesToSkip() handles being called repeatedly, but this code unilaterally transitioning to STATE_UNCOMPRESSED_BODY seems to assume that the footer will always fit into a single input buffer. Shouldn't this transition only occur if gzip_footer_bytes_left_ == 0? (Anticipated future comment: I'd rather not read&set gzip_footer_bytes_left_ in NumGzip_FooterBytesToSkip and also read it here; that feels like a (minor) layering violation. So if you solve this the obvious way and test gzip_footer_bytes_left_ in evaluating the state transition, could you also inline NumGzipFooterBytesToSkip()? Or alternatively simply transition and break if NumGzipFooterBytesToSkip() returns 0?) https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:176: int to_copy = std::min(input_data_size, output_buffer_size - bytes_out); Suggestion: I don't know if it's worth it, but if you wanted to, you could do the same thing with the output buffer/size you've done with the input buffer & size and have variables that track the current place to write to and the amount of space that's available to write. (It would be a different way of addressing my assertion concern above about only going through STATE_COMPRESSED_BODY once per call, because you'd be updating the output pointers.) https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... File net/filter/gzip_source_stream_unittest.cc (right): https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream_unittest.cc:316: TEST_F(GzipSourceStreamTest, GzipCorrectnessWithSmallOutputBuffer) { Parallel test with a small input buffer? (I.e. drib in a byte at a time?)
Patchset #6 (id:200001) has been deleted
Thanks a lot for the review! I am embarrassed at how many bugs there are :( Hope this round is slight better! PTAL. https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... File net/filter/gzip_source_stream.cc (right): https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:98: if (ShouldFallbackToPlain(input_data[0])) { On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > Suggestion: I'll call out that this is assuming that input data is non-zero, > which will always be true because of the combination of a) the conditional about > input_buffer_size == 0 at the top of the function, and b) the fact that no other > state enters STATE_START. I'm finding myself wondering if that's worth either a > comment or a DCHECK, but I'm not sure. Up to you. Done. I added a DCHECK. https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:118: input_data_size -= bytes_consumed; On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > If this drops input_data_size to zero, we want to return, correct? I think the > best way to handle that is by pulling the "if (input_buffer_size == 0)" > conditional inside the while loop and making it on input_data_size. > > Actually, isn't that always true for INCOMPLETE_HEADER, above? Does that > suggest a hole in testing (i.e. that there's no test that triggers the > INCOMPLETE_HEADER return from GzipHeader::ReadMore())? Done. This is a bug! Thanks for catching it. There's a test in url_request_job_unittest.cc (SlowFilterRead), but not here. I have added a test. https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:130: zlib_stream_.get()->avail_out = output_buffer_size; On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > Suggestion: The fact that {next,avail}_out are being set directly from the > output buffer passed into the function is assuming that the > STATE_COMPRESSED_BODY element in the switch statement will never be visited more > than once for a single invocation of FilterData(). I believe that that > assumption is both based on the fact that we never leave this block with a > simple "break;"; we either return or set a new state and break. But I find > myself a bit uncomfortable with that chain of assumptions (even though I think > it's a valid chain). How would you feel about having a DCHECK at the top of > this clause asserting that this is the only time for this function invocation > the clause has been entered? (I'd imagine this would involve declaring a > boolean variable in function scope, asserting on it here, and then changing its > state.) Done. https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:160: return ERR_CONTENT_DECODING_FAILED; On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > nit, suggestion: Is there a reason this isn't up above before all the arithmetic > around how much has been consumed and produced, which I sorta presume isn't > valid if inflate() returned an error? Done. https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:172: input_state_ = STATE_UNCOMPRESSED_BODY; On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > NumGzipFooterBytesToSkip() handles being called repeatedly, but this code > unilaterally transitioning to STATE_UNCOMPRESSED_BODY seems to assume that the > footer will always fit into a single input buffer. Shouldn't this transition > only occur if gzip_footer_bytes_left_ == 0? > > (Anticipated future comment: I'd rather not read&set gzip_footer_bytes_left_ in > NumGzip_FooterBytesToSkip and also read it here; that feels like a (minor) > layering violation. So if you solve this the obvious way and test > gzip_footer_bytes_left_ in evaluating the state transition, could you also > inline NumGzipFooterBytesToSkip()? Or alternatively simply transition and break > if NumGzipFooterBytesToSkip() returns 0?) Done. Good catch. This is a bug. The newly added SmallOutputBuffer test will catch this. https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream.cc:176: int to_copy = std::min(input_data_size, output_buffer_size - bytes_out); On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > Suggestion: I don't know if it's worth it, but if you wanted to, you could do > the same thing with the output buffer/size you've done with the input buffer & > size and have variables that track the current place to write to and the amount > of space that's available to write. (It would be a different way of addressing > my assertion concern above about only going through STATE_COMPRESSED_BODY once > per call, because you'd be updating the output pointers.) Acknowledged. The new code is less clustered. |output_buffer_size| is only used in three places. Since we already have a |bytes_out| so I didn't add the variables. https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... File net/filter/gzip_source_stream_unittest.cc (right): https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... net/filter/gzip_source_stream_unittest.cc:316: TEST_F(GzipSourceStreamTest, GzipCorrectnessWithSmallOutputBuffer) { On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > Parallel test with a small input buffer? (I.e. drib in a byte at a time?) Done.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/26 15:29:43, xunjieli wrote: > Thanks a lot for the review! I am embarrassed at how many bugs there are :( Hope > this round is slight better! PTAL. Don't worry about it. This is hard stuff--there are a *lot* of corner case details to get right. That's part of why I'm reviewing as closely as I am, and part of why I'm pushing so hard for as simple code as possible--it brings more bugs to light. This round LGTM. Thanks very much for your patience. (I still totally expect to find problems when it gets released :-}.) > > https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... > File net/filter/gzip_source_stream.cc (right): > > https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... > net/filter/gzip_source_stream.cc:98: if (ShouldFallbackToPlain(input_data[0])) { > On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > > Suggestion: I'll call out that this is assuming that input data is non-zero, > > which will always be true because of the combination of a) the conditional > about > > input_buffer_size == 0 at the top of the function, and b) the fact that no > other > > state enters STATE_START. I'm finding myself wondering if that's worth either > a > > comment or a DCHECK, but I'm not sure. Up to you. > > Done. I added a DCHECK. > > https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... > net/filter/gzip_source_stream.cc:118: input_data_size -= bytes_consumed; > On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > > If this drops input_data_size to zero, we want to return, correct? I think > the > > best way to handle that is by pulling the "if (input_buffer_size == 0)" > > conditional inside the while loop and making it on input_data_size. > > > > Actually, isn't that always true for INCOMPLETE_HEADER, above? Does that > > suggest a hole in testing (i.e. that there's no test that triggers the > > INCOMPLETE_HEADER return from GzipHeader::ReadMore())? > > Done. This is a bug! Thanks for catching it. There's a test in > url_request_job_unittest.cc (SlowFilterRead), but not here. I have added a test. > > https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... > net/filter/gzip_source_stream.cc:130: zlib_stream_.get()->avail_out = > output_buffer_size; > On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > > Suggestion: The fact that {next,avail}_out are being set directly from the > > output buffer passed into the function is assuming that the > > STATE_COMPRESSED_BODY element in the switch statement will never be visited > more > > than once for a single invocation of FilterData(). I believe that that > > assumption is both based on the fact that we never leave this block with a > > simple "break;"; we either return or set a new state and break. But I find > > myself a bit uncomfortable with that chain of assumptions (even though I think > > it's a valid chain). How would you feel about having a DCHECK at the top of > > this clause asserting that this is the only time for this function invocation > > the clause has been entered? (I'd imagine this would involve declaring a > > boolean variable in function scope, asserting on it here, and then changing > its > > state.) > > Done. > > https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... > net/filter/gzip_source_stream.cc:160: return ERR_CONTENT_DECODING_FAILED; > On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > > nit, suggestion: Is there a reason this isn't up above before all the > arithmetic > > around how much has been consumed and produced, which I sorta presume isn't > > valid if inflate() returned an error? > > Done. > > https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... > net/filter/gzip_source_stream.cc:172: input_state_ = STATE_UNCOMPRESSED_BODY; > On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > > NumGzipFooterBytesToSkip() handles being called repeatedly, but this code > > unilaterally transitioning to STATE_UNCOMPRESSED_BODY seems to assume that the > > footer will always fit into a single input buffer. Shouldn't this transition > > only occur if gzip_footer_bytes_left_ == 0? > > > > (Anticipated future comment: I'd rather not read&set gzip_footer_bytes_left_ > in > > NumGzip_FooterBytesToSkip and also read it here; that feels like a (minor) > > layering violation. So if you solve this the obvious way and test > > gzip_footer_bytes_left_ in evaluating the state transition, could you also > > inline NumGzipFooterBytesToSkip()? Or alternatively simply transition and > break > > if NumGzipFooterBytesToSkip() returns 0?) > > Done. Good catch. This is a bug. The newly added SmallOutputBuffer test will > catch this. > > https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... > net/filter/gzip_source_stream.cc:176: int to_copy = std::min(input_data_size, > output_buffer_size - bytes_out); > On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > > Suggestion: I don't know if it's worth it, but if you wanted to, you could do > > the same thing with the output buffer/size you've done with the input buffer & > > size and have variables that track the current place to write to and the > amount > > of space that's available to write. (It would be a different way of > addressing > > my assertion concern above about only going through STATE_COMPRESSED_BODY once > > per call, because you'd be updating the output pointers.) > > Acknowledged. The new code is less clustered. |output_buffer_size| is only used > in three places. Since we already have a |bytes_out| so I didn't add the > variables. > > https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... > File net/filter/gzip_source_stream_unittest.cc (right): > > https://codereview.chromium.org/2334773002/diff/180001/net/filter/gzip_source... > net/filter/gzip_source_stream_unittest.cc:316: TEST_F(GzipSourceStreamTest, > GzipCorrectnessWithSmallOutputBuffer) { > On 2016/09/23 20:53:47, Randy Smith - Not in Mondays wrote: > > Parallel test with a small input buffer? (I.e. drib in a byte at a time?) > > Done.
Thanks a lot for the review! Much appreciated.
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add net::GzipSourceStream This CL adds a net::GzipSourceStream which implements net::FilterSourceStream to do gzip decoding. This is a part of the efforts to convert net::Filter into a pull-based interface. See the linked bug for more details. BUG=474859 ========== to ========== Add net::GzipSourceStream This CL adds a net::GzipSourceStream which implements net::FilterSourceStream to do gzip decoding. This is a part of the efforts to convert net::Filter into a pull-based interface. See the linked bug for more details. BUG=474859 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add net::GzipSourceStream This CL adds a net::GzipSourceStream which implements net::FilterSourceStream to do gzip decoding. This is a part of the efforts to convert net::Filter into a pull-based interface. See the linked bug for more details. BUG=474859 ========== to ========== Add net::GzipSourceStream This CL adds a net::GzipSourceStream which implements net::FilterSourceStream to do gzip decoding. This is a part of the efforts to convert net::Filter into a pull-based interface. See the linked bug for more details. BUG=474859 Committed: https://crrev.com/89be8ae4a98355e81372cc35dab5e125913c5e3f Cr-Commit-Position: refs/heads/master@{#421314} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/89be8ae4a98355e81372cc35dab5e125913c5e3f Cr-Commit-Position: refs/heads/master@{#421314} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
