Make net::URLRequestContext a MemoryDumpProvider

Make net::URLRequestContext a MemoryDumpProvider

This CL makes net::URLRequestContext a MemoryDumpProvider.
The design doc is linked in the crbug.

BUG=669108
Committed: https://crrev.com/96ab36a707e8e8633ec92185bd3f41a5157af83b
Cr-Commit-Position: refs/heads/master@{#436417}

[xunjieli, 2016-11-30 18:37:05]

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

[commit-bot: I haz the power, 2016-11-30 18:37: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-30 19:46:44]

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

[commit-bot: I haz the power, 2016-11-30 19:46:46]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-12-01 22:01:58]

xunjieli@chromium.org changed reviewers:
+ eroman@chromium.org, estark@chromium.org, mattm@chromium.org,
michaeln@chromium.org, mmenke@chromium.org

[xunjieli, 2016-12-01 22:01:59]

(This is CL #1 that is split off from 2525743002.)

eroman: could you give me a net OWNER stamp?
mmenke: PTAL at chrome/browser/io_thread.cc and profile_io_data.cc. 
mattm: PTAL at chrome/browser/safe_browsing/safe_browsing_service.cc
estark: PTAL at chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
michaeln: PTAL at content/browser/appcache/appcache_request_handler_unittest.cc

Thanks!

https://codereview.chromium.org/2536723009/diff/1/chrome/browser/ssl/chrome_e...
File chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc (right):

https://codereview.chromium.org/2536723009/diff/1/chrome/browser/ssl/chrome_e...
chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc:308: context_.reset(new
net::TestURLRequestContext(true));
I changed |context_| to be initialized in SetUp() because URLRequestContext
constructor now accesses current thread handle
(base::ThreadTaskRunnerHandle::Get()) and we need to wait for the thread to be
set up.

https://codereview.chromium.org/2536723009/diff/1/content/browser/appcache/ap...
File content/browser/appcache/appcache_request_handler_unittest.cc (right):

https://codereview.chromium.org/2536723009/diff/1/content/browser/appcache/ap...
content/browser/appcache/appcache_request_handler_unittest.cc:219:
empty_context_.reset(new net::URLRequestContext);
I changed |empty_context_| to be initialized in SetUpTest() because
URLRequestContext constructor calls base::ThreadTaskRunnerHandle::Get() and we
need to wait for the thread to be set up.

[eroman, 2016-12-01 22:16:00]

lgtm

https://codereview.chromium.org/2536723009/diff/1/net/url_request/url_request...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/2536723009/diff/1/net/url_request/url_request...
net/url_request/url_request_context.h:241: void set_name_string(const
std::string& name_string) {
how about calling this "set_name()" and the variable "name_" ?
i.e. dropping the "_string" suffix.

Also: Can you add some documentation explaining the purpose of the name, and
expectations on uniqueness (i.e. it doesn't need to be necessarily unique).

[xunjieli, 2016-12-01 22:45:22]

Thanks!

https://codereview.chromium.org/2536723009/diff/1/net/url_request/url_request...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/2536723009/diff/1/net/url_request/url_request...
net/url_request/url_request_context.h:241: void set_name_string(const
std::string& name_string) {
On 2016/12/01 22:16:00, eroman (slow) wrote:
> how about calling this "set_name()" and the variable "name_" ?
> i.e. dropping the "_string" suffix.
> 
> Also: Can you add some documentation explaining the purpose of the name, and
> expectations on uniqueness (i.e. it doesn't need to be necessarily unique).

Done.

[michaeln, 2016-12-01 22:52:46]

appcache lgtm

[mattm, 2016-12-02 02:08:07]

safe_browsing lgtm

[xunjieli, 2016-12-02 14:07:35]

Description was changed from

==========
Make net::URLRequestContext a MemoryDumpProvider

This CL makes net::URLRequestContext a MemoryDumpProvider.
This CL is the first in the series. The overrall approach can be seen
in crrev.com/2525743002.

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

to

==========
Make net::URLRequestContext a MemoryDumpProvider

This CL makes net::URLRequestContext a MemoryDumpProvider.
The design doc is linked in the crbug.

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

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

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:563: set_name("media");
"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.

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:563: set_name("media");
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?

https://codereview.chromium.org/2536723009/diff/40001/net/url_request/url_req...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/2536723009/diff/40001/net/url_request/url_req...
net/url_request/url_request_context.h:248: base::trace_event::ProcessMemoryDump*
pmd) override;
Does this work?  The URLRequestContext depends on all its subcomponents.  If
those subcomponents use the URLRequestContext as a memory dump provider, that
seems to create a circular dependency, no?

Worth noting that URLRequestJobs and URLRequests already do have circular
dependencies on the URLRequestContext, but adding more such dependencies to
lower layers of the network stack (Which may even be shared between
URLRequestContexts, some of which could be destroyed well before the main
URLRequestContext, and the shared HttpNetworkSession...) seems like it would
make the situation worse.

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

Thanks! PTAL.

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:563: set_name("media");
On 2016/12/02 15:47:28, mmenke wrote:
> 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?

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.

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:563: set_name("media");
On 2016/12/02 15:47:28, mmenke wrote:
> "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.

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.

https://codereview.chromium.org/2536723009/diff/40001/net/url_request/url_req...
File net/url_request/url_request_context.h (right):

https://codereview.chromium.org/2536723009/diff/40001/net/url_request/url_req...
net/url_request/url_request_context.h:248: base::trace_event::ProcessMemoryDump*
pmd) override;
On 2016/12/02 15:47:28, mmenke wrote:
> Does this work?  The URLRequestContext depends on all its subcomponents.  If
> those subcomponents use the URLRequestContext as a memory dump provider, that
> seems to create a circular dependency, no?
> 
> Worth noting that URLRequestJobs and URLRequests already do have circular
> dependencies on the URLRequestContext, but adding more such dependencies to
> lower layers of the network stack (Which may even be shared between
> URLRequestContexts, some of which could be destroyed well before the main
> URLRequestContext, and the shared HttpNetworkSession...) seems like it would
> make the situation worse.

Great question. I don't think there's a circular dependency. MemoryDumpProvider
is polling based. Here we are just trying to reach associated components. The
subcomponents don't need to know about the parent/caller. If a subcomponent goes
away, we can't reach it from URLRequestContext, so I think we won't complicate
the lifetime issues.

I have a draft CL at https://codereview.chromium.org/2525743002/ which outlines
the overall approach.

[estark, 2016-12-02 17:24:40]

chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc lgtm

[ssid, 2016-12-02 21:42:12]

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

[ssid, 2016-12-02 21:42:13]

This cl looks good, but please see comments in
https://codereview.chromium.org/2525743002 for further CLs. I didn't realize you
split the cl.

[xunjieli, 2016-12-05 13:52:41]

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

[commit-bot: I haz the power, 2016-12-05 13:52:51]

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-05 13:53:00]

mmenke@: A friendly ping. Thank you!

[commit-bot: I haz the power, 2016-12-05 14:45:11]

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

[commit-bot: I haz the power, 2016-12-05 14:45:12]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:563: set_name("media");
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).

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:594: set_name("app_request");
Naming these in SetHttpTransactionFactory seems weird.  Why can't they just do
it in their constructors?

[mmenke, 2016-12-05 18:02:14]

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:594: set_name("app_request");
On 2016/12/05 17:59:25, mmenke wrote:
> Naming these in SetHttpTransactionFactory seems weird.  Why can't they just do
> it in their constructors?

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]

Thanks. PTAL.

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:563: set_name("media");
On 2016/12/05 17:59:25, mmenke wrote:
> 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).

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.

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:594: set_name("app_request");
On 2016/12/05 17:59:25, mmenke wrote:
> Naming these in SetHttpTransactionFactory seems weird.  Why can't they just do
> it in their constructors?

Done.

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

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:563: set_name("media");
On 2016/12/05 18:15:16, xunjieli wrote:
> On 2016/12/05 17:59:25, mmenke wrote:
> > 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).
> 
> 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. 

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]

PTAL.Thanks!

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2536723009/diff/40001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:563: set_name("media");
On 2016/12/05 18:18:51, mmenke wrote:
> On 2016/12/05 18:15:16, xunjieli wrote:
> > On 2016/12/05 17:59:25, mmenke wrote:
> > > 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).
> > 
> > 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. 
> 
> 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?

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.

[xunjieli, 2016-12-05 19:01:42]

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

[commit-bot: I haz the power, 2016-12-05 19:02:29]

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

[mmenke, 2016-12-05 19:06:01]

LGTM

[xunjieli, 2016-12-05 19:07:54]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2016-12-05 19:07:59]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-12-05 19:07:59]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
michaeln@chromium.org, mattm@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2536723009/#ps100001
(title: "self review")

[commit-bot: I haz the power, 2016-12-05 19:08:24]

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

[xunjieli, 2016-12-05 19:21:00]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2016-12-05 19:29:52]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-12-05 19:29:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, mattm@chromium.org, eroman@chromium.org,
mmenke@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2536723009/#ps120001
(title: "fix compile")

[commit-bot: I haz the power, 2016-12-05 19:30:37]

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-05 21:36:08]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480966192494060,
"parent_rev": "b3a7768bb2ea897410e37f2f2ed0f123b5893436", "commit_rev":
"dfa2b5da73a801350fa089cb8cdb263f29dde344"}

[commit-bot: I haz the power, 2016-12-05 21:36:36]

Description was changed from

==========
Make net::URLRequestContext a MemoryDumpProvider

This CL makes net::URLRequestContext a MemoryDumpProvider.
The design doc is linked in the crbug.

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

to

==========
Make net::URLRequestContext a MemoryDumpProvider

This CL makes net::URLRequestContext a MemoryDumpProvider.
The design doc is linked in the crbug.

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

[commit-bot: I haz the power, 2016-12-05 21:36:38]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-05 21:40:27]

Description was changed from

==========
Make net::URLRequestContext a MemoryDumpProvider

This CL makes net::URLRequestContext a MemoryDumpProvider.
The design doc is linked in the crbug.

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

to

==========
Make net::URLRequestContext a MemoryDumpProvider

This CL makes net::URLRequestContext a MemoryDumpProvider.
The design doc is linked in the crbug.

BUG=669108

Committed: https://crrev.com/96ab36a707e8e8633ec92185bd3f41a5157af83b
Cr-Commit-Position: refs/heads/master@{#436417}
==========

[commit-bot: I haz the power, 2016-12-05 21:40:29]

Patchset 7 (id:??) landed as
https://crrev.com/96ab36a707e8e8633ec92185bd3f41a5157af83b
Cr-Commit-Position: refs/heads/master@{#436417}