This launch bug has not been modified in the last 90 days and has no milestone label. Please take a look and add appropriate milestone label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Going to unassign this from me, as I'm not currently working on it, and the trial has expired.  Not enough time.  :(

This launch bug has not been modified in the last 90 days and has no milestone label. Please take a look and add appropriate milestone label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This launch bug has not been modified in the last 90 days and has no milestone label. Please take a look and add appropriate milestone label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

davidben@ has done related work in  Issue 524258 . Copied and pasted below for reference. 

Should we close/merge this one? 

The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/0f5d7d3f042a9ab5b9cf63534a1b1de5396a4fe8

commit 0f5d7d3f042a9ab5b9cf63534a1b1de5396a4fe8
Author: David Benjamin <davidben@google.com>
Date: Mon Mar 27 16:37:53 2017

Just allocate what's needed for SSL write buffers.

When we refactored all the buffering logic, we retained upstream
OpenSSL's allocation patterns. In particular, we always allocated fixed
size write buffer, even though, unlike when reading, we trivially know a
tighter bound (namely however much we happen to be writing right now).

Since the cutoff for when Windows' malloc starts having a hard time is
just below the TLS maximum record size, do the more natural thing of
allocating what we need to hold outgoing ciphertext.

(This only does anything to the write half. Read half is a bit more
involved.)

BUG= chromium:524258 

Change-Id: I0165f9ce822b9cc413f3c77e269e6154160537a7
Reviewed-on: https://boringssl-review.googlesource.com/14405
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

[modify] https://crrev.com/0f5d7d3f042a9ab5b9cf63534a1b1de5396a4fe8/ssl/ssl_buffer.c


The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/3120950b1e27635ee9b9d167052ce11ce9c96fd4

commit 3120950b1e27635ee9b9d167052ce11ce9c96fd4
Author: David Benjamin <davidben@google.com>
Date: Fri Jun 23 23:08:35 2017

Size TLS read buffers based on the size requested.

Like the write half, rather than allocating the maximum size needed and
relying on the malloc implementation to pool this sanely, allocate the
size the TLS record-layer code believes it needs.

As currently arranged, this will cause us to alternate from a small
allocation (for the record header) and then an allocation sized to the
record itself. Windows is reportedly bad at pooling large allocations,
so, *if the server sends us smaller records*, this will avoid hitting
the problem cases.

If the server sends us size 16k records, the maximum allowed by ther
protocol, we simply must buffer up to that amount and will continue to
allocate similar sizes as before (although slightly smaller; this CL
also fixes small double-counting we did on the allocation sizes).

Separately, we'll gather some metrics in Chromium to see what common
record sizes are to determine if this optimization is sufficient. This
is intended as an easy optimization we can do now, in advance of ongoing
work to fix the extra layer of buffering between Chromium and BoringSSL
with an in-place decrypt API.

Bug:  chromium:524258 
Change-Id: I233df29df1212154c49fee4285ccc37be12f81dc
Reviewed-on: https://boringssl-review.googlesource.com/17329
Reviewed-by: Adam Langley <agl@google.com>

[modify] https://crrev.com/3120950b1e27635ee9b9d167052ce11ce9c96fd4/ssl/ssl_buffer.c

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a961f5f372eef024f722a49f3e921c62fb2f9f3a

commit a961f5f372eef024f722a49f3e921c62fb2f9f3a
Author: David Benjamin <davidben@chromium.org>
Date: Wed Jun 28 13:40:16 2017

Histogram the sizes of received TLS records.

This will help us determine which cases to optimize for.

Bug:  524258 
Change-Id: Ie04aa6c09499a49058eff84006f2e173e6356d83
Reviewed-on: https://chromium-review.googlesource.com/545058
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Steven Valdez <svaldez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482967}
[modify] https://crrev.com/a961f5f372eef024f722a49f3e921c62fb2f9f3a/net/socket/ssl_client_socket_impl.cc
[modify] https://crrev.com/a961f5f372eef024f722a49f3e921c62fb2f9f3a/tools/metrics/histograms/histograms.xml

No, those are not the same buffers. See the comments I left on the other bug explaining the two layers of buffers and the work underway to merge them.

This launch bug has not been modified in the last 90 days and has no milestone label. Please take a look and add appropriate milestone label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This launch bug has not been modified in the last 90 days and has no milestone label. Please take a look and add appropriate milestone label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This launch bug has not been modified in the last 90 days and has no milestone label. Please take a look and add appropriate milestone label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Updating launch bugs to convert to new launch process in go/newChromeFeature.  Automatically setting Launch-Exp-Leadership-NA since this appears to be a legacy technical / simple launch (all existing cross-functional bits are set to NA).  Note that in the new process, no review / approval for technical / simple launches is required (though please be sure you are confident there is no cross-functional impact here).  Contact amineer@ with any questions or concerns.

Updating launch bugs to convert to new launch process in go/newChromeFeature.  Automatically setting Launch--Leadership-NA since this appears to be a legacy technical / simple launch (all existing cross-functional bits are set to NA).  Note that in the new process, no review / approval for technical / simple launches is required (though please be sure you are confident there is no cross-functional impact here).  Contact amineer@ with any questions or concerns.

Assigning to xunjieli to figure out next steps - think we can archive this, but may need to remove histograms.

I am archiving this. The chromium side buffers are aggressively deallocated after use since  Issue 690915 . I haven't received reports saying these Chromium-side SSL buffers are a memory problem. The histograms added by davidben@ in r482967 tracks the lower-level buffers, and are orthogonal to this bug.

Those histograms have also since been erasered I believe.

If this is archived, can we unwind the SSLBufferSizeRecv and SSLBufferSizeSend Finch hooks? I think you all added them for this.

Thanks, David! Good catch. Let me upload a CL to remove them.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3aa9624735f82c32491f667af5aa7fa975e857b

commit f3aa9624735f82c32491f667af5aa7fa975e857b
Author: Helen Li <xunjieli@chromium.org>
Date: Thu May 24 00:18:07 2018

Remove unused field trial from ssl_client_socket_impl.cc

These two field trials are not actively being worked on. It's unclear whether
tuning the buffer size is still relevant given the other changes (with respect
to socket buffers) that landed since. This CL removes the field trials from the
code.

Bug:  631140 
Change-Id: If4fc665551f76d35f36b702055fef64499e4e582
Reviewed-on: https://chromium-review.googlesource.com/1070650
Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561323}
[modify] https://crrev.com/f3aa9624735f82c32491f667af5aa7fa975e857b/net/socket/ssl_client_socket_impl.cc