Make CanonicalCookie::Source() private.

Make CanonicalCookie::Source() private.

So that it can be removed in a followup CL.

This CL also fixes saving/restoring incognito cookie stores
on Android, which was not being done correctly.

BUG=620770, 627256
Committed: https://crrev.com/009cf62ee240d54d1f840d3075697ea8c8974164
Cr-Commit-Position: refs/heads/master@{#406063}

[mmenke, 2016-07-12 15:30:05]

Description was changed from

==========
Make CanonicalCookie::Source() private.

So it can be removed in a followup CL.

BUG=620770
==========

to

==========
Make CanonicalCookie::Source() private.

So that it can be removed in a followup CL.

This CL also fixes saving/restoring incognito cookie stores
on Android, which was not being done correctly.

BUG=620770
==========

[mmenke, 2016-07-12 15:31:10]

Description was changed from

==========
Make CanonicalCookie::Source() private.

So that it can be removed in a followup CL.

This CL also fixes saving/restoring incognito cookie stores
on Android, which was not being done correctly.

BUG=620770
==========

to

==========
Make CanonicalCookie::Source() private.

So that it can be removed in a followup CL.

This CL also fixes saving/restoring incognito cookie stores
on Android, which was not being done correctly.

BUG=620770,627256
==========

[mmenke, 2016-07-12 18:08:42]

Patchset #9 (id:150001) has been deleted

[mmenke, 2016-07-12 18:08:53]

Patchset #8 (id:130001) has been deleted

[mmenke, 2016-07-12 19:01:03]

mmenke@chromium.org changed reviewers:
+ achuith@chromium.org, dfalcantara@chromium.org, marq@chromium.org,
mkwst@chromium.org

[mmenke, 2016-07-12 19:01:04]

[+mkwst]:  Please review everything.  We may want to make it easier to inject a
cookie taken from one CookieStore into another at some point, though I'm not
really sure that's something we want to be encouraging.

[+dfalcantara]:  Please review chrome/android/ and chrome/browser/android/. 
This is a bugfix, in addition to updating it to not use the removed method.  See
https://crbug.com/627256 for details.

[+achuith]:  Please review chrome/browser/chromeos/login/profile_auth_data.cc. 
This shouldn't change behavior, since the cookies are copied from an in-memory
store (Though it now also works if an on-disk store is used instead).

[+marq]:  Please review iOS.

[achuithb, 2016-07-13 09:18:31]

achuith@chromium.org changed reviewers:
+ xiyuan@chromium.org

[achuithb, 2016-07-13 09:18:32]

Xiyuan is more familiar with this code than I.

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/profile_auth_data.cc (right):

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/chromeo...
chrome/browser/chromeos/login/profile_auth_data.cc:47: std::string
effective_domain = cookie.Domain();
Could we add a unit test for this?

[marq (ping after 24h), 2016-07-13 09:31:00]

ios/ LGTM.

[Mike West, 2016-07-13 09:59:26]

Looking totally reasonable so far.

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/android...
File chrome/browser/android/cookies/cookies_fetcher.cc (left):

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/android...
chrome/browser/android/cookies/cookies_fetcher.cc:115: bool
experimental_features_enabled =
// TODO(me): I need to talk with Emily and Joel about removing this flag
entirely. :/

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/profile_auth_data.cc (right):

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/chromeo...
chrome/browser/chromeos/login/profile_auth_data.cc:66: // cookie, prior to
import. This allows URL to be treated as an HTTP
Nit: "... to be treated as an _HTTPS_ URL"?

https://codereview.chromium.org/2103863003/diff/210001/net/cookies/cookie_mon...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2103863003/diff/210001/net/cookies/cookie_mon...
net/cookies/cookie_monster.cc:1639: // See:
https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone
Nit: While you're here, could you point this at the new draft:
https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone?

https://codereview.chromium.org/2103863003/diff/210001/net/cookies/cookie_mon...
net/cookies/cookie_monster.cc:1817: // Use an empty GURL.  This method does not
support setting secure cookies.
Hrm. This is a bit surprising.

It looks like the only entry point to this code is
https://cs.chromium.org/chromium/src/net/cookies/cookie_monster_unittest.cc?r....
Perhaps we could remove it entirely and inline a simpler mechanism into that
test?

For this patch, I'd be happy with renaming 'CookieMonster:: SetAllCookiesAsync'
to 'CookieMonster::SetAllCookiesAsyncForTesting' to make sure we don't start
using the method elsewhere.

[xiyuan, 2016-07-13 16:28:27]

c/b/chromeos/login/* lgtm

[gone, 2016-07-13 18:15:38]

lgtm

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:177:
// matters.
This comment doesn't inspire any confidence :P  This function looks to be only
called from readListFromStream, which is called from
CookiesFetcher.restoreCookiesInternal(), which looks like it restores a set of
encrypted Incognito cookies from disk.

Android throws away our Incognito cipher keys frequently, including (I believe)
after Chrome upgrades.  I wouldn't be surprised if we didn't need to worry about
backward compatibility because we wouldn't be able to restore the cookies
anyway.

[mmenke, 2016-07-14 17:34:58]

Thanks for the feedback!

https://codereview.chromium.org/2103863003/diff/210001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://codereview.chromium.org/2103863003/diff/210001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:177:
// matters.
On 2016/07/13 18:15:38, dfalcantara wrote:
> This comment doesn't inspire any confidence :P  This function looks to be only
> called from readListFromStream, which is called from
> CookiesFetcher.restoreCookiesInternal(), which looks like it restores a set of
> encrypted Incognito cookies from disk.
> 
> Android throws away our Incognito cipher keys frequently, including (I
believe)
> after Chrome upgrades.  I wouldn't be surprised if we didn't need to worry
about
> backward compatibility because we wouldn't be able to restore the cookies
> anyway.

Then it's serving its intended purpose - this code isn't well documented, and it
took me a while to figure out what it's for.  Seems like things could improve
there.

"Not being surprised if it's not needed" doesn't exactly give me the urge to
take responsibility for removing it, and keep an eye out for any resulting
problems.  :)

The possibility that we'd end up writing weird stuff to the cookie store if it's
not deleted during the upgrade seems more than a bit concerning.

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/android...
File chrome/browser/android/cookies/cookies_fetcher.cc (left):

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/android...
chrome/browser/android/cookies/cookies_fetcher.cc:115: bool
experimental_features_enabled =
On 2016/07/13 09:59:26, Mike West wrote:
> // TODO(me): I need to talk with Emily and Joel about removing this flag
> entirely. :/

I pinged the bug when writing this CL, but yea...seems about time for that.

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/profile_auth_data.cc (right):

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/chromeo...
chrome/browser/chromeos/login/profile_auth_data.cc:47: std::string
effective_domain = cookie.Domain();
On 2016/07/13 09:18:31, achuithb wrote:
> Could we add a unit test for this?

The no-dot path was already being tested. Added a cookie with a leading dot to
the existing test code.

https://codereview.chromium.org/2103863003/diff/210001/chrome/browser/chromeo...
chrome/browser/chromeos/login/profile_auth_data.cc:66: // cookie, prior to
import. This allows URL to be treated as an HTTP
On 2016/07/13 09:59:26, Mike West wrote:
> Nit: "... to be treated as an _HTTPS_ URL"?

Done

https://codereview.chromium.org/2103863003/diff/210001/net/cookies/cookie_mon...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2103863003/diff/210001/net/cookies/cookie_mon...
net/cookies/cookie_monster.cc:1639: // See:
https://tools.ietf.org/html/draft-west-leave-secure-cookies-alone
On 2016/07/13 09:59:26, Mike West wrote:
> Nit: While you're here, could you point this at the new draft:
> https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone

Done.  I prefer the old URL, though.  :)

https://codereview.chromium.org/2103863003/diff/210001/net/cookies/cookie_mon...
net/cookies/cookie_monster.cc:1817: // Use an empty GURL.  This method does not
support setting secure cookies.
On 2016/07/13 09:59:26, Mike West wrote:
> Hrm. This is a bit surprising.
> 
> It looks like the only entry point to this code is
>
https://cs.chromium.org/chromium/src/net/cookies/cookie_monster_unittest.cc?r....
> Perhaps we could remove it entirely and inline a simpler mechanism into that
> test?
> 
> For this patch, I'd be happy with renaming 'CookieMonster::
SetAllCookiesAsync'
> to 'CookieMonster::SetAllCookiesAsyncForTesting' to make sure we don't start
> using the method elsewhere.

It's used in production on iOS (ios/net/cookies/cookie_store_ios.mm).  Worth
noting iOS doesn't populate the Source() for cookies when it takes them from the
system cookie store (Which is where it gets the cookies it passes this
function), so this doesn't actually change behavior.

I added a TODO to consider removing this method....Which will probably be there
for another 4 years, until the next cookie overhaul.  :)

[gone, 2016-07-14 21:25:10]

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:177:
// matters.
> Then it's serving its intended purpose - this code isn't well documented, and
it
> took me a while to figure out what it's for.  Seems like things could improve
> there.

> "Not being surprised if it's not needed" doesn't exactly give me the urge to
> take responsibility for removing it, and keep an eye out for any resulting
> problems.  :)

The new comment doesn't document anything though -- it just says that you don't
know what it does.

> The possibility that we'd end up writing weird stuff to the cookie store if
it's
> not deleted during the upgrade seems more than a bit concerning.

How so?  If the cipher key is invalid on upgrade (or when the user explicitly
closes Chrome), then the file can't be read and the cookie store file is
useless.  Same principle applies to the tab state files, which are discarded if
the key is invalid.  

Android doesn't tell us when the user explicitly closes us by swiping away the
task, nor does it give Chrome any opportunity to cleanly shut down before it
kills the process.  While there is a hook for upgrades that we've started
listening to, addressing that corner case while the more common scenarios can't
be addressed is less helpful.

[mmenke, 2016-07-14 21:38:28]

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:177:
// matters.
On 2016/07/14 21:25:10, dfalcantara wrote:
> > Then it's serving its intended purpose - this code isn't well documented,
and
> it
> > took me a while to figure out what it's for.  Seems like things could
improve
> > there.
> 
> > "Not being surprised if it's not needed" doesn't exactly give me the urge to
> > take responsibility for removing it, and keep an eye out for any resulting
> > problems.  :)
> 
> The new comment doesn't document anything though -- it just says that you
don't
> know what it does.

Which is true.  It does however, document the lack of documentation.  Since I'll
probably never be touching this code again, I'm reluctant to invest time in
understanding it well enough to document it.  If you prefer, I can remove the
comment.

> > The possibility that we'd end up writing weird stuff to the cookie store if
> it's
> > not deleted during the upgrade seems more than a bit concerning.
> 
> How so?  If the cipher key is invalid on upgrade (or when the user explicitly
> closes Chrome), then the file can't be read and the cookie store file is
> useless.  Same principle applies to the tab state files, which are discarded
if
> the key is invalid.  

Cypher key?  I don't work on Android, and have no clue how things work there. 
I'm concerned we would end up reading the file from an old version, and that all
the fields we'd read here would be one off for the first cookie, off by two for
the second, etc, and we'd try setting cookies with the resulting corrupt data. 
You seem unsure of whether this would happen or not.

> Android doesn't tell us when the user explicitly closes us by swiping away the
> task, nor does it give Chrome any opportunity to cleanly shut down before it
> kills the process.  While there is a hook for upgrades that we've started
> listening to, addressing that corner case while the more common scenarios
can't
> be addressed is less helpful.

Addressing the corner cases?  Once we have corrupted our cookie store, it's
potentially corrupted eternally here, since we keep on saving it and re-loading
it.  At some point this code will presumably throw it away.  I don't know when. 
Since I'll never be touching this code again, I'm reluctant to invest any time
in learning it better.  I'm certainly open to improving how this CL fixes the
problem, just reluctant to invest time in figuring out how to improve how this
CL fixes the problem, since it touches on a number of topics I know nothing
about (Which includes learning what happens to whatever type of data this is
stored as on Android when upgrading between Chrome versions).

[gone, 2016-07-14 21:55:58]

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:177:
// matters.
On 2016/07/14 21:38:28, mmenke wrote:
> Which is true.  It does however, document the lack of documentation.  Since
I'll
> probably never be touching this code again, I'm reluctant to invest time in
> understanding it well enough to document it.  If you prefer, I can remove the
> comment.
Yeah, just remove it.  It's only called from one function, which is only called
from one Java class (which has a class comment describing what it's for) and a
Java test.  Just leave the readUTF() line in, too; it shouldn't have any effect
either way.

I started writing an explanation of exactly what I was unsure about, what the
cipher key was, and why corrupt files shouldn't be an issue because of how
exceptions are caught in CookiesFetcher#restoreCookiesInternal() to deal with
how Android works, but based on your comment you don't seem at all interested in
the topic.

[mmenke, 2016-07-14 22:18:19]

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://chromiumcodereview.appspot.com/2103863003/diff/210001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:177:
// matters.
On 2016/07/14 21:55:58, dfalcantara wrote:
> On 2016/07/14 21:38:28, mmenke wrote:
> > Which is true.  It does however, document the lack of documentation.  Since
> I'll
> > probably never be touching this code again, I'm reluctant to invest time in
> > understanding it well enough to document it.  If you prefer, I can remove
the
> > comment.
> Yeah, just remove it.  It's only called from one function, which is only
called
> from one Java class (which has a class comment describing what it's for) and a
> Java test.  Just leave the readUTF() line in, too; it shouldn't have any
effect
> either way.

Done (Well, I duplicated the backward compatibility comment from above).

> I started writing an explanation of exactly what I was unsure about, what the
> cipher key was, and why corrupt files shouldn't be an issue because of how
> exceptions are caught in CookiesFetcher#restoreCookiesInternal() to deal with
> how Android works, but based on your comment you don't seem at all interested
in
> the topic.

Unfortunately, I just don't have time to be interested, lately.

Relying on readLong() / readInt() / readBoolean() to throw an exception in the
case of misalignment (Which I think is what you're suggesting?) just seems
inherently flaky to me.

[mmenke, 2016-07-15 17:53:02]

[mkwst]:  No rush on this CL, just want to make sure you're aware it's in your
court.

[Mike West, 2016-07-18 08:35:23]

LGTM. Thanks for taking another pass!

[mmenke, 2016-07-18 16:55:53]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-07-18 16:55:54]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
dfalcantara@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2103863003/#ps270001
(title: "Merge")

[commit-bot: I haz the power, 2016-07-18 16:56:38]

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-07-18 19:33:48]

Description was changed from

==========
Make CanonicalCookie::Source() private.

So that it can be removed in a followup CL.

This CL also fixes saving/restoring incognito cookie stores
on Android, which was not being done correctly.

BUG=620770,627256
==========

to

==========
Make CanonicalCookie::Source() private.

So that it can be removed in a followup CL.

This CL also fixes saving/restoring incognito cookie stores
on Android, which was not being done correctly.

BUG=620770,627256
==========

[commit-bot: I haz the power, 2016-07-18 19:33:50]

Committed patchset #13 (id:270001)

[commit-bot: I haz the power, 2016-07-18 19:34:30]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-18 19:36:00]

Description was changed from

==========
Make CanonicalCookie::Source() private.

So that it can be removed in a followup CL.

This CL also fixes saving/restoring incognito cookie stores
on Android, which was not being done correctly.

BUG=620770,627256
==========

to

==========
Make CanonicalCookie::Source() private.

So that it can be removed in a followup CL.

This CL also fixes saving/restoring incognito cookie stores
on Android, which was not being done correctly.

BUG=620770,627256

Committed: https://crrev.com/009cf62ee240d54d1f840d3075697ea8c8974164
Cr-Commit-Position: refs/heads/master@{#406063}
==========

[commit-bot: I haz the power, 2016-07-18 19:36:02]

Patchset 13 (id:??) landed as
https://crrev.com/009cf62ee240d54d1f840d3075697ea8c8974164
Cr-Commit-Position: refs/heads/master@{#406063}