Make URLRequestContext a MemoryDumpProvider (Abandoned)

Make URLRequestContext a MemoryDumpProvider

This CL makes URLRequestContext implement MemoryDumpProvider.
URLRequestContext will call into relevant classes to
gather memory usage stats via DumpMemoryStats().

This CL is mostly plumbing with the following exceptions:
(1) SSLClientSocketImpl reports cert size and buffer size
(2) SSLClientSessionCache reports cert size.
(3) SdchManager reports dictionaries size.

The goal of this CL is to lay a foundation on how the rest
of net/ could be instrumented using MemmoryDumpProvider.
Once this initial CL is reviewed and landed, there will
be follow-up CLs to instrument other parts of net/ and make
the instrumentation better.

BUG=669108

[xunjieli, 2016-11-22 21:31:17]

Patchset #1 (id:1) has been deleted

[xunjieli, 2016-11-23 21:04:46]

Patchset #2 (id:40001) has been deleted

[xunjieli, 2016-11-28 01:08:04]

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

[commit-bot: I haz the power, 2016-11-28 01:08: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-11-28 01:24:45]

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

[commit-bot: I haz the power, 2016-11-28 01:24:46]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[xunjieli, 2016-11-28 14:29:25]

Patchset #1 (id:20001) has been deleted

[xunjieli, 2016-11-28 16:04:55]

Patchset #2 (id:80001) has been deleted

[xunjieli, 2016-11-28 16:05:04]

Patchset #2 (id:100001) has been deleted

[xunjieli, 2016-11-28 16:45:04]

Patchset #2 (id:120001) has been deleted

[xunjieli, 2016-11-28 16:45:14]

Patchset #2 (id:140001) has been deleted

[xunjieli, 2016-11-28 16:45:25]

Patchset #2 (id:160001) has been deleted

[xunjieli, 2016-11-28 17:55:03]

Patchset #2 (id:180001) has been deleted

[xunjieli, 2016-11-28 17:55:13]

Patchset #2 (id:200001) has been deleted

[xunjieli, 2016-11-28 18:18:37]

Description was changed from

==========
Make URLRequestContext a MemoryDumpProvider

BUG=
==========

to

==========
Make URLRequestContext a MemoryDumpProvider

This CL makes URLRequestContext implement MemoryDumpProvider.
URLRequestContext will call into relevant classes to
gather memory usage stats via DumpMemoryStats().

This CL is mostly plumbing with the following exceptions:
(1) SSLClientSocketImpl reports cert size and buffer size
(2) SSLClientSessionCache reports cert size.
(3) SdchManager reports dictionaries size.

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

[xunjieli, 2016-11-28 18:21:40]

Description was changed from

==========
Make URLRequestContext a MemoryDumpProvider

This CL makes URLRequestContext implement MemoryDumpProvider.
URLRequestContext will call into relevant classes to
gather memory usage stats via DumpMemoryStats().

This CL is mostly plumbing with the following exceptions:
(1) SSLClientSocketImpl reports cert size and buffer size
(2) SSLClientSessionCache reports cert size.
(3) SdchManager reports dictionaries size.

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

to

==========
Make URLRequestContext a MemoryDumpProvider

This CL makes URLRequestContext implement MemoryDumpProvider.
URLRequestContext will call into relevant classes to
gather memory usage stats via DumpMemoryStats().

This CL is mostly plumbing with the following exceptions:
(1) SSLClientSocketImpl reports cert size and buffer size
(2) SSLClientSessionCache reports cert size.
(3) SdchManager reports dictionaries size.

Once this initial CL is reviewed and landed, there will
be follow-up CLs to instrument other parts of net/.

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

[xunjieli, 2016-11-28 18:24:22]

Description was changed from

==========
Make URLRequestContext a MemoryDumpProvider

This CL makes URLRequestContext implement MemoryDumpProvider.
URLRequestContext will call into relevant classes to
gather memory usage stats via DumpMemoryStats().

This CL is mostly plumbing with the following exceptions:
(1) SSLClientSocketImpl reports cert size and buffer size
(2) SSLClientSessionCache reports cert size.
(3) SdchManager reports dictionaries size.

Once this initial CL is reviewed and landed, there will
be follow-up CLs to instrument other parts of net/.

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

to

==========
Make URLRequestContext a MemoryDumpProvider

This CL makes URLRequestContext implement MemoryDumpProvider.
URLRequestContext will call into relevant classes to
gather memory usage stats via DumpMemoryStats().

This CL is mostly plumbing with the following exceptions:
(1) SSLClientSocketImpl reports cert size and buffer size
(2) SSLClientSessionCache reports cert size.
(3) SdchManager reports dictionaries size.

The goal of this CL is to lay a foundation on how the rest
of net/ could be instrumented using MemmoryDumpProvider.
Once this initial CL is reviewed and landed, there will
be follow-up CLs to instrument other parts of net/ and make
the instrumentation better.

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

[xunjieli, 2016-11-28 18:25:11]

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

[commit-bot: I haz the power, 2016-11-28 18:26:05]

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

[xunjieli, 2016-11-28 18:27:45]

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

[xunjieli, 2016-11-28 18:27:46]

This is an initial draft. 
Eric, could you take a look at the overall approach? 
David, could you take a look at SSLClientSocketImpl and SSLClientSessionCache?
Cc-ing mmenke@ FHI.
Thanks!

[commit-bot: I haz the power, 2016-11-28 20:19:31]

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

[commit-bot: I haz the power, 2016-11-28 20:19:34]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xunjieli, 2016-11-28 21:34:28]

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

[commit-bot: I haz the power, 2016-11-28 21:35:22]

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-11-28 22:30:00]

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

[commit-bot: I haz the power, 2016-11-28 22:30:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xunjieli, 2016-11-29 00:16:56]

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

[commit-bot: I haz the power, 2016-11-29 00:18:03]

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-11-29 02:38:55]

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

[commit-bot: I haz the power, 2016-11-29 02:38:56]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-11-29 19:39:36]

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

[commit-bot: I haz the power, 2016-11-29 19:40: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-11-29 19:44:54]

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

[commit-bot: I haz the power, 2016-11-29 19:44:55]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[xunjieli, 2016-11-29 21:10:00]

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

[commit-bot: I haz the power, 2016-11-29 21:10:53]

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

[xunjieli, 2016-11-29 21:21:14]

Patchset #6 (id:300001) has been deleted

[xunjieli, 2016-11-29 21:21:59]

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

[commit-bot: I haz the power, 2016-11-29 21:22:37]

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

[eroman, 2016-11-29 22:22:51]

general approach lg.

I didn't dig through the entire CL, but tried to look for the interface changes.

Let me know when you are ready for a more thorough review pass

https://codereview.chromium.org/2525743002/diff/240001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2525743002/diff/240001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:897: context->set_name_string("systems");
nit: systems --> system

https://codereview.chromium.org/2525743002/diff/240001/jingle/glue/proxy_reso...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/2525743002/diff/240001/jingle/glue/proxy_reso...
jingle/glue/proxy_resolving_client_socket.cc:427:
base::trace_event::MemoryAllocatorDump* dump) const {}
Presumably this should forward to transport_->socket() and will be addressed in
a follow-up? See my other comment about making DumpMemoryStats() have a default
implementation, so this CL doesn't need to add all these empty boilerplates for
compile.

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

https://codereview.chromium.org/2525743002/diff/280001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:99: auto iter = cache_.begin();
I would expect |lock_| should be held while accessing cache_, although the class
doesn't really document its threadsafety or for that matter enforce it
everywhere (SSLClientSessionCache::size() for instance doesn't acquire a lock,
so perhaps I have misunderstood its intention?)

https://codereview.chromium.org/2525743002/diff/320001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/2525743002/diff/320001/net/base/sdch_manager....
net/base/sdch_manager.cc:335: if (dictionary_dump)
What are the circumstances when this would be reached?

Is the thinking that we may have a shared SdchManager between
URLRequestContexts? And hence when we enumerate through the URLRequestContexts
we skip duplicate logging here?

Please add a comment on intent. Should it add an ownership edge like in the
other location with shared state?

https://codereview.chromium.org/2525743002/diff/320001/net/base/sdch_manager....
net/base/sdch_manager.cc:341: ++total_count;
maybe just dictionaries_.size() outside of loop?

https://codereview.chromium.org/2525743002/diff/320001/net/base/sdch_manager....
net/base/sdch_manager.cc:350: // Use the existing column name so as not to add a
new column.
I am not sure what this comment means regarding "existing" column? Is this about
the UI presentation or "count" being a common name used somewhere else?

https://codereview.chromium.org/2525743002/diff/320001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2525743002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.h:93: base::trace_event::MemoryAllocatorDump*
dump) const = 0;
What is the relation between this and DumpMemoryStats() ?  (i.e. document it)

https://codereview.chromium.org/2525743002/diff/320001/net/socket/stream_sock...
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/2525743002/diff/320001/net/socket/stream_sock...
net/socket/stream_socket.h:122: base::trace_event::MemoryAllocatorDump* dump)
const = 0;
For these various interface additions, I suggest making them non-abstract.

In other words, have a default implementation that does nothing.

That should reduce a lot of the locations that needed to be updated in this CL
(which I don't think are very useful to have to specify everywhere).

In general I agree that mandatory abstract methods are cleaner, but in this case
I think it is just a burden for iteratively adding things, and makes this
initial CL a bit harder to read than it needs to be :)

https://codereview.chromium.org/2525743002/diff/320001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/2525743002/diff/320001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:341: base::trace_event::MemoryAllocatorDump*
dump) const {}
Presumably this remains to be implemented?

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
net/url_request/url_request.cc:1226: "%s/url_request/%p",
dump->absolute_name().c_str(), this));
Note, this presumes names can never contain a NUL character (here and anywhere
.c_str() is used). That may be a fine assumption to make.

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
net/url_request/url_request_context.cc:126: name_string_ = "uninitialized";
nit: how about something more like <unnamed> or <unknown>

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
net/url_request/url_request_context.cc:130: for (auto url_request :
*url_requests_.get()) {
nit: auto*  (came up on some chromium-dev style thread somewhere I think)

[xunjieli, 2016-11-29 22:42:04]

Thanks for the quick reply. I will address the comments. I tried the
non-abstract approach in PS#1 but ran into some linker error. Let me know if you
have any guess on what I might be doing wrong. I will look into it.

https://codereview.chromium.org/2525743002/diff/320001/net/socket/stream_sock...
File net/socket/stream_socket.h (right):

https://codereview.chromium.org/2525743002/diff/320001/net/socket/stream_sock...
net/socket/stream_socket.h:122: base::trace_event::MemoryAllocatorDump* dump)
const = 0;
On 2016/11/29 22:22:51, eroman (slow) wrote:
> For these various interface additions, I suggest making them non-abstract.
> 
> In other words, have a default implementation that does nothing.
> 
> That should reduce a lot of the locations that needed to be updated in this CL
> (which I don't think are very useful to have to specify everywhere).
> 
> In general I agree that mandatory abstract methods are cleaner, but in this
case
> I think it is just a burden for iteratively adding things, and makes this
> initial CL a bit harder to read than it needs to be :)

I had the non-abstract version in PS #1. However, there is a linker error, and I
am not quite sure what it means. I will look into it. 
 
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...

FAILED: newlib_pnacl/remoting_client_plugin_newlib.pexe
newlib_pnacl/exe.unstripped/remoting_client_plugin_newlib.pexe 
python "../../build/toolchain/gcc_link_wrapper.py"
--strip="../../native_client/toolchain/mac_x86/pnacl_newlib/bin/pnacl-finalize"
--unstripped-file="newlib_pnacl/exe.unstripped/remoting_client_plugin_newlib.pexe"
--output="newlib_pnacl/remoting_client_plugin_newlib.pexe" --
../../native_client/toolchain/mac_x86/pnacl_newlib/bin/pnacl-clang++
-Wl,--fatal-warnings -Werror -Wl,-O1 -Wl,--gc-sections -o
"newlib_pnacl/exe.unstripped/remoting_client_plugin_newlib.pexe"
-Wl,--start-group @"newlib_pnacl/remoting_client_plugin_newlib.pexe.rsp" 
-Wl,--end-group   
newlib_pnacl/obj/remoting/protocol/libprotocol.a: error: undefined reference to
'net::StreamSocket::DumpMemoryStats(base::trace_event::MemoryAllocatorDump*)
const'

[eroman, 2016-11-29 22:49:43]

I imagine that is due to component build.

You should be able to fix it by making the body of the virtual inline in the
header.

Or marking it as NET_EXPORT (assuming you can mark virtual functions
NET_EXPORT).

[xunjieli, 2016-11-30 15:00:04]

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

[commit-bot: I haz the power, 2016-11-30 15:00:20]

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

[xunjieli, 2016-11-30 16:02:03]

Thanks a lot for the review! The change looks better and more manageable now
after applying the suggestions. 

If the overall approach looks good, I can start splitting this into smaller CLs
so it's easier for review?

(1) One Cl adds URLRequestContext as a MemoryDumpProvider and |name_string_|
(2) One Cl instruments HttpNetworkSession, SSLClientSocketImpl and
SSLClientSessionCache
(3) One Cl instruments SourceStream
(4) One Cl instruments SdchManager

https://codereview.chromium.org/2525743002/diff/240001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2525743002/diff/240001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:897: context->set_name_string("systems");
On 2016/11/29 22:22:50, eroman (slow) wrote:
> nit: systems --> system

Done.

https://codereview.chromium.org/2525743002/diff/240001/jingle/glue/proxy_reso...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/2525743002/diff/240001/jingle/glue/proxy_reso...
jingle/glue/proxy_resolving_client_socket.cc:427:
base::trace_event::MemoryAllocatorDump* dump) const {}
On 2016/11/29 22:22:50, eroman (slow) wrote:
> Presumably this should forward to transport_->socket() and will be addressed
in
> a follow-up? See my other comment about making DumpMemoryStats() have a
default
> implementation, so this CL doesn't need to add all these empty boilerplates
for
> compile.

Done.

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

https://codereview.chromium.org/2525743002/diff/280001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:99: auto iter = cache_.begin();
On 2016/11/29 22:22:50, eroman (slow) wrote:
> I would expect |lock_| should be held while accessing cache_, although the
class
> doesn't really document its threadsafety or for that matter enforce it
> everywhere (SSLClientSessionCache::size() for instance doesn't acquire a lock,
> so perhaps I have misunderstood its intention?)

Done.

David, do you know why some methods that access |cache_| are not lock protected?
eg. size() and FlushExpiredSessions() which is called from OnMemoryStateChange()
?

https://codereview.chromium.org/2525743002/diff/320001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/2525743002/diff/320001/net/base/sdch_manager....
net/base/sdch_manager.cc:335: if (dictionary_dump)
On 2016/11/29 22:22:50, eroman (slow) wrote:
> What are the circumstances when this would be reached?
> 
> Is the thinking that we may have a shared SdchManager between
> URLRequestContexts? And hence when we enumerate through the URLRequestContexts
> we skip duplicate logging here?
> 
> Please add a comment on intent. Should it add an ownership edge like in the
> other location with shared state?

Done.

URLRequestContexts currently don't share a SdchManager, though they in theory
can. I have added a comment here and an ownership edge.

https://codereview.chromium.org/2525743002/diff/320001/net/base/sdch_manager....
net/base/sdch_manager.cc:341: ++total_count;
On 2016/11/29 22:22:50, eroman (slow) wrote:
> maybe just dictionaries_.size() outside of loop?

Done.

https://codereview.chromium.org/2525743002/diff/320001/net/base/sdch_manager....
net/base/sdch_manager.cc:350: // Use the existing column name so as not to add a
new column.
On 2016/11/29 22:22:50, eroman (slow) wrote:
> I am not sure what this comment means regarding "existing" column? Is this
about
> the UI presentation or "count" being a common name used somewhere else?

Done. Sorry, there is a constant
(base::trace_event::MemoryAllocatorDump::kObjectsCount) that I can used here. I
have removed the confusing comment.

https://codereview.chromium.org/2525743002/diff/320001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2525743002/diff/320001/net/filter/filter_sour...
net/filter/filter_source_stream.h:93: base::trace_event::MemoryAllocatorDump*
dump) const = 0;
On 2016/11/29 22:22:50, eroman (slow) wrote:
> What is the relation between this and DumpMemoryStats() ?  (i.e. document it)

Done. I've added a comment. The main reason is that only FilterSourceStream have
access to the upstream FilterSourceStream, but subclasses of FilterSourceStream
do not.

https://codereview.chromium.org/2525743002/diff/320001/net/socket/tcp_client_...
File net/socket/tcp_client_socket.cc (right):

https://codereview.chromium.org/2525743002/diff/320001/net/socket/tcp_client_...
net/socket/tcp_client_socket.cc:341: base::trace_event::MemoryAllocatorDump*
dump) const {}
On 2016/11/29 22:22:51, eroman (slow) wrote:
> Presumably this remains to be implemented?

Acknowledged. I've adopted the non-abstract approach. I will revisit whether we
want to instrument tcp socket.

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
net/url_request/url_request.cc:1226: "%s/url_request/%p",
dump->absolute_name().c_str(), this));
On 2016/11/29 22:22:51, eroman (slow) wrote:
> Note, this presumes names can never contain a NUL character (here and anywhere
> .c_str() is used). That may be a fine assumption to make.

Acknowledged.  
Looks like elsewhere .c_str() is used, so I guess we can make this assumption.
https://cs.chromium.org/search/?q=CreateAllocatorDump+base::StringPrintf&sq=p...
.
Thanks for pointing it out. I will keep this in mind.

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
net/url_request/url_request_context.cc:126: name_string_ = "uninitialized";
On 2016/11/29 22:22:51, eroman (slow) wrote:
> nit: how about something more like <unnamed> or <unknown>

Done.

https://codereview.chromium.org/2525743002/diff/320001/net/url_request/url_re...
net/url_request/url_request_context.cc:130: for (auto url_request :
*url_requests_.get()) {
On 2016/11/29 22:22:51, eroman (slow) wrote:
> nit: auto*  (came up on some chromium-dev style thread somewhere I think)

Done. Thanks for the helpful pointer I found
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/%22aut...
 , and I changed this to "const auto*" because the function accessed is a const
function.

[xunjieli, 2016-11-30 16:02:35]

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

[commit-bot: I haz the power, 2016-11-30 16:03:01]

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-11-30 17:16:19]

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

[commit-bot: I haz the power, 2016-11-30 17:16:20]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-12-01 21:04:37]

lgtm.

Splitting this into separate CLs sgtm

https://codereview.chromium.org/2525743002/diff/400001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/2525743002/diff/400001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:897: context->set_name_string("system");
maybe "set_name()" is sufficient?

https://codereview.chromium.org/2525743002/diff/400001/chrome/browser/ssl/chr...
File chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc (right):

https://codereview.chromium.org/2525743002/diff/400001/chrome/browser/ssl/chr...
chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:304: :
thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {}
What is this change about?

https://codereview.chromium.org/2525743002/diff/400001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler_unittest.cc (right):

https://codereview.chromium.org/2525743002/diff/400001/content/browser/appcac...
content/browser/appcache/appcache_request_handler_unittest.cc:1039:
std::unique_ptr<net::URLRequestContext> empty_context_;
Same question here -- what was the reason to change this?

https://codereview.chromium.org/2525743002/diff/400001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/2525743002/diff/400001/net/base/sdch_manager....
net/base/sdch_manager.h:203: // |url_request_context_dump| is the associated
URLRequestContext memory dump.
Would calling this parent_dump (or something like that) be reasonable?

The relationship that this is for the URLRequestContext feels like a layering
violation (given that this code is in //net/base, which ideally would not depend
on net/url_request)

https://codereview.chromium.org/2525743002/diff/400001/net/filter/filter_sour...
File net/filter/filter_source_stream.h (right):

https://codereview.chromium.org/2525743002/diff/400001/net/filter/filter_sour...
net/filter/filter_source_stream.h:90: // Subclasses can implement this method to
dump memory allocation stats.
Can you mention what the default implementation does?

[xunjieli, 2016-12-01 21:48:24]

Thanks a lot for the review! I will close this one and address the new comments
in the separate CLs.

[ssid, 2016-12-02 21:36:33]

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

[ssid, 2016-12-02 21:36:35]

Please change the interface to pass around pmd instead of mad. Right now you are
passing around a MemoryAllocatorDump and changing the PMD object that is not
owned by it. MAD is like a child of PMD and it should not know about PMD.
If you would like to pass around MAD or just the string name for dumps, I am
fine either way.
Also, please cc me or primiano in the MDP CLs.

https://codereview.chromium.org/2525743002/diff/400001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/2525743002/diff/400001/net/base/sdch_manager....
net/base/sdch_manager.cc:331: base::trace_event::MemoryAllocatorDump*
url_request_context_dump) const {
Please pass ProcessMemoryDump* here instead.

https://codereview.chromium.org/2525743002/diff/400001/net/base/sdch_manager....
net/base/sdch_manager.cc:334:
url_request_context_dump->process_memory_dump()->GetAllocatorDump(name);
Please do not use process_memory_dump() method. I am removing this method here:
https://codereview.chromium.org/2545173002

You should be passing ProcessMemoryDump object itself to this method if you want
to change it.

https://codereview.chromium.org/2525743002/diff/400001/net/filter/brotli_sour...
File net/filter/brotli_source_stream.cc (right):

https://codereview.chromium.org/2525743002/diff/400001/net/filter/brotli_sour...
net/filter/brotli_source_stream.cc:85: "%s/brotli_%p",
dump->absolute_name().c_str(), this));
Pass the dump name as an extra argument to this function instead of passing
|mad|. You only need the name's prefix here not the dump. This is to make clear
that this function is not changing the |mad| itself, but just using the name to
create different |mad|

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

https://codereview.chromium.org/2525743002/diff/400001/net/http/http_network_...
net/http/http_network_session.cc:405: }
I feel it will look better if it's written as:
if (http_network_session_dump == nullptr) {
  http_network_session_dump = CreateAllocatorDump();
  DumpMemoryStats();
}
AddOwnershipEdge();

https://codereview.chromium.org/2525743002/diff/400001/net/socket/client_sock...
File net/socket/client_socket_pool_manager.h (right):

https://codereview.chromium.org/2525743002/diff/400001/net/socket/client_sock...
net/socket/client_socket_pool_manager.h:90:
base::trace_event::MemoryAllocatorDump* dump) const = 0;
I think this should be:
virtual void DumpMemoryStats(base::trace_event::ProcessMemoryDump* pmd, const
std::string& dump_name_prefix) = 0;

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

https://codereview.chromium.org/2525743002/diff/400001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.cc:98: if (cache_dump)
You'd not have to do this if this class is MDP.

https://codereview.chromium.org/2525743002/diff/400001/net/ssl/ssl_client_ses...
File net/ssl/ssl_client_session_cache.h (right):

https://codereview.chromium.org/2525743002/diff/400001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.h:62: void
DumpMemoryStats(base::trace_event::ProcessMemoryDump* pmd);
I wonder if it is better to add a MemoryDumpProvider implementation to this
class instead of calling this multiple times and dumping only the first time.
This class already seems to implement MemoryCoordinatorClient, so it should be
possible to implement MemoryDumpProvider.

https://codereview.chromium.org/2525743002/diff/400001/net/url_request/url_re...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/2525743002/diff/400001/net/url_request/url_re...
net/url_request/url_request_context.h:288: std::string name_string_;
just name_ or client_name_ or embedder_name_?

[xunjieli, 2016-12-02 22:01:23]

One quick reply on making SSLClientSessionCache as a standalone MDP below. I
will cc you and address other comments in upcoming CLs.

https://codereview.chromium.org/2525743002/diff/400001/net/ssl/ssl_client_ses...
File net/ssl/ssl_client_session_cache.h (right):

https://codereview.chromium.org/2525743002/diff/400001/net/ssl/ssl_client_ses...
net/ssl/ssl_client_session_cache.h:62: void
DumpMemoryStats(base::trace_event::ProcessMemoryDump* pmd);
On 2016/12/02 21:36:35, ssid wrote:
> I wonder if it is better to add a MemoryDumpProvider implementation to this
> class instead of calling this multiple times and dumping only the first time.
> This class already seems to implement MemoryCoordinatorClient, so it should be
> possible to implement MemoryDumpProvider.

There is an effort underway to tie it with a URLRequestContext (such that
SSLClientSessionCache won't be a singleton). I will keep it this way and not
make it into a MDP.