Flush Channel IDs when Cookies get saved to a persistent backend

Flush Channel IDs when Cookies get saved to a persistent backend

BUG=548423
Review-Url: https://codereview.chromium.org/2874973002
Cr-Commit-Position: refs/heads/master@{#473631}
Committed: https://chromium.googlesource.com/chromium/src/+/2b0ad9af1dab8c843ad6cf5fcac8f13a75822f84

[nharper, 2017-05-11 22:17:36]

Description was changed from

==========
Flush Channel IDs when Cookies get saved to a persistent backend

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

to

==========
Flush Channel IDs when Cookies get saved to a persistent backend

BUG=548423
==========

[nharper, 2017-05-11 22:17:37]

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

[mmenke, 2017-05-11 22:20:20]

Could you please be clearer on what you want each person to review?  Who's the
primary reviewer?

[nharper, 2017-05-11 22:30:57]

On 2017/05/11 22:20:20, mmenke wrote:
> Could you please be clearer on what you want each person to review?  Who's the
> primary reviewer?

Sorry about that. I think mattm is the primary reviewer.

Could mattm review the net changes (Channel ID and Cookies) and safe browsing
change?
Could mmenke review the profile_impl_io_data change?
Could jam review content/browser/net/quota_policy_cookie_store.cc and
content/public/browser/cookie_store_factory.h or suggest a different reviewer
for those files?

[mattm, 2017-05-12 00:02:47]

https://codereview.chromium.org/2874973002/diff/20001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2874973002/diff/20001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:885:
channel_id_service_->GetChannelIDStore()->Flush();
What thread is this getting run on? It's unclear but it seem to implicitly
require that  CookieMonster::FlushStore gets called on whatever
SQLiteChannelIDStore::Backend::background_task_runner_ is.

Additionally channel_id_service_->GetChannelIDStore()->Flush() here appears to
do a synchronous flush, while the cookiemonster store_->Flush below ends up
doing an asynchronous flush by posting to a (different?) background task.

(Have you tested this on a debug build to see if all of the thread checks pass?)

https://codereview.chromium.org/2874973002/diff/20001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.h (right):

https://codereview.chromium.org/2874973002/diff/20001/net/cookies/cookie_mons...
net/cookies/cookie_monster.h:139: // corresponding ChannelIDService used with
this CookieStore.
add note about |channel_id_service| lifetime

[nharper, 2017-05-12 02:47:23]

https://codereview.chromium.org/2874973002/diff/20001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2874973002/diff/20001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:885:
channel_id_service_->GetChannelIDStore()->Flush();
On 2017/05/12 00:02:47, mattm wrote:
> What thread is this getting run on? It's unclear but it seem to implicitly
> require that  CookieMonster::FlushStore gets called on whatever
> SQLiteChannelIDStore::Backend::background_task_runner_ is.
> 
> Additionally channel_id_service_->GetChannelIDStore()->Flush() here appears to
> do a synchronous flush, while the cookiemonster store_->Flush below ends up
> doing an asynchronous flush by posting to a (different?) background task.
> 
> (Have you tested this on a debug build to see if all of the thread checks
pass?)

I changed this to do a PostTask on the background_task_runner_, so the channel
ID store Flush should behave the same as the cookie store flush, both in terms
of which thread its run on and being async.

https://codereview.chromium.org/2874973002/diff/20001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.h (right):

https://codereview.chromium.org/2874973002/diff/20001/net/cookies/cookie_mons...
net/cookies/cookie_monster.h:139: // corresponding ChannelIDService used with
this CookieStore.
On 2017/05/12 00:02:47, mattm wrote:
> add note about |channel_id_service| lifetime

Done.

[mmenke, 2017-05-12 16:36:35]

ProfileIOData LGTM

[jam, 2017-05-12 19:51:51]

On 2017/05/11 22:30:57, nharper wrote:
> On 2017/05/11 22:20:20, mmenke wrote:
> > Could you please be clearer on what you want each person to review?  Who's
the
> > primary reviewer?
> 
> Sorry about that. I think mattm is the primary reviewer.
> 
> Could mattm review the net changes (Channel ID and Cookies) and safe browsing
> change?
> Could mmenke review the profile_impl_io_data change?
> Could jam review content/browser/net/quota_policy_cookie_store.cc and
> content/public/browser/cookie_store_factory.h or suggest a different reviewer
> for those files?

lgtm

[mattm, 2017-05-12 21:51:51]

https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:360: nullptr,
channel_id_service ?

https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:360: nullptr,
Add unittests?

https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:885:
channel_id_service_->GetChannelIDStore()->Flush();
The ChannelIDStore has a SequencedTaskRunner created in profile_io_data, while
the CookieStore has a different SequencedTaskRunner created in
quota_policy_cookie_store.cc.  I believe that means there is no guarantee that
the channel id flush will get run first, even if it is posted first, so there
could still be some chance of the cookies getting saved and not the channel ids.

It seems like the ideal solution would be to use the same SequencedTaskRunner
for both.

Another option might be to have the channelidstore flush method also take a
callback, and have that callback run the cookie store flush only ofter the
channel id store is done flushing. But that would probably involve an extra
thread hop delay.

[nharper, 2017-05-17 18:19:19]

mmenke: I changed ProfileImplIOData - do you want to take a look at it again?

https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:360: nullptr,
On 2017/05/12 21:51:51, mattm wrote:
> Add unittests?

Done.

https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:360: nullptr,
On 2017/05/12 21:51:51, mattm wrote:
> channel_id_service ?

Done.

https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:885:
channel_id_service_->GetChannelIDStore()->Flush();
On 2017/05/12 21:51:51, mattm wrote:
> The ChannelIDStore has a SequencedTaskRunner created in profile_io_data, while
> the CookieStore has a different SequencedTaskRunner created in
> quota_policy_cookie_store.cc.  I believe that means there is no guarantee that
> the channel id flush will get run first, even if it is posted first, so there
> could still be some chance of the cookies getting saved and not the channel
ids.
> 
> It seems like the ideal solution would be to use the same SequencedTaskRunner
> for both.
> 
> Another option might be to have the channelidstore flush method also take a
> callback, and have that callback run the cookie store flush only ofter the
> channel id store is done flushing. But that would probably involve an extra
> thread hop delay. 

I made a change to ProfileImplIOData so they share the background task runner.

[mmenke, 2017-05-17 18:23:21]

On 2017/05/17 18:19:19, nharper wrote:
> mmenke: I changed ProfileImplIOData - do you want to take a look at it again?
> 
>
https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
> File net/cookies/cookie_monster.cc (right):
> 
>
https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:360: nullptr,
> On 2017/05/12 21:51:51, mattm wrote:
> > Add unittests?
> 
> Done.
> 
>
https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:360: nullptr,
> On 2017/05/12 21:51:51, mattm wrote:
> > channel_id_service ?
> 
> Done.
> 
>
https://codereview.chromium.org/2874973002/diff/40001/net/cookies/cookie_mons...
> net/cookies/cookie_monster.cc:885:
> channel_id_service_->GetChannelIDStore()->Flush();
> On 2017/05/12 21:51:51, mattm wrote:
> > The ChannelIDStore has a SequencedTaskRunner created in profile_io_data,
while
> > the CookieStore has a different SequencedTaskRunner created in
> > quota_policy_cookie_store.cc.  I believe that means there is no guarantee
that
> > the channel id flush will get run first, even if it is posted first, so
there
> > could still be some chance of the cookies getting saved and not the channel
> ids.
> > 
> > It seems like the ideal solution would be to use the same
SequencedTaskRunner
> > for both.
> > 
> > Another option might be to have the channelidstore flush method also take a
> > callback, and have that callback run the cookie store flush only ofter the
> > channel id store is done flushing. But that would probably involve an extra
> > thread hop delay. 
> 
> I made a change to ProfileImplIOData so they share the background task runner.

No really...however...  Real Soon Now, URLRequestContext construction will all
go through URLRequestContextBuilder.  So maybe you should update that as well,
in this CL?

[nharper, 2017-05-17 22:13:18]

On 2017/05/17 18:23:21, mmenke wrote:
> No really...however...  Real Soon Now, URLRequestContext construction will all
> go through URLRequestContextBuilder.  So maybe you should update that as well,
> in this CL?

Looking at URLRequestContextBuilder, it only creates in-memory channel id/cookie
stores, so there's no need to set the ChannelIDService on the cookie store (or
deal with making them use the same background task runner). The other option is
that URLRequestContextBuilder::SetCookieAndChannelIdStores was called, in which
case it's the caller's responsibility to call the 3-arg CookieMonster c'tor that
takes a ChannelIDService (the URLRequestContextBuilder can't do that, since the
CookieMonster has already been created).

In the long term, I'd like to make CookieMonster implement the ChannelIDService
interface and combine it into a single store.
(URLRequestContext::set_channel_id_service() would go away, and
URLRequestContext::channel_id_service() would return a pointer to the cookie
monster.) I still need to write up what this end goal might look like (and
possible intermediate steps). I think changes to URLRequestContext(Builder)
would fit under that work, not this CL.

[mmenke, 2017-05-17 22:17:11]

On 2017/05/17 22:13:18, nharper wrote:
> On 2017/05/17 18:23:21, mmenke wrote:
> > No really...however...  Real Soon Now, URLRequestContext construction will
all
> > go through URLRequestContextBuilder.  So maybe you should update that as
well,
> > in this CL?
> 
> Looking at URLRequestContextBuilder, it only creates in-memory channel
id/cookie
> stores, so there's no need to set the ChannelIDService on the cookie store (or
> deal with making them use the same background task runner). The other option
is
> that URLRequestContextBuilder::SetCookieAndChannelIdStores was called, in
which
> case it's the caller's responsibility to call the 3-arg CookieMonster c'tor
that
> takes a ChannelIDService (the URLRequestContextBuilder can't do that, since
the
> CookieMonster has already been created).
> 
> In the long term, I'd like to make CookieMonster implement the
ChannelIDService
> interface and combine it into a single store.
> (URLRequestContext::set_channel_id_service() would go away, and
> URLRequestContext::channel_id_service() would return a pointer to the cookie
> monster.) I still need to write up what this end goal might look like (and
> possible intermediate steps). I think changes to URLRequestContext(Builder)
> would fit under that work, not this CL.

Ah, you're right.  I had thought it already supported creating on disk
cookie/channel ID stores, but looks like I'll get to be the lucky one to add
on-disk stores to URLRequestcontext, and I'll make sure to hook up this extra
stuff as well.

[mmenke, 2017-05-17 22:18:25]

And just to be explicit about it:  Feel free to land this whenever mattm has
signed off on it

[mattm, 2017-05-17 23:28:09]

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:493:
scoped_refptr<base::SequencedTaskRunner> background_task_runner =
may want to add comment or different naming to convey why the cookie store and
channel id store use the same task runner, and that this isn't just a background
task runner any other random thing should use (unless it also needs to be
serialized with the cookie/channel id tasks for some reason)

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:516:
cookie_config.channel_id_service = main_context->channel_id_service();
Should this also be done in
android_webview/browser/net/aw_url_request_context_getter.cc ? (I have no idea
what's going on with AwCookieStoreWrapper)

ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm and
ios/chrome/browser/browser_state/off_the_record_chrome_browser_state_io_data.mm?
(Dunno if it matters for iOS these days)

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:521:
main_context->cookie_store()->SetChannelIDServiceID(
Seems like this could be obviated as ChannelIDService passed in the
cookie_config now, or are there some differences in how they are used?

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:648:
scoped_refptr<base::SequencedTaskRunner> background_task_runner =
same

[nharper, 2017-05-18 00:45:53]

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:493:
scoped_refptr<base::SequencedTaskRunner> background_task_runner =
On 2017/05/17 23:28:09, mattm wrote:
> may want to add comment or different naming to convey why the cookie store and
> channel id store use the same task runner, and that this isn't just a
background
> task runner any other random thing should use (unless it also needs to be
> serialized with the cookie/channel id tasks for some reason)

Done.

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:516:
cookie_config.channel_id_service = main_context->channel_id_service();
On 2017/05/17 23:28:08, mattm wrote:
> Should this also be done in
> android_webview/browser/net/aw_url_request_context_getter.cc ? (I have no idea
> what's going on with AwCookieStoreWrapper)

android webview has a CookieManager::GetCookieStore() method which returns a
singleton net::CookieStore* - doing this in android would require changing how
the CookieManager constructs its singleton. I don't think it's worth it to mess
with that right now.
> 
> ios/chrome/browser/browser_state/chrome_browser_state_impl_io_data.mm and
>
ios/chrome/browser/browser_state/off_the_record_chrome_browser_state_io_data.mm?
> (Dunno if it matters for iOS these days)

I could wire it up for iOS - its cookie_util::CookieStoreConfig and
cookie_util::CreateCookieStore are pretty similar to the same struct/function in
the content namespace. I don't think it's worth doing though, given that it's
not used for much.

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:521:
main_context->cookie_store()->SetChannelIDServiceID(
On 2017/05/17 23:28:09, mattm wrote:
> Seems like this could be obviated as ChannelIDService passed in the
> cookie_config now, or are there some differences in how they are used? 

They should get merged together - I'd prefer to postpone that to a future CL
that brings the two stores closer to being one instead of piling more things
onto this CL. (The SetChannelIDServiceID method is used for debugging/metrics,
while the c'tor arg is used for synchronizing Flushing, so there is a
difference, but it's always the same ChannelIDService/Store.)

https://codereview.chromium.org/2874973002/diff/100001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:648:
scoped_refptr<base::SequencedTaskRunner> background_task_runner =
On 2017/05/17 23:28:09, mattm wrote:
> same

Done.

[mattm, 2017-05-18 01:01:17]

sounds good.

lgtm

[nharper, 2017-05-18 01:04:22]

The CQ bit was checked by nharper@chromium.org

[nharper, 2017-05-18 01:04:23]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2874973002/#ps120001
(title: "clarify use of background task runner")

[commit-bot: I haz the power, 2017-05-18 01:07:07]

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, 2017-05-18 02:56:25]

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

[commit-bot: I haz the power, 2017-05-18 02:56:26]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[nharper, 2017-05-19 17:37:09]

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

[commit-bot: I haz the power, 2017-05-19 17:37:56]

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, 2017-05-19 18:54:21]

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

[commit-bot: I haz the power, 2017-05-19 18:54:22]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nharper, 2017-05-20 01:31:47]

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

[commit-bot: I haz the power, 2017-05-20 01:32:16]

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, 2017-05-20 04:30:00]

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

[commit-bot: I haz the power, 2017-05-20 04:30:01]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-05-22 15:45:48]

On 2017/05/18 02:56:26, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

nharper:  Note that this one didn't land.

[nharper, 2017-05-22 17:09:02]

The CQ bit was checked by nharper@chromium.org

[nharper, 2017-05-22 17:09:03]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
mmenke@chromium.org, mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2874973002/#ps160001
(title: "Initialize channel_id_service to nullptr")

[commit-bot: I haz the power, 2017-05-22 17:09:31]

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, 2017-05-22 18:33:53]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495472942956830,
"parent_rev": "38a9bddc6ae85965ef78553fa63e05d35f57f363", "commit_rev":
"2b0ad9af1dab8c843ad6cf5fcac8f13a75822f84"}

[commit-bot: I haz the power, 2017-05-22 18:34:05]

Description was changed from

==========
Flush Channel IDs when Cookies get saved to a persistent backend

BUG=548423
==========

to

==========
Flush Channel IDs when Cookies get saved to a persistent backend

BUG=548423

Review-Url: https://codereview.chromium.org/2874973002
Cr-Commit-Position: refs/heads/master@{#473631}
Committed:
https://chromium.googlesource.com/chromium/src/+/2b0ad9af1dab8c843ad6cf5fcac8...
==========

[commit-bot: I haz the power, 2017-05-22 18:34:07]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/2b0ad9af1dab8c843ad6cf5fcac8...