Instrument SSL sockets using MemoryDumpProvider

Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using
MemoryDumpProvider. Design doc is linked in the crbug.

BUG=669108
Committed: https://crrev.com/9f8c5fb56da88f719d36423e58e8f9f89b32d542
Cr-Commit-Position: refs/heads/master@{#437097}

[xunjieli, 2016-12-01 22:51:52]

Description was changed from

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using MemoryDumpProvider.

BUG=669108
==========

to

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using 
MemoryDumpProvider. The overral approach can be seen in
crrev.com/2525743002. Design doc is linked in the crbug.

BUG=669108
==========

[xunjieli, 2016-12-01 22:54:06]

Description was changed from

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using 
MemoryDumpProvider. The overral approach can be seen in
crrev.com/2525743002. Design doc is linked in the crbug.

BUG=669108
==========

to

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using 
MemoryDumpProvider. The overall approach can be seen in
crrev.com/2525743002. Design doc is linked in the crbug.

BUG=669108
==========

[xunjieli, 2016-12-01 23:26:14]

xunjieli@chromium.org changed reviewers:
+ davidben@chromium.org, eroman@chromium.org

[xunjieli, 2016-12-01 23:26:14]

Hi Eric and David, PTAL. Thanks!
Hopefully this is easier for review. Currently I only have one test that makes
sure dumping memory does not crash. I don't have more granular tests. Let me
know if you feel strongly about adding those in this CL. I expect the format
will evolve a quite bit since we are still figuring out what to instrument and
what to keep in the long run.

[eroman, 2016-12-02 00:27:24]

I suggest removing the reference to the (uncommitted) codereview from the CL
description.

That reference can be left in a codereview message, however may not be relevant
as a final commit message.

https://codereview.chromium.org/2541093003/diff/60001/net/socket/client_socke...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/2541093003/diff/60001/net/socket/client_socke...
net/socket/client_socket_pool_base.cc:709: const std::list<IdleSocket> sockets =
it->second->idle_sockets();
Looks like there are some unnecessary copies here. (a) copying the sockets list
(b) making copies of the entries when iterating below.

https://codereview.chromium.org/2541093003/diff/60001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2541093003/diff/60001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache.cc:92: base::AutoLock lock(lock_);
Can the lock acquisition be moved to after CreateAllocatorDump() ?

[xunjieli, 2016-12-02 14:39:32]

Description was changed from

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using 
MemoryDumpProvider. The overall approach can be seen in
crrev.com/2525743002. Design doc is linked in the crbug.

BUG=669108
==========

to

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using 
MemoryDumpProvider. Design doc is linked in the crbug.

BUG=669108
==========

[xunjieli, 2016-12-02 15:31:34]

Thanks! PTAL

https://codereview.chromium.org/2541093003/diff/60001/net/socket/client_socke...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/2541093003/diff/60001/net/socket/client_socke...
net/socket/client_socket_pool_base.cc:709: const std::list<IdleSocket> sockets =
it->second->idle_sockets();
On 2016/12/02 00:27:24, eroman (slow) wrote:
> Looks like there are some unnecessary copies here. (a) copying the sockets
list
> (b) making copies of the entries when iterating below.

oops, sorry. Done.

https://codereview.chromium.org/2541093003/diff/60001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2541093003/diff/60001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache.cc:92: base::AutoLock lock(lock_);
On 2016/12/02 00:27:24, eroman (slow) wrote:
> Can the lock acquisition be moved to after CreateAllocatorDump() ?

Done.

[xunjieli, 2016-12-02 23:09:43]

xunjieli@chromium.org changed reviewers:
+ ssid@chromium.org

[xunjieli, 2016-12-02 23:09:43]

+ ssid@:
I have addressed your comment to use PMD + parent absolute name approach instead
of passing MAD.
I might have missed some other things. I will get back to this next Monday. 

Have a great weekend everyone.

[xunjieli, 2016-12-05 17:49:48]

xunjieli@chromium.org changed reviewers:
+ primiano@chromium.org

[xunjieli, 2016-12-05 17:49:48]

+ primiano@
Primiano: Sid is out. Could you take a look at the CL?

[xunjieli, 2016-12-05 23:29:30]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 23:30:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 00:34:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 00:34:14]

Dry run: This issue passed the CQ dry run.

[davidben, 2016-12-06 01:02:07]

Sorry about the delay. I kept getting side-tracked today. Just looked at the
SSL-related files:

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
net/socket/ssl_client_socket_impl.cc:834: total_cert_size);
Just to confirm: this will not measure the amount of memory used by the
certificate, but the lower bound size of the serialized certificate itself. Is
this what you want? (As we just chatted about out-of-band, OpenSSL's X.509
representation blows it up by around 4x.) You're only going to record the lower
bound number, not the 4x number (which is probably a nuisance to record).

Assuming you're okay with what this is measuring, probably still worth a comment
about this. You can cite https://crbug.com/671420 if you like.

(I wonder if this should be named serialized_cert_size or something to be
clearer?)

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3707: parent_dump2->absolute_name());
I'm not familiar with the MemoryAllocatorDump stuff... what are "parent2" and
"parent3" testing that "parent1" didn't test?

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3724: for (const auto& it :
allocator_dumps) {
Super nitpicky nitpick: it => pair? it is usually for iterators and this is a
key/value pair, not an iterator.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:7: #include <openssl/ssl.h>
Already included below.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:8: #include <openssl/x509.h>
#include "third_party/boringssl/src/include/openssl/x509.h"

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:96: // a singleton, only log memory stats
once.
Could you link to https://crbug.com/458365? That's the bug to fix this
ridiculous singleton.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:98: return;
How does this interact with multiple threads? (I'm not familiar with this API.)
The SSLClientSessionCache is shared between all URLRequestContexts, regardless
of which thread they're on.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:102: while (iter != cache_.end()) {
Does this work?

  for (const auto& pair : cache_) {

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:114: entry_dump->AddScalar("cert_size",
Same comment as on ssl_client_socket_impl.cc

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
File net/ssl/ssl_client_session_cache_unittest.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache_unittest.cc:332: for (const auto& it :
allocator_dumps) {
Super nitpicky nitpick: it => pair? it is usually for iterators and this is a
key/value pair, not an iterator.

[Primiano Tucci (use gerrit), 2016-12-06 17:06:28]

> Primiano: Sid is out. Could you take a look at the CL? 
Sure. I have only one comment (+2 micro ones).
All the rest looks good to me.

https://codereview.chromium.org/2541093003/diff/140001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/http/http_network_...
net/http/http_network_session.cc:394:
pmd->AddOwnershipEdge(pmd->GetAllocatorDump(parent_absolute_name)->guid(),
Hmm I think this is not going to make anything useful (if not hit a DCHECK).
What is the intented result of this?

The part where you check if network_session_X exists and early out is okay. But
creating two edges between the same nodes doesn't make lot of sense.

https://codereview.chromium.org/2541093003/diff/140001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:708: for (GroupMap::const_iterator it =
group_map_.begin(); it != group_map_.end();
for (const auto& kv : group_map) might save one line and make it a little bit
more readable :)

https://codereview.chromium.org/2541093003/diff/140001/net/socket/socket_bio_...
File net/socket/socket_bio_adapter.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/socket/socket_bio_...
net/socket/socket_bio_adapter.cc:57: size_t effective_size = 0;
just a super minor comment: in memory-infra context we use the term
"effective_size" to refer to (memory - aliasing) (see the various
AddOwnershipEdge). Using the same name here might generate a bit of confusion.
Can this be named just size, or real_size, used_size or whatever !=
effective_size? :)

[xunjieli, 2016-12-06 18:36:09]

Thanks a lot for the reviews! I have uploaded a new patch to address the
comments. PTAL.

https://codereview.chromium.org/2541093003/diff/140001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/http/http_network_...
net/http/http_network_session.cc:394:
pmd->AddOwnershipEdge(pmd->GetAllocatorDump(parent_absolute_name)->guid(),
On 2016/12/06 17:06:28, Primiano Tucci wrote:
> Hmm I think this is not going to make anything useful (if not hit a DCHECK).
> What is the intented result of this?
> 
> The part where you check if network_session_X exists and early out is okay.
But
> creating two edges between the same nodes doesn't make lot of sense.

No, it won't ever create two edges between the same nodes.

Say we have context1 and context2, which share a http_network_session.
context1->OnMemoryDump() calls http_network_session::DumpMemoryStats() and adds
an edge between context1 and http_network_session.
context2->OnMemoryDump() will add an edge between context2 and
http_network_session.

https://codereview.chromium.org/2541093003/diff/140001/net/socket/client_sock...
File net/socket/client_socket_pool_base.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/socket/client_sock...
net/socket/client_socket_pool_base.cc:708: for (GroupMap::const_iterator it =
group_map_.begin(); it != group_map_.end();
On 2016/12/06 17:06:28, Primiano Tucci wrote:
> for (const auto& kv : group_map) might save one line and make it a little bit
> more readable :)

Done.

https://codereview.chromium.org/2541093003/diff/140001/net/socket/socket_bio_...
File net/socket/socket_bio_adapter.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/socket/socket_bio_...
net/socket/socket_bio_adapter.cc:57: size_t effective_size = 0;
On 2016/12/06 17:06:28, Primiano Tucci wrote:
> just a super minor comment: in memory-infra context we use the term
> "effective_size" to refer to (memory - aliasing) (see the various
> AddOwnershipEdge). Using the same name here might generate a bit of confusion.
> Can this be named just size, or real_size, used_size or whatever !=
> effective_size? :)

Done.

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
net/socket/ssl_client_socket_impl.cc:834: total_cert_size);
On 2016/12/06 01:02:07, davidben wrote:
> Just to confirm: this will not measure the amount of memory used by the
> certificate, but the lower bound size of the serialized certificate itself. Is
> this what you want? (As we just chatted about out-of-band, OpenSSL's X.509
> representation blows it up by around 4x.) You're only going to record the
lower
> bound number, not the 4x number (which is probably a nuisance to record).
> 
> Assuming you're okay with what this is measuring, probably still worth a
comment
> about this. You can cite https://crbug.com/671420 if you like.
> 
> (I wonder if this should be named serialized_cert_size or something to be
> clearer?)

Done.  That's a very important distinction -- thanks for bring it up. I don't
think we can easily measure the actual allocation incurred for the X509, can we?
We don't want to spend much overhead when MemoryDumpProvider is not enabled. If
there's a more accurate way with minimum overhead, I'd like to explore that.

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3707: parent_dump2->absolute_name());
On 2016/12/06 01:02:07, davidben wrote:
> I'm not familiar with the MemoryAllocatorDump stuff... what are "parent2" and
> "parent3" testing that "parent1" didn't test?

Done. I tried to dump memory multiple times to be sure that there is no crash.
This is probably an overkill. I eliminated one and adjusted the test to check
that |buffer_size| is different after request finishes. A ProcessMemoryDump
doesn't allow having two MemoryAllocatorDumps with the same name, so I added a
different prefix each time. 

I also added a comment.

https://codereview.chromium.org/2541093003/diff/140001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3724: for (const auto& it :
allocator_dumps) {
On 2016/12/06 01:02:07, davidben wrote:
> Super nitpicky nitpick: it => pair? it is usually for iterators and this is a
> key/value pair, not an iterator.

Done.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:7: #include <openssl/ssl.h>
On 2016/12/06 01:02:07, davidben wrote:
> Already included below.

Done.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:8: #include <openssl/x509.h>
On 2016/12/06 01:02:07, davidben wrote:
> #include "third_party/boringssl/src/include/openssl/x509.h"

Done.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:96: // a singleton, only log memory stats
once.
On 2016/12/06 01:02:07, davidben wrote:
> Could you link to https://crbug.com/458365 That's the bug to fix this
> ridiculous singleton.

Done.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:98: return;
On 2016/12/06 01:02:07, davidben wrote:
> How does this interact with multiple threads? (I'm not familiar with this
API.)
> The SSLClientSessionCache is shared between all URLRequestContexts, regardless
> of which thread they're on.

Right, this came up in the design doc discussion as well. I think we are going
to depend on |lock_| for now and change this once 458365 is fixed. I tested this
locally. I don't see any weird issue.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:102: while (iter != cache_.end()) {
On 2016/12/06 01:02:07, davidben wrote:
> Does this work?
> 
>   for (const auto& pair : cache_) {

Done. Yep it works. Copied from SSLClientSessionCache::FlushExpiredSessions().
I will change this to const auto& pair.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:114: entry_dump->AddScalar("cert_size",
On 2016/12/06 01:02:07, davidben wrote:
> Same comment as on ssl_client_socket_impl.cc

Done.

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
File net/ssl/ssl_client_session_cache_unittest.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache_unittest.cc:332: for (const auto& it :
allocator_dumps) {
On 2016/12/06 01:02:07, davidben wrote:
> Super nitpicky nitpick: it => pair? it is usually for iterators and this is a
> key/value pair, not an iterator.

Done.

[xunjieli, 2016-12-06 18:41:46]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 18:42:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 20:01:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 20:01:28]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-12-06 20:15:41]

https://codereview.chromium.org/2541093003/diff/140001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/http/http_network_...
net/http/http_network_session.cc:394:
pmd->AddOwnershipEdge(pmd->GetAllocatorDump(parent_absolute_name)->guid(),
On 2016/12/06 18:36:08, xunjieli wrote:
> On 2016/12/06 17:06:28, Primiano Tucci wrote:
> > Hmm I think this is not going to make anything useful (if not hit a DCHECK).
> > What is the intented result of this?
> > 
> > The part where you check if network_session_X exists and early out is okay.
> But
> > creating two edges between the same nodes doesn't make lot of sense.
> 
> No, it won't ever create two edges between the same nodes.
> 
> Say we have context1 and context2, which share a http_network_session.
> context1->OnMemoryDump() calls http_network_session::DumpMemoryStats() and
adds
> an edge between context1 and http_network_session.
> context2->OnMemoryDump() will add an edge between context2 and
> http_network_session.
> 

We had an offline chat on this. I dumped my brain on
https://docs.google.com/document/d/1-dmr_dVgO090Y5_3K9itjsE0MsBTmbqwovu6lup1i...

The rest of this CL makes sense to me, and I think we are aligned on two
possible options here.
My suggestion is to go ahead and land this regardless of this conversation (%
net owners being okay with the rest of the code) in order to avoid a cross
timezone round-trip-time.
Even in the worst case this is the kind of thing that can be fixed quite easily
in a follow-up.
So speculatively LGTM from the memory-reporting viewpoint.

Thanks a lot for working on this, It's quite conceptually challenging (also fun)
:)

[xunjieli, 2016-12-06 21:22:12]

https://codereview.chromium.org/2541093003/diff/140001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/2541093003/diff/140001/net/http/http_network_...
net/http/http_network_session.cc:394:
pmd->AddOwnershipEdge(pmd->GetAllocatorDump(parent_absolute_name)->guid(),
On 2016/12/06 20:15:41, Primiano Tucci wrote:
> On 2016/12/06 18:36:08, xunjieli wrote:
> > On 2016/12/06 17:06:28, Primiano Tucci wrote:
> > > Hmm I think this is not going to make anything useful (if not hit a
DCHECK).
> > > What is the intented result of this?
> > > 
> > > The part where you check if network_session_X exists and early out is
okay.
> > But
> > > creating two edges between the same nodes doesn't make lot of sense.
> > 
> > No, it won't ever create two edges between the same nodes.
> > 
> > Say we have context1 and context2, which share a http_network_session.
> > context1->OnMemoryDump() calls http_network_session::DumpMemoryStats() and
> adds
> > an edge between context1 and http_network_session.
> > context2->OnMemoryDump() will add an edge between context2 and
> > http_network_session.
> > 
> 
> We had an offline chat on this. I dumped my brain on
>
https://docs.google.com/document/d/1-dmr_dVgO090Y5_3K9itjsE0MsBTmbqwovu6lup1i...
> 
> The rest of this CL makes sense to me, and I think we are aligned on two
> possible options here.
> My suggestion is to go ahead and land this regardless of this conversation (%
> net owners being okay with the rest of the code) in order to avoid a cross
> timezone round-trip-time.
> Even in the worst case this is the kind of thing that can be fixed quite
easily
> in a follow-up.
> So speculatively LGTM from the memory-reporting viewpoint.
> 
> Thanks a lot for working on this, It's quite conceptually challenging (also
fun)
> :)

Thanks for the follow-up! I tried your suggestion (Option #2), but I don't think
the UI gives what we want. I will keep this as it is and continue this
discussion via email.

[xunjieli, 2016-12-07 14:50:22]

Hi Eric and David, this CL is ready for a second round of review. Thank you!

[davidben, 2016-12-07 19:42:59]

SSL-related files lgtm with minor comments.

https://codereview.chromium.org/2541093003/diff/160001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2541093003/diff/160001/net/socket/ssl_client_...
net/socket/ssl_client_socket_impl.cc:838:
socket_dump->AddScalar("serialized_cert_count",
Probably just cert_count. The number of certificates is a perfectly meaningful
number regardless of how we represent them. :-)

https://codereview.chromium.org/2541093003/diff/160001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/2541093003/diff/160001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3745: // Buffer size should take into
account the read buffer size.
Perhaps:

// The buffers should be released if the transport is idle.

https://codereview.chromium.org/2541093003/diff/160001/net/ssl/ssl_client_ses...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2541093003/diff/160001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:113:
entry_dump->AddScalar("serialized_cert_size",
Probably worth replicating the comment here, so it's clear looking at this file
what's up.

https://codereview.chromium.org/2541093003/diff/160001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:116:
entry_dump->AddScalar("serialized_cert_count",
Ditto re serialized_cert_count => cert_count

[xunjieli, 2016-12-07 20:32:25]

David, I have one question below. PTAL.

https://codereview.chromium.org/2541093003/diff/160001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2541093003/diff/160001/net/socket/ssl_client_...
net/socket/ssl_client_socket_impl.cc:838:
socket_dump->AddScalar("serialized_cert_count",
On 2016/12/07 19:42:58, davidben wrote:
> Probably just cert_count. The number of certificates is a perfectly meaningful
> number regardless of how we represent them. :-)

Done.

https://codereview.chromium.org/2541093003/diff/160001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/2541093003/diff/160001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3745: // Buffer size should take into
account the read buffer size.
On 2016/12/07 19:42:58, davidben wrote:
> Perhaps:
> 
> // The buffers should be released if the transport is idle.

But in this case, the buffer size is not equals to 0. Should I say something
like: "The read buffer is not released, so |buffer_size| can't be 0" ?

https://codereview.chromium.org/2541093003/diff/160001/net/ssl/ssl_client_ses...
File net/ssl/ssl_client_session_cache.cc (right):

https://codereview.chromium.org/2541093003/diff/160001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:113:
entry_dump->AddScalar("serialized_cert_size",
On 2016/12/07 19:42:58, davidben wrote:
> Probably worth replicating the comment here, so it's clear looking at this
file
> what's up.

Done.

https://codereview.chromium.org/2541093003/diff/160001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:116:
entry_dump->AddScalar("serialized_cert_count",
On 2016/12/07 19:42:58, davidben wrote:
> Ditto re serialized_cert_count => cert_count

Done.

[eroman, 2016-12-07 21:16:01]

lgtm

[xunjieli, 2016-12-07 21:26:32]

The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 21:27:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xunjieli, 2016-12-07 21:27:45]

Thanks David for identifying the flaky SSLClientSocket::DumpMemoryStats test and
the suggestion for how to fix it. I have uploaded a new patch. I will CQ this.

Thanks everyone for the review!

[xunjieli, 2016-12-07 22:06:58]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2016-12-07 22:07:03]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-12-07 22:07:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, davidben@chromium.org, eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2541093003/#ps220001
(title: "Fix flaky test")

[commit-bot: I haz the power, 2016-12-07 22:07:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 22:59:36]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1481148423898250,
"parent_rev": "b57cf4fcd4321e4d0face414d25026061d1a96ae", "commit_rev":
"3693b68c0d4b4e43e78b09aad802c1bf9a12eac0"}

[commit-bot: I haz the power, 2016-12-07 23:00:00]

Description was changed from

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using 
MemoryDumpProvider. Design doc is linked in the crbug.

BUG=669108
==========

to

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using 
MemoryDumpProvider. Design doc is linked in the crbug.

BUG=669108
==========

[commit-bot: I haz the power, 2016-12-07 23:00:02]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-12-07 23:03:49]

Description was changed from

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using 
MemoryDumpProvider. Design doc is linked in the crbug.

BUG=669108
==========

to

==========
Instrument SSL sockets using MemoryDumpProvider

This CL instruments SSL sockets and SSL session cache using
MemoryDumpProvider. Design doc is linked in the crbug.

BUG=669108

Committed: https://crrev.com/9f8c5fb56da88f719d36423e58e8f9f89b32d542
Cr-Commit-Position: refs/heads/master@{#437097}
==========

[commit-bot: I haz the power, 2016-12-07 23:03:53]

Patchset 12 (id:??) landed as
https://crrev.com/9f8c5fb56da88f719d36423e58e8f9f89b32d542
Cr-Commit-Position: refs/heads/master@{#437097}