Make net::URLRequestContext a MemoryDumpProvider

chrome/browser/profiles/profile_io_data.cc

Index: chrome/browser/profiles/profile_io_data.cc
diff --git a/chrome/browser/profiles/profile_io_data.cc b/chrome/browser/profiles/profile_io_data.cc
index 7f94c7eff327b7a64b009e23207e98222f953b1a..0751e9001765c14eea552c3f95fcccb79817bb6f 100644
--- a/chrome/browser/profiles/profile_io_data.cc
+++ b/chrome/browser/profiles/profile_io_data.cc
@@ -560,6 +560,7 @@ ProfileIOData::MediaRequestContext::MediaRequestContext() {
 void ProfileIOData::MediaRequestContext::SetHttpTransactionFactory(
     std::unique_ptr<net::HttpTransactionFactory> http_factory) {
   http_factory_ = std::move(http_factory);
+  set_name("media");

[mmenke, 2016/12/02 15:47:28]

This contradicts my above suggestion, but the main media request context and
main request context share HttpNetworkSessions, which means they share socket
pools.  Should they be considered the same RequestContext, for memory purposes?

[mmenke, 2016/12/02 15:47:28]

"SetHttpTransactionFactory" seems a weird place to do this.  Why not do it in
the constructor?  Or even make it a constructor parameter of URLRequestContext
(And make it const).  That way, we'd be able to make sure we catch everything,
which this CL doesn't do.

[xunjieli, 2016/12/02 16:13:13]

That's a very good point. I've explained this briefly in the design doc
(https://docs.google.com/document/d/1zBX27tvkc8ZJHp9yvm6EQJrMDIhDCU92YlahaGFws...)
as well. HttpNetworkSessions will be logged under "net/http_network_session/x."
We use ProcessMemoryDump::AddOwnershipEdge() to indicate the ownership
relationship and how much weight to attribute the memory to each
URLRequestContext.

[xunjieli, 2016/12/02 16:13:14]

I've considered that. However, URLRequestContext's constructor currently doesn't
take anything. All params and configs are passed in through set_*() methods. To
be consistent, I kept it that way.

[mmenke, 2016/12/05 17:59:25]

Given what you told me about how this works, I don't think we want to media
request contexts to have their own name.  Since they split attribution of stuff
owned by the main request context, I think this is just going to add
noise...Particularly since app request contexts also use this method for their
media request contexts.

I think instead, media request contexts should share the name of their parent
request context (main one is called "main", etc).

[xunjieli, 2016/12/05 18:15:16]

I am not sure if sharing the same name is a good idea. If they are both logged
under "main_%p" How do we differentiate them? MemoryDumpProvider doesn't allow
adding two MemoryAllocatorDumps with the same name. 

We could call this one "main_%p_app_request", but I think that's not strictly
necessary. For all shared resources, we will be able to attribute them with
AddOwnershipEdge. That's a clearer way to show sharing relationship. 

[mmenke, 2016/12/05 18:18:51]

I'm not following.  There can be multiple app request contexts, as we're calling
them all the same thing.  Each app request context can have a media request
context.  So we can have multiple media request contexts, all of which will have
the same name.  Are you saying that won't work?

[xunjieli, 2016/12/05 19:01:33]

Done. Thanks for the clarifying this offline.
I didn't realize there are two types of media request contexts.
I have moved the set_name() to the MediaRequestContext's constructor and
AppRequestContext's constructor. PTAL.

   set_http_transaction_factory(http_factory_.get());
 }
 
@@ -590,6 +591,7 @@ void ProfileIOData::AppRequestContext::SetHttpNetworkSession(
 void ProfileIOData::AppRequestContext::SetHttpTransactionFactory(
     std::unique_ptr<net::HttpTransactionFactory> http_factory) {
   http_factory_ = std::move(http_factory);
+  set_name("app_request");

[mmenke, 2016/12/05 17:59:25]

Naming these in SetHttpTransactionFactory seems weird.  Why can't they just do
it in their constructors?

[mmenke, 2016/12/05 18:02:13]

Note that you would need to add a parameter to media request context
contructors, to distinguish app and media but I think that's reasonable.

[xunjieli, 2016/12/05 18:15:16]

Done.

   set_http_transaction_factory(http_factory_.get());
 }
 
@@ -1022,6 +1024,8 @@ void ProfileIOData::Init(
   main_request_context_storage_.reset(
       new net::URLRequestContextStorage(main_request_context_.get()));
   extensions_request_context_.reset(new net::URLRequestContext());
+  main_request_context_->set_name("main");
+  extensions_request_context_->set_name("extensions");
 
   main_request_context_->set_enable_brotli(io_thread_globals->enable_brotli);