Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1882043003:
Blimp: add padding for zlib framing to buffer preallocation calculations. (Closed)
|
Created:
4 years, 8 months ago by Kevin M Modified:
4 years, 8 months ago Reviewers:
Wez CC:
chromium-reviews, cbentzel+watch_chromium.org, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp: add padding for zlib framing to buffer preallocation calculations.
Without the padding, messages with very low compression rates can exceed
the size of the preallocated buffer, causing some trailing bytes to be
written to subsequent messages.
R=wez@chromium.org
BUG=603213
Committed: https://crrev.com/581a3f83eeb9241f698095f552813c355f2426c2
Cr-Commit-Position: refs/heads/master@{#387152}
Patch Set 1 #
Total comments: 10
Patch Set 2 : wez feedback #Patch Set 3 : wez feedback #
Total comments: 1
Patch Set 4 : Use RandBytesAsString() in unit tests. #
Messages
Total messages: 24 (11 generated)
Just some nits to iron out, basically. https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_unittest.cc (right): https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_unittest.cc:25: const char kLargeIncompressiblePayload[] = Would it work for us to replace this with an equivalent sized stream of pseudo-random bytes (e.g. by setting a seed and calling rand()...)? https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_unittest.cc:140: LOG(ERROR) << "Compressed size: " << compressed_buf->BytesRemaining(); Ick! Nasty, noisy test! Shoo! Shoo! https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_unittest.cc:162: LOG(ERROR) << "Decompressed size: " << size; Out, damned LOG(ERROR)! Out, I tell you! https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_writer.cc (right): https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_writer.cc:25: const size_t kZlibBlockFooterLength = 4; nit: You call it a marker but name it "footer" - be consistent. :) Similarly, don't we use "size" to refer to a number-of-bytes, normally, rather than length? https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_writer.cc:99: kZlibBlockFooterLength; Indentation looks wrong - git cl format this?
https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_unittest.cc (right): https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_unittest.cc:25: const char kLargeIncompressiblePayload[] = On 2016/04/12 23:54:34, Wez wrote: > Would it work for us to replace this with an equivalent sized stream of > pseudo-random bytes (e.g. by setting a seed and calling rand()...)? Done. https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_unittest.cc:140: LOG(ERROR) << "Compressed size: " << compressed_buf->BytesRemaining(); On 2016/04/12 23:54:34, Wez wrote: > Ick! Nasty, noisy test! Shoo! Shoo! Done. https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_unittest.cc:162: LOG(ERROR) << "Decompressed size: " << size; On 2016/04/12 23:54:34, Wez wrote: > Out, damned LOG(ERROR)! Out, I tell you! Done. https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... File blimp/net/compressed_packet_writer.cc (right): https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_writer.cc:25: const size_t kZlibBlockFooterLength = 4; On 2016/04/12 23:54:34, Wez wrote: > nit: You call it a marker but name it "footer" - be consistent. :) > > Similarly, don't we use "size" to refer to a number-of-bytes, normally, rather > than length? Done, done. https://codereview.chromium.org/1882043003/diff/1/blimp/net/compressed_packet... blimp/net/compressed_packet_writer.cc:99: kZlibBlockFooterLength; On 2016/04/12 23:54:34, Wez wrote: > Indentation looks wrong - git cl format this? Already formatted.
The CQ bit was checked by wez@chromium.org
lgtm https://codereview.chromium.org/1882043003/diff/40001/blimp/net/compressed_pa... File blimp/net/compressed_packet_unittest.cc (right): https://codereview.chromium.org/1882043003/diff/40001/blimp/net/compressed_pa... blimp/net/compressed_packet_unittest.cc:164: // Write two incompressible (high entropy) blocks. nit: Why two?
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882043003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882043003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
Description was changed from ========== Blimp: add padding for zlib framing to buffer preallocation calculations. Without the padding, messages with very low compression rates can exceed the size of the preallocated buffer, causing some trailing bytes to be written to subsequent messages. R=wez@chromium.org BUG= ========== to ========== Blimp: add padding for zlib framing to buffer preallocation calculations. Without the padding, messages with very low compression rates can exceed the size of the preallocated buffer, causing some trailing bytes to be written to subsequent messages. R=wez@chromium.org BUG=603213 ==========
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wez@chromium.org Link to the patchset: https://codereview.chromium.org/1882043003/#ps60001 (title: "Use RandBytesAsString() in unit tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882043003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882043003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882043003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882043003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by kmarshall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1882043003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1882043003/60001
Message was sent while issue was closed.
Description was changed from ========== Blimp: add padding for zlib framing to buffer preallocation calculations. Without the padding, messages with very low compression rates can exceed the size of the preallocated buffer, causing some trailing bytes to be written to subsequent messages. R=wez@chromium.org BUG=603213 ========== to ========== Blimp: add padding for zlib framing to buffer preallocation calculations. Without the padding, messages with very low compression rates can exceed the size of the preallocated buffer, causing some trailing bytes to be written to subsequent messages. R=wez@chromium.org BUG=603213 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Blimp: add padding for zlib framing to buffer preallocation calculations. Without the padding, messages with very low compression rates can exceed the size of the preallocated buffer, causing some trailing bytes to be written to subsequent messages. R=wez@chromium.org BUG=603213 ========== to ========== Blimp: add padding for zlib framing to buffer preallocation calculations. Without the padding, messages with very low compression rates can exceed the size of the preallocated buffer, causing some trailing bytes to be written to subsequent messages. R=wez@chromium.org BUG=603213 Committed: https://crrev.com/581a3f83eeb9241f698095f552813c355f2426c2 Cr-Commit-Position: refs/heads/master@{#387152} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/581a3f83eeb9241f698095f552813c355f2426c2 Cr-Commit-Position: refs/heads/master@{#387152} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
