Fix callsites to URLRequestContext::CopyFrom

Fix callsites to URLRequestContext::CopyFrom

If URLRequestContext::CopyFrom is called and the CookieStore is changed, there
will be two URLRequestContexts sharing the same ChannelIDService but using
different CookieStores. Since the CookieStore could have channel-bound cookies,
a new ChannelIDService should be created and used as well.

Unfortunately, calling URLRequestContext::set_channel_id_service after
URLRequestContext::set_cookie_store isn't enough: the ChannelIDService is
also used in creating the HttpNetworkSession and HttpTransactionFactory.

BUG=291417, 548423
Committed: https://crrev.com/e7b7bcddfe9e709ab316f85548d140c4c2090847
Cr-Commit-Position: refs/heads/master@{#390403}

[nharper, 2016-03-16 23:49:56]

nharper@chromium.org changed reviewers:
+ mattm@chromium.org, mmenke@chromium.org

[mmenke, 2016-03-17 18:53:41]

This is going to take a bit of digging to figure out the full ramifications (Not
of switching the ChannelIDService, so much as creating a new
HttpNetworkSession).  Not sure I'll be able to get back to you this week.

[mmenke, 2016-03-18 16:02:02]

On 2016/03/17 18:53:41, mmenke wrote:
> This is going to take a bit of digging to figure out the full ramifications
(Not
> of switching the ChannelIDService, so much as creating a new
> HttpNetworkSession).  Not sure I'll be able to get back to you this week.

Could you include a BUG= line in the description, filing one if we don't already
have one?  I think we have at least a more general bug for isolating App request
contexts, not sure if there's one for this specific issue.

[nharper, 2016-03-18 17:13:54]

Description was changed from

==========
Fix callsites to URLRequestContext::CopyFrom

If URLRequestContext::CopyFrom is called and the CookieStore is changed, there
will be two URLRequestContexts sharing the same ChannelIDService but using
different CookieStores. Since the CookieStore could have channel-bound cookies,
a new ChannelIDService should be created and used as well.

Unfortunately, calling URLRequestContext::set_channel_id_service after
URLRequestContext::set_cookie_store isn't enough: the ChannelIDService is
also used in creating the HttpNetworkSession and HttpTransactionFactory.
==========

to

==========
Fix callsites to URLRequestContext::CopyFrom

If URLRequestContext::CopyFrom is called and the CookieStore is changed, there
will be two URLRequestContexts sharing the same ChannelIDService but using
different CookieStores. Since the CookieStore could have channel-bound cookies,
a new ChannelIDService should be created and used as well.

Unfortunately, calling URLRequestContext::set_channel_id_service after
URLRequestContext::set_cookie_store isn't enough: the ChannelIDService is
also used in creating the HttpNetworkSession and HttpTransactionFactory.

BUG=291417,548423
==========

[nharper, 2016-03-18 17:16:27]

BUG= line updated.

[mmenke, 2016-03-22 18:39:49]

On 2016/03/18 17:16:27, nharper wrote:
> BUG= line updated.

Sorry for the delay.  I think this works, though I'm a bit concerned about
regressions, and other leaks between the contexts (SDCH dictionaries, for
instance).  I'd really like to see us used completely separate
URLRequestContexts at some point - sharing state between them has gotten us into
all sorts of trouble.

That having been said, I'm ok with landing this as-is, but I think we want a
test...Can we having an integration test to make sure that the ChannelID service
isn't shared between an app request context and the main request context?  I'm
not sure about what sort of ChannelID stuff our testing infrastructure supports.
 This is the sort of thing that could regress completely invisibly, so I'd
really like a test here.

[nharper, 2016-03-22 21:53:00]

On 2016/03/22 18:39:49, mmenke wrote:
> On 2016/03/18 17:16:27, nharper wrote:
> > BUG= line updated.
> 
> Sorry for the delay.  I think this works, though I'm a bit concerned about
> regressions, and other leaks between the contexts (SDCH dictionaries, for
> instance).  I'd really like to see us used completely separate
> URLRequestContexts at some point - sharing state between them has gotten us
into
> all sorts of trouble.
> 
> That having been said, I'm ok with landing this as-is, but I think we want a
> test...Can we having an integration test to make sure that the ChannelID
service
> isn't shared between an app request context and the main request context?  I'm
> not sure about what sort of ChannelID stuff our testing infrastructure
supports.
>  This is the sort of thing that could regress completely invisibly, so I'd
> really like a test here.

In terms of what sort of ChannelID stuff is available for testing, probably the
easiest thing to do is check that the ChannelIDService in the
HttpNetworkSession::Params from URLRequestContext::GetNetworkSessionParams on
each URLRequestContext is different. A better test would be to use the two
URLRequestContexts to make URLRequests, and check that the channel ID keys used
on those requests are different. I don't see a clear way to do this for Channel
ID, but it would be possible to hack something together with token binding
(enable token binding on both requests, get the Token Binding header from the
requests, and parse the public key out of the headers to compare them). Another
option could be to modify URLRequest::Delegate to have some sort of callback
that the TestDelegate can use to smuggle the channel ID key out, but that also
seems a bit hacky.

I haven't figured out how to get the two URLRequestContexts to use for testing.
It looks like I need a ProfileIOData to get the URLRequestContext and
AppRequestContext (and more specifically, to execute the code here). Any advice
or pointers on tests that use an AppRequestContext from a ProfileIOData would be
appreciated.

[mmenke, 2016-03-22 23:05:44]

On 2016/03/22 21:53:00, nharper wrote:
> On 2016/03/22 18:39:49, mmenke wrote:
> > On 2016/03/18 17:16:27, nharper wrote:
> > > BUG= line updated.
> > 
> > Sorry for the delay.  I think this works, though I'm a bit concerned about
> > regressions, and other leaks between the contexts (SDCH dictionaries, for
> > instance).  I'd really like to see us used completely separate
> > URLRequestContexts at some point - sharing state between them has gotten us
> into
> > all sorts of trouble.
> > 
> > That having been said, I'm ok with landing this as-is, but I think we want a
> > test...Can we having an integration test to make sure that the ChannelID
> service
> > isn't shared between an app request context and the main request context? 
I'm
> > not sure about what sort of ChannelID stuff our testing infrastructure
> supports.
> >  This is the sort of thing that could regress completely invisibly, so I'd
> > really like a test here.
> 
> In terms of what sort of ChannelID stuff is available for testing, probably
the
> easiest thing to do is check that the ChannelIDService in the
> HttpNetworkSession::Params from URLRequestContext::GetNetworkSessionParams on
> each URLRequestContext is different. A better test would be to use the two
> URLRequestContexts to make URLRequests, and check that the channel ID keys
used
> on those requests are different. I don't see a clear way to do this for
Channel
> ID, but it would be possible to hack something together with token binding
> (enable token binding on both requests, get the Token Binding header from the
> requests, and parse the public key out of the headers to compare them).
Another
> option could be to modify URLRequest::Delegate to have some sort of callback
> that the TestDelegate can use to smuggle the channel ID key out, but that also
> seems a bit hacky.
> 
> I haven't figured out how to get the two URLRequestContexts to use for
testing.
> It looks like I need a ProfileIOData to get the URLRequestContext and
> AppRequestContext (and more specifically, to execute the code here). Any
advice
> or pointers on tests that use an AppRequestContext from a ProfileIOData would
be
> appreciated.

I think a full-on browser test would be simplest to do.  I'm not sure how to get
an AppRequestContext (The main request context is trivial).  I'll see if I can
dig something up tomorrow.

[mmenke, 2016-03-23 15:10:11]

On 2016/03/22 23:05:44, mmenke wrote:
> On 2016/03/22 21:53:00, nharper wrote:
> > On 2016/03/22 18:39:49, mmenke wrote:
> > > On 2016/03/18 17:16:27, nharper wrote:
> > > > BUG= line updated.
> > > 
> > > Sorry for the delay.  I think this works, though I'm a bit concerned about
> > > regressions, and other leaks between the contexts (SDCH dictionaries, for
> > > instance).  I'd really like to see us used completely separate
> > > URLRequestContexts at some point - sharing state between them has gotten
us
> > into
> > > all sorts of trouble.
> > > 
> > > That having been said, I'm ok with landing this as-is, but I think we want
a
> > > test...Can we having an integration test to make sure that the ChannelID
> > service
> > > isn't shared between an app request context and the main request context? 
> I'm
> > > not sure about what sort of ChannelID stuff our testing infrastructure
> > supports.
> > >  This is the sort of thing that could regress completely invisibly, so I'd
> > > really like a test here.
> > 
> > In terms of what sort of ChannelID stuff is available for testing, probably
> the
> > easiest thing to do is check that the ChannelIDService in the
> > HttpNetworkSession::Params from URLRequestContext::GetNetworkSessionParams
on
> > each URLRequestContext is different. A better test would be to use the two
> > URLRequestContexts to make URLRequests, and check that the channel ID keys
> used
> > on those requests are different. I don't see a clear way to do this for
> Channel
> > ID, but it would be possible to hack something together with token binding
> > (enable token binding on both requests, get the Token Binding header from
the
> > requests, and parse the public key out of the headers to compare them).
> Another
> > option could be to modify URLRequest::Delegate to have some sort of callback
> > that the TestDelegate can use to smuggle the channel ID key out, but that
also
> > seems a bit hacky.
> > 
> > I haven't figured out how to get the two URLRequestContexts to use for
> testing.
> > It looks like I need a ProfileIOData to get the URLRequestContext and
> > AppRequestContext (and more specifically, to execute the code here). Any
> advice
> > or pointers on tests that use an AppRequestContext from a ProfileIOData
would
> be
> > appreciated.
> 
> I think a full-on browser test would be simplest to do.  I'm not sure how to
get
> an AppRequestContext (The main request context is trivial).  I'll see if I can
> dig something up tomorrow.

One option is to just copy how isolated_app_browsertest.cc installs isolated
apps, and use XHRs, rather than use the URLRequestContexts directly.

A better approach may be to set up your own storage partition, without going
through the isolate app stuff.  I'm not quite sure how to do that - you could
just start from StoragePartitionImplMap::Get's call to
CreateRequestContextForStoragePartition, and work your way back (Either through
code search, or through using the aforementioned browsertests and either a
debugger or CHECKs).

[nharper, 2016-04-20 23:11:13]

Patchset #3 (id:40001) has been deleted

[nharper, 2016-04-20 23:11:19]

Patchset #3 (id:60001) has been deleted

[nharper, 2016-04-20 23:52:44]

Patchset #4 (id:100001) has been deleted

[nharper, 2016-04-20 23:58:22]

Patchset #4 (id:120001) has been deleted

[nharper, 2016-04-21 00:25:22]

Patchset #4 (id:140001) has been deleted

[nharper, 2016-04-21 00:27:31]

Updated to have browser tests.

I'm having a problem that the tests are very flaky. Sometimes they'll pass, but
most of the time they will time out or error (like
https://paste.googleplex.com/5760973281427456). By adding some print statements,
it looks like when it times out it has made it past the RunLoop and posted tasks
and to the end of the test body.

Any suggestions on how to debug/fix this flakiness would be appreciated. I'm not
very familiar working with RunLoops, browser threads, or task runners. (In my
debugging, I tried cutting the test down to match
ProfileBrowserTest.CreateNewProfileSynchronous and building off of there, but
then I found that ProfileBrowserTest.CreateNewProfileSynchronous was sometimes
timing out as well. I don't know if there are flakiness issues with all of these
tests, or if something's up with my workstation, or something else.)

[nharper, 2016-04-21 18:54:18]

On 2016/04/21 00:27:31, nharper wrote:
> Updated to have browser tests.
> 
> I'm having a problem that the tests are very flaky. Sometimes they'll pass,
but
> most of the time they will time out or error (like
> https://paste.googleplex.com/5760973281427456). By adding some print
statements,
> it looks like when it times out it has made it past the RunLoop and posted
tasks
> and to the end of the test body.
> 
> Any suggestions on how to debug/fix this flakiness would be appreciated. I'm
not
> very familiar working with RunLoops, browser threads, or task runners. (In my
> debugging, I tried cutting the test down to match
> ProfileBrowserTest.CreateNewProfileSynchronous and building off of there, but
> then I found that ProfileBrowserTest.CreateNewProfileSynchronous was sometimes
> timing out as well. I don't know if there are flakiness issues with all of
these
> tests, or if something's up with my workstation, or something else.)

I'm pretty sure the flakiness that I was seeing was due to issues on my
workstation. I rebooted my workstation this morning and ran the tests several
times and they all passed consistently.

[mmenke, 2016-04-26 18:58:27]

Ahh, sorry, forgot about this one.  Bunch of reviews last week.  I'll try to get
to this today.  If I haven't gotten back to you by Thursday, feel free to ping
me.

[mmenke, 2016-04-27 02:28:47]

LGTM, modulo comments.  Thanks for doing this!  Comments apply to both tests, of
course.  Sorry for the delay.

https://codereview.chromium.org/1814543002/diff/160001/chrome/browser/profile...
File chrome/browser/profiles/profile_browsertest.cc (right):

https://codereview.chromium.org/1814543002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/profile_browsertest.cc:498: CHECK(main_context);
We'd crash just below if these aren't true, anyways, so I don't think we get
anything from these CHECKs.  If you prefer the test fixture not to crash, and
use ASSERTs instead, but I'm happy, either way.

https://codereview.chromium.org/1814543002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/profile_browsertest.cc:502:
main_context->channel_id_service());
Dereferencing the context on the UI thread just doesn't seem like a great idea. 
Let's just get rid of FetchURLRequestContext, and make a single method that
takes two getters and compares them instead.

Also, can we compare the channel ID services set on the network sessions, too
(Can just use EXPECT_EQ to compare them with the corresponding request context's
channel ID service).

Could optionally throw in a check that the cookie stores don't match as well.

[nharper, 2016-04-27 22:25:44]

https://codereview.chromium.org/1814543002/diff/160001/chrome/browser/profile...
File chrome/browser/profiles/profile_browsertest.cc (right):

https://codereview.chromium.org/1814543002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/profile_browsertest.cc:498: CHECK(main_context);
On 2016/04/27 02:28:47, mmenke wrote:
> We'd crash just below if these aren't true, anyways, so I don't think we get
> anything from these CHECKs.  If you prefer the test fixture not to crash, and
> use ASSERTs instead, but I'm happy, either way.

Done.

https://codereview.chromium.org/1814543002/diff/160001/chrome/browser/profile...
chrome/browser/profiles/profile_browsertest.cc:502:
main_context->channel_id_service());
On 2016/04/27 02:28:47, mmenke wrote:
> Dereferencing the context on the UI thread just doesn't seem like a great
idea. 
> Let's just get rid of FetchURLRequestContext, and make a single method that
> takes two getters and compares them instead.
> 
> Also, can we compare the channel ID services set on the network sessions, too
> (Can just use EXPECT_EQ to compare them with the corresponding request
context's
> channel ID service).
> 
> Could optionally throw in a check that the cookie stores don't match as well.

Done.

[nharper, 2016-04-27 22:28:12]

The CQ bit was checked by nharper@chromium.org

[nharper, 2016-04-27 22:28:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1814543002/#ps180001
(title: "refactor checks to happen on IO thread")

[commit-bot: I haz the power, 2016-04-27 22:28:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1814543002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1814543002/180001

[commit-bot: I haz the power, 2016-04-27 22:38:15]

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

[commit-bot: I haz the power, 2016-04-27 22:38:16]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nharper, 2016-04-27 23:46:00]

mattm, can you review chrome/browser/safe_browsing/safe_browsing_service.cc?

[mattm, 2016-04-28 01:56:43]

lgtm

[nharper, 2016-04-28 15:39:52]

The CQ bit was checked by nharper@chromium.org

[nharper, 2016-04-28 15:39:53]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1814543002/#ps200001
(title: "rebase; s/scoped_ptr/std::unique_ptr/")

[commit-bot: I haz the power, 2016-04-28 15:40:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1814543002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1814543002/200001

[commit-bot: I haz the power, 2016-04-28 16:26:44]

Description was changed from

==========
Fix callsites to URLRequestContext::CopyFrom

If URLRequestContext::CopyFrom is called and the CookieStore is changed, there
will be two URLRequestContexts sharing the same ChannelIDService but using
different CookieStores. Since the CookieStore could have channel-bound cookies,
a new ChannelIDService should be created and used as well.

Unfortunately, calling URLRequestContext::set_channel_id_service after
URLRequestContext::set_cookie_store isn't enough: the ChannelIDService is
also used in creating the HttpNetworkSession and HttpTransactionFactory.

BUG=291417,548423
==========

to

==========
Fix callsites to URLRequestContext::CopyFrom

If URLRequestContext::CopyFrom is called and the CookieStore is changed, there
will be two URLRequestContexts sharing the same ChannelIDService but using
different CookieStores. Since the CookieStore could have channel-bound cookies,
a new ChannelIDService should be created and used as well.

Unfortunately, calling URLRequestContext::set_channel_id_service after
URLRequestContext::set_cookie_store isn't enough: the ChannelIDService is
also used in creating the HttpNetworkSession and HttpTransactionFactory.

BUG=291417,548423
==========

[commit-bot: I haz the power, 2016-04-28 16:26:44]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-04-30 17:19:08]

Patchset 6 (id:??) landed as
https://crrev.com/e7b7bcddfe9e709ab316f85548d140c4c2090847
Cr-Commit-Position: refs/heads/master@{#390403}

[commit-bot: I haz the power, 2016-04-30 17:56:13]

Description was changed from

==========
Fix callsites to URLRequestContext::CopyFrom

If URLRequestContext::CopyFrom is called and the CookieStore is changed, there
will be two URLRequestContexts sharing the same ChannelIDService but using
different CookieStores. Since the CookieStore could have channel-bound cookies,
a new ChannelIDService should be created and used as well.

Unfortunately, calling URLRequestContext::set_channel_id_service after
URLRequestContext::set_cookie_store isn't enough: the ChannelIDService is
also used in creating the HttpNetworkSession and HttpTransactionFactory.

BUG=291417,548423
==========

to

==========
Fix callsites to URLRequestContext::CopyFrom

If URLRequestContext::CopyFrom is called and the CookieStore is changed, there
will be two URLRequestContexts sharing the same ChannelIDService but using
different CookieStores. Since the CookieStore could have channel-bound cookies,
a new ChannelIDService should be created and used as well.

Unfortunately, calling URLRequestContext::set_channel_id_service after
URLRequestContext::set_cookie_store isn't enough: the ChannelIDService is
also used in creating the HttpNetworkSession and HttpTransactionFactory.

BUG=291417,548423

Committed: https://crrev.com/e7b7bcddfe9e709ab316f85548d140c4c2090847
Cr-Commit-Position: refs/heads/master@{#390403}
==========