android: Fix CanonicalCookie same_site field

android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

Also update the format:
* Add an explicit version number that's just a date
* Explicitly encode the length so don't need EOF

Note the cookies can never persist across app updates, so no
need for any migration logic.

BUG=459154
Committed: https://crrev.com/e391c6d887ac88d4d8aa9dde783e35607325abff
Cr-Commit-Position: refs/heads/master@{#390655}

[boliu, 2016-04-18 20:50:21]

boliu@chromium.org changed reviewers:
+ yfriedman@chromium.org

[boliu, 2016-04-18 20:50:22]

ptal

https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
out.writeInt(mSameSite);
this should be safe since CookiesFetcher catches Throwable to make sure it
doesn't crash..?

[boliu, 2016-04-18 21:58:55]

cc mkwst

[Yaron, 2016-04-19 14:36:13]

https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
out.writeInt(mSameSite);
On 2016/04/18 20:50:22, boliu wrote:
> this should be safe since CookiesFetcher catches Throwable to make sure it
> doesn't crash..?

Confused why you're worried about the write - it's the read of the old boolean
as int which would be my concern. :)
Now you'd be trying to read 2 ints instead of a boolean & an int so the second
int will hit EOF. What's worse is that there's no separator between canonical
cookies so you'll actually start reading into the next canonical cookies's data
and you'd be off alignment and reading grabage :S

The safest thing may be to blow away the cookie store as a one-time operation
for this migration and add a version # to allow future upgrades to be seamless?

[boliu, 2016-04-19 15:07:40]

On 2016/04/19 14:36:13, Yaron wrote:
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
> (right):
> 
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
> out.writeInt(mSameSite);
> On 2016/04/18 20:50:22, boliu wrote:
> > this should be safe since CookiesFetcher catches Throwable to make sure it
> > doesn't crash..?
> 
> Confused why you're worried about the write - it's the read of the old boolean
> as int which would be my concern. :)
> Now you'd be trying to read 2 ints instead of a boolean & an int so the second
> int will hit EOF. What's worse is that there's no separator between canonical
> cookies so you'll actually start reading into the next canonical cookies's
data
> and you'd be off alignment and reading grabage :S
> 
> The safest thing may be to blow away the cookie store as a one-time operation
> for this migration and add a version # to allow future upgrades to be
seamless?

Wow, this code is so bad it's making me want to cry. Catching an exception to
detect end of stream? Who wrote this??

[Yaron, 2016-04-19 15:20:06]

On 2016/04/19 15:07:40, boliu wrote:
> On 2016/04/19 14:36:13, Yaron wrote:
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
> > out.writeInt(mSameSite);
> > On 2016/04/18 20:50:22, boliu wrote:
> > > this should be safe since CookiesFetcher catches Throwable to make sure it
> > > doesn't crash..?
> > 
> > Confused why you're worried about the write - it's the read of the old
boolean
> > as int which would be my concern. :)
> > Now you'd be trying to read 2 ints instead of a boolean & an int so the
second
> > int will hit EOF. What's worse is that there's no separator between
canonical
> > cookies so you'll actually start reading into the next canonical cookies's
> data
> > and you'd be off alignment and reading grabage :S
> > 
> > The safest thing may be to blow away the cookie store as a one-time
operation
> > for this migration and add a version # to allow future upgrades to be
> seamless?
> 
> Wow, this code is so bad it's making me want to cry. Catching an exception to
> detect end of stream? Who wrote this??

:(
Think of all the beautiful cookie files out there that are so rigidly written :)

[boliu, 2016-04-19 15:23:25]

On 2016/04/19 15:20:06, Yaron wrote:
> On 2016/04/19 15:07:40, boliu wrote:
> > On 2016/04/19 14:36:13, Yaron wrote:
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > File
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
> > > out.writeInt(mSameSite);
> > > On 2016/04/18 20:50:22, boliu wrote:
> > > > this should be safe since CookiesFetcher catches Throwable to make sure
it
> > > > doesn't crash..?
> > > 
> > > Confused why you're worried about the write - it's the read of the old
> boolean
> > > as int which would be my concern. :)
> > > Now you'd be trying to read 2 ints instead of a boolean & an int so the
> second
> > > int will hit EOF. What's worse is that there's no separator between
> canonical
> > > cookies so you'll actually start reading into the next canonical cookies's
> > data
> > > and you'd be off alignment and reading grabage :S
> > > 
> > > The safest thing may be to blow away the cookie store as a one-time
> operation
> > > for this migration and add a version # to allow future upgrades to be
> > seamless?
> > 
> > Wow, this code is so bad it's making me want to cry. Catching an exception
to
> > detect end of stream? Who wrote this??
> 
> :(
> Think of all the beautiful cookie files out there that are so rigidly written
:)

I'm working on this on the side, because my main project is stalled, and
thinking, oh, these small things on the side will cheer me up because easy
progress. Oh was I wrong.. ok rant over.

So how would we detect version skew if there is no version to begin with then..?
I can add a version as the first thing, but there is always a chance it collides
with the actual cookie value or something. Do you know if data streams are type
safe, like bool can never be read back as an int or something like that?

[Yaron, 2016-04-19 15:29:14]

On 2016/04/19 15:23:25, boliu wrote:
> On 2016/04/19 15:20:06, Yaron wrote:
> > On 2016/04/19 15:07:40, boliu wrote:
> > > On 2016/04/19 14:36:13, Yaron wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > > File
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
> > > > out.writeInt(mSameSite);
> > > > On 2016/04/18 20:50:22, boliu wrote:
> > > > > this should be safe since CookiesFetcher catches Throwable to make
sure
> it
> > > > > doesn't crash..?
> > > > 
> > > > Confused why you're worried about the write - it's the read of the old
> > boolean
> > > > as int which would be my concern. :)
> > > > Now you'd be trying to read 2 ints instead of a boolean & an int so the
> > second
> > > > int will hit EOF. What's worse is that there's no separator between
> > canonical
> > > > cookies so you'll actually start reading into the next canonical
cookies's
> > > data
> > > > and you'd be off alignment and reading grabage :S
> > > > 
> > > > The safest thing may be to blow away the cookie store as a one-time
> > operation
> > > > for this migration and add a version # to allow future upgrades to be
> > > seamless?
> > > 
> > > Wow, this code is so bad it's making me want to cry. Catching an exception
> to
> > > detect end of stream? Who wrote this??
> > 
> > :(
> > Think of all the beautiful cookie files out there that are so rigidly
written
> :)
> 
> I'm working on this on the side, because my main project is stalled, and
> thinking, oh, these small things on the side will cheer me up because easy
> progress. Oh was I wrong.. ok rant over.
> 
> So how would we detect version skew if there is no version to begin with
then..?
> I can add a version as the first thing, but there is always a chance it
collides
> with the actual cookie value or something. Do you know if data streams are
type
> safe, like bool can never be read back as an int or something like that?

Nope, won't work. :(
Easiest way may be to rename the file once and introduce a version number in
that version.
You would change fetch fetchFileName to try the deprecated name after trying to
fetch the new name. So modulo the one upgrade, it's no additional IO.
Oh, actually! Check out MAGIC_STRING.. you could change the value of it and it
would invalidate any old data. :)

In the same change, I'd also write a "record count" to avoid that EOF ;)

[boliu, 2016-04-19 15:41:37]

On 2016/04/19 15:29:14, Yaron wrote:
> On 2016/04/19 15:23:25, boliu wrote:
> > On 2016/04/19 15:20:06, Yaron wrote:
> > > On 2016/04/19 15:07:40, boliu wrote:
> > > > On 2016/04/19 14:36:13, Yaron wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > > > File
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
> > > > > out.writeInt(mSameSite);
> > > > > On 2016/04/18 20:50:22, boliu wrote:
> > > > > > this should be safe since CookiesFetcher catches Throwable to make
> sure
> > it
> > > > > > doesn't crash..?
> > > > > 
> > > > > Confused why you're worried about the write - it's the read of the old
> > > boolean
> > > > > as int which would be my concern. :)
> > > > > Now you'd be trying to read 2 ints instead of a boolean & an int so
the
> > > second
> > > > > int will hit EOF. What's worse is that there's no separator between
> > > canonical
> > > > > cookies so you'll actually start reading into the next canonical
> cookies's
> > > > data
> > > > > and you'd be off alignment and reading grabage :S
> > > > > 
> > > > > The safest thing may be to blow away the cookie store as a one-time
> > > operation
> > > > > for this migration and add a version # to allow future upgrades to be
> > > > seamless?
> > > > 
> > > > Wow, this code is so bad it's making me want to cry. Catching an
exception
> > to
> > > > detect end of stream? Who wrote this??
> > > 
> > > :(
> > > Think of all the beautiful cookie files out there that are so rigidly
> written
> > :)
> > 
> > I'm working on this on the side, because my main project is stalled, and
> > thinking, oh, these small things on the side will cheer me up because easy
> > progress. Oh was I wrong.. ok rant over.
> > 
> > So how would we detect version skew if there is no version to begin with
> then..?
> > I can add a version as the first thing, but there is always a chance it
> collides
> > with the actual cookie value or something. Do you know if data streams are
> type
> > safe, like bool can never be read back as an int or something like that?
> 
> Nope, won't work. :(
> Easiest way may be to rename the file once and introduce a version number in
> that version.
> You would change fetch fetchFileName to try the deprecated name after trying
to
> fetch the new name. So modulo the one upgrade, it's no additional IO.
> Oh, actually! Check out MAGIC_STRING.. you could change the value of it and it
> would invalidate any old data. :)
> 
> In the same change, I'd also write a "record count" to avoid that EOF ;)

So I assume you think it's ok not to recover the old content, and just blow
everything away?

[Yaron, 2016-04-19 15:48:35]

On 2016/04/19 15:41:37, boliu wrote:
> On 2016/04/19 15:29:14, Yaron wrote:
> > On 2016/04/19 15:23:25, boliu wrote:
> > > On 2016/04/19 15:20:06, Yaron wrote:
> > > > On 2016/04/19 15:07:40, boliu wrote:
> > > > > On 2016/04/19 14:36:13, Yaron wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > > > > File
> > > > > >
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
> > > > > > (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > > > >
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
> > > > > > out.writeInt(mSameSite);
> > > > > > On 2016/04/18 20:50:22, boliu wrote:
> > > > > > > this should be safe since CookiesFetcher catches Throwable to make
> > sure
> > > it
> > > > > > > doesn't crash..?
> > > > > > 
> > > > > > Confused why you're worried about the write - it's the read of the
old
> > > > boolean
> > > > > > as int which would be my concern. :)
> > > > > > Now you'd be trying to read 2 ints instead of a boolean & an int so
> the
> > > > second
> > > > > > int will hit EOF. What's worse is that there's no separator between
> > > > canonical
> > > > > > cookies so you'll actually start reading into the next canonical
> > cookies's
> > > > > data
> > > > > > and you'd be off alignment and reading grabage :S
> > > > > > 
> > > > > > The safest thing may be to blow away the cookie store as a one-time
> > > > operation
> > > > > > for this migration and add a version # to allow future upgrades to
be
> > > > > seamless?
> > > > > 
> > > > > Wow, this code is so bad it's making me want to cry. Catching an
> exception
> > > to
> > > > > detect end of stream? Who wrote this??
> > > > 
> > > > :(
> > > > Think of all the beautiful cookie files out there that are so rigidly
> > written
> > > :)
> > > 
> > > I'm working on this on the side, because my main project is stalled, and
> > > thinking, oh, these small things on the side will cheer me up because easy
> > > progress. Oh was I wrong.. ok rant over.
> > > 
> > > So how would we detect version skew if there is no version to begin with
> > then..?
> > > I can add a version as the first thing, but there is always a chance it
> > collides
> > > with the actual cookie value or something. Do you know if data streams are
> > type
> > > safe, like bool can never be read back as an int or something like that?
> > 
> > Nope, won't work. :(
> > Easiest way may be to rename the file once and introduce a version number in
> > that version.
> > You would change fetch fetchFileName to try the deprecated name after trying
> to
> > fetch the new name. So modulo the one upgrade, it's no additional IO.
> > Oh, actually! Check out MAGIC_STRING.. you could change the value of it and
it
> > would invalidate any old data. :)
> > 
> > In the same change, I'd also write a "record count" to avoid that EOF ;)
> 
> So I assume you think it's ok not to recover the old content, and just blow
> everything away?

This is a best-effort thing IMHO but you can ask your friendly neighbourghood PM
if they object
Of course you do have a version # - if it's the old MAGIC_STRING, then call
CanonicalCookie.createFromStreamOld and keep that around for a release or two :)

[boliu, 2016-04-19 15:52:07]

On 2016/04/19 15:48:35, Yaron wrote:
> On 2016/04/19 15:41:37, boliu wrote:
> > On 2016/04/19 15:29:14, Yaron wrote:
> > > On 2016/04/19 15:23:25, boliu wrote:
> > > > On 2016/04/19 15:20:06, Yaron wrote:
> > > > > On 2016/04/19 15:07:40, boliu wrote:
> > > > > > On 2016/04/19 14:36:13, Yaron wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > > > > > File
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
> > > > > > > (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
> > > > > > > out.writeInt(mSameSite);
> > > > > > > On 2016/04/18 20:50:22, boliu wrote:
> > > > > > > > this should be safe since CookiesFetcher catches Throwable to
make
> > > sure
> > > > it
> > > > > > > > doesn't crash..?
> > > > > > > 
> > > > > > > Confused why you're worried about the write - it's the read of the
> old
> > > > > boolean
> > > > > > > as int which would be my concern. :)
> > > > > > > Now you'd be trying to read 2 ints instead of a boolean & an int
so
> > the
> > > > > second
> > > > > > > int will hit EOF. What's worse is that there's no separator
between
> > > > > canonical
> > > > > > > cookies so you'll actually start reading into the next canonical
> > > cookies's
> > > > > > data
> > > > > > > and you'd be off alignment and reading grabage :S
> > > > > > > 
> > > > > > > The safest thing may be to blow away the cookie store as a
one-time
> > > > > operation
> > > > > > > for this migration and add a version # to allow future upgrades to
> be
> > > > > > seamless?
> > > > > > 
> > > > > > Wow, this code is so bad it's making me want to cry. Catching an
> > exception
> > > > to
> > > > > > detect end of stream? Who wrote this??
> > > > > 
> > > > > :(
> > > > > Think of all the beautiful cookie files out there that are so rigidly
> > > written
> > > > :)
> > > > 
> > > > I'm working on this on the side, because my main project is stalled, and
> > > > thinking, oh, these small things on the side will cheer me up because
easy
> > > > progress. Oh was I wrong.. ok rant over.
> > > > 
> > > > So how would we detect version skew if there is no version to begin with
> > > then..?
> > > > I can add a version as the first thing, but there is always a chance it
> > > collides
> > > > with the actual cookie value or something. Do you know if data streams
are
> > > type
> > > > safe, like bool can never be read back as an int or something like that?
> > > 
> > > Nope, won't work. :(
> > > Easiest way may be to rename the file once and introduce a version number
in
> > > that version.
> > > You would change fetch fetchFileName to try the deprecated name after
trying
> > to
> > > fetch the new name. So modulo the one upgrade, it's no additional IO.
> > > Oh, actually! Check out MAGIC_STRING.. you could change the value of it
and
> it
> > > would invalidate any old data. :)
> > > 
> > > In the same change, I'd also write a "record count" to avoid that EOF ;)
> > 
> > So I assume you think it's ok not to recover the old content, and just blow
> > everything away?
> 
> This is a best-effort thing IMHO but you can ask your friendly neighbourghood
PM
> if they object
> Of course you do have a version # - if it's the old MAGIC_STRING, then call
> CanonicalCookie.createFromStreamOld and keep that around for a release or two
:)

Who's the PM for this feature?

[Yaron, 2016-04-19 15:57:43]

On 2016/04/19 15:52:07, boliu wrote:
> On 2016/04/19 15:48:35, Yaron wrote:
> > On 2016/04/19 15:41:37, boliu wrote:
> > > On 2016/04/19 15:29:14, Yaron wrote:
> > > > On 2016/04/19 15:23:25, boliu wrote:
> > > > > On 2016/04/19 15:20:06, Yaron wrote:
> > > > > > On 2016/04/19 15:07:40, boliu wrote:
> > > > > > > On 2016/04/19 14:36:13, Yaron wrote:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > > > > > > File
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
> > > > > > > > (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1894213003/diff/1/chrome/android/java/src/org...
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:123:
> > > > > > > > out.writeInt(mSameSite);
> > > > > > > > On 2016/04/18 20:50:22, boliu wrote:
> > > > > > > > > this should be safe since CookiesFetcher catches Throwable to
> make
> > > > sure
> > > > > it
> > > > > > > > > doesn't crash..?
> > > > > > > > 
> > > > > > > > Confused why you're worried about the write - it's the read of
the
> > old
> > > > > > boolean
> > > > > > > > as int which would be my concern. :)
> > > > > > > > Now you'd be trying to read 2 ints instead of a boolean & an int
> so
> > > the
> > > > > > second
> > > > > > > > int will hit EOF. What's worse is that there's no separator
> between
> > > > > > canonical
> > > > > > > > cookies so you'll actually start reading into the next canonical
> > > > cookies's
> > > > > > > data
> > > > > > > > and you'd be off alignment and reading grabage :S
> > > > > > > > 
> > > > > > > > The safest thing may be to blow away the cookie store as a
> one-time
> > > > > > operation
> > > > > > > > for this migration and add a version # to allow future upgrades
to
> > be
> > > > > > > seamless?
> > > > > > > 
> > > > > > > Wow, this code is so bad it's making me want to cry. Catching an
> > > exception
> > > > > to
> > > > > > > detect end of stream? Who wrote this??
> > > > > > 
> > > > > > :(
> > > > > > Think of all the beautiful cookie files out there that are so
rigidly
> > > > written
> > > > > :)
> > > > > 
> > > > > I'm working on this on the side, because my main project is stalled,
and
> > > > > thinking, oh, these small things on the side will cheer me up because
> easy
> > > > > progress. Oh was I wrong.. ok rant over.
> > > > > 
> > > > > So how would we detect version skew if there is no version to begin
with
> > > > then..?
> > > > > I can add a version as the first thing, but there is always a chance
it
> > > > collides
> > > > > with the actual cookie value or something. Do you know if data streams
> are
> > > > type
> > > > > safe, like bool can never be read back as an int or something like
that?
> > > > 
> > > > Nope, won't work. :(
> > > > Easiest way may be to rename the file once and introduce a version
number
> in
> > > > that version.
> > > > You would change fetch fetchFileName to try the deprecated name after
> trying
> > > to
> > > > fetch the new name. So modulo the one upgrade, it's no additional IO.
> > > > Oh, actually! Check out MAGIC_STRING.. you could change the value of it
> and
> > it
> > > > would invalidate any old data. :)
> > > > 
> > > > In the same change, I'd also write a "record count" to avoid that EOF ;)
> > > 
> > > So I assume you think it's ok not to recover the old content, and just
blow
> > > everything away?
> > 
> > This is a best-effort thing IMHO but you can ask your friendly
neighbourghood
> PM
> > if they object
> > Of course you do have a version # - if it's the old MAGIC_STRING, then call
> > CanonicalCookie.createFromStreamOld and keep that around for a release or
two
> :)
> 
> Who's the PM for this feature?

ewald is PM for identity

[boliu, 2016-04-20 21:36:12]

I'll start a thread with ewald today. But data is already getting lost in M51.

mkwst when are back: The current M51 behavior is LAX_MODE (1) and STRICT_MODE
(2) are being merged into LAX_MODE when it's saved to disk and read back. Is
that ok/safe/compatible/etc?

[boliu, 2016-04-25 18:41:28]

Description was changed from

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

BUG=459154
==========

to

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

BUG=459154
==========

[boliu, 2016-04-25 18:41:28]

boliu@chromium.org changed reviewers:
+ mkwst@chromium.org

[boliu, 2016-04-25 18:42:15]

On 2016/04/20 21:36:12, boliu wrote:
> I'll start a thread with ewald today. But data is already getting lost in M51.
> 
> mkwst when are back: The current M51 behavior is LAX_MODE (1) and STRICT_MODE
> (2) are being merged into LAX_MODE when it's saved to disk and read back. Is
> that ok/safe/compatible/etc?

Add mkwst as explicit reviewer.

mkwst: ping! to answer question above

[Mike West, 2016-04-26 07:59:48]

On 2016/04/25 at 18:42:15, boliu wrote:
> On 2016/04/20 21:36:12, boliu wrote:
> > I'll start a thread with ewald today. But data is already getting lost in
M51.
> > 
> > mkwst when are back: The current M51 behavior is LAX_MODE (1) and
STRICT_MODE
> > (2) are being merged into LAX_MODE when it's saved to disk and read back. Is
> > that ok/safe/compatible/etc?
> 
> Add mkwst as explicit reviewer.
> 
> mkwst: ping! to answer question above

Hrm. Well, this is pretty crappy. Sorry I missed this when adding the
functionality in the first place.

Folding `Lax` and `Strict` into the same value has basically 0 effect in the
short term, as no one is using this functionality (it launches in Chrome 52).
That gives us a bit of flexibility with regard to addressing the issue (e.g.
wiping the field and starting over would be fine).

Long term, folding those together would be a bad thing. We need to distinguish
them in the database, as they have distinct behavior and distinct guarantees.

[boliu, 2016-04-26 15:04:02]

On 2016/04/26 07:59:48, Mike West (slow until 25th) wrote:
> On 2016/04/25 at 18:42:15, boliu wrote:
> > On 2016/04/20 21:36:12, boliu wrote:
> > > I'll start a thread with ewald today. But data is already getting lost in
> M51.
> > > 
> > > mkwst when are back: The current M51 behavior is LAX_MODE (1) and
> STRICT_MODE
> > > (2) are being merged into LAX_MODE when it's saved to disk and read back.
Is
> > > that ok/safe/compatible/etc?
> > 
> > Add mkwst as explicit reviewer.
> > 
> > mkwst: ping! to answer question above
> 
> Hrm. Well, this is pretty crappy. Sorry I missed this when adding the
> functionality in the first place.
> 
> Folding `Lax` and `Strict` into the same value has basically 0 effect in the
> short term, as no one is using this functionality (it launches in Chrome 52).
> That gives us a bit of flexibility with regard to addressing the issue (e.g.
> wiping the field and starting over would be fine).

Ok that's good. That means there is no need to merge anything back to m51.

Looks like we are getting ok to drop cookies from m51 to m52 upgrade, so I'll
probably do that.

> 
> Long term, folding those together would be a bad thing. We need to distinguish
> them in the database, as they have distinct behavior and distinct guarantees.

[boliu, 2016-04-27 01:34:06]

unit test still wip, still need to figure out how to initialize a cipher for the
end to end test

but other bits are mostly ready

Not the greatest code, but certainly much better than before. I mean.. there are
tests!

[boliu, 2016-04-28 00:00:34]

Description was changed from

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

BUG=459154
==========

to

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

Implement new format that's easier to support migrations:
* Add an explicit version number that's just a date
* Explicitly encode the length so don't need EOF

Add migration logic from old format.

Add unit tests for serialization and migration logic. This
required all of that logic into CanonicalCookie.

BUG=459154
==========

[boliu, 2016-04-28 00:00:43]

yaron: ptal

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java
(right):

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java:95:
cookies.add(new CanonicalCookie("http://url", "name", "value", "domain", "path",
you can blame weird formatting here on clang..

[Yaron, 2016-04-28 03:43:10]

This is pretty awesome! Thanks for doing this

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:232:
public static List<CanonicalCookie> readFromFileUnknownFormat(File fileIn,
Cipher cipher)
Couldn't you at least omit "public" and leave this package-protected?

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:245:
in = null;
Do you really need to null this out?

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:256:
in = null;
same here

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java
(right):

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java:33:
private static byte[] saveToStream(final List<CanonicalCookie> cookies)
reuse CanonicalCookies.writeToByteArray instead of re-writing?

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java:173:
true /* httpOnly */, 2 /* sameSite */, 0 /* priority */));
This seems a little strange. 2 couldn't exist in legacy format so you're sorta
testing the test code. I guess you're trying to show true->1 ?

[boliu, 2016-04-28 17:49:26]

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java
(right):

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:232:
public static List<CanonicalCookie> readFromFileUnknownFormat(File fileIn,
Cipher cipher)
On 2016/04/28 03:43:10, Yaron wrote:
> Couldn't you at least omit "public" and leave this package-protected?

I guess .cookies is its own package, so there actually is value to distinguish
between public and package protected. Ok..

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:245:
in = null;
On 2016/04/28 03:43:10, Yaron wrote:
> Do you really need to null this out?

I want to make sure that the "in" here and the "in" below are definitely
different objects. Used different var names instead to make that clear, so can
remove setting it to null.

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/cookies/CanonicalCookie.java:256:
in = null;
On 2016/04/28 03:43:10, Yaron wrote:
> same here

ditto

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java
(right):

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java:33:
private static byte[] saveToStream(final List<CanonicalCookie> cookies)
On 2016/04/28 03:43:10, Yaron wrote:
> reuse CanonicalCookies.writeToByteArray instead of re-writing?

Disagree. writeToByteArray also does encryption, which is explicitly not tested
here. writeToByteArray is supposed to match readFromFileUnknownFormat, but it's
not writeToFile is to avoid pulling in ImportantFileWriterAndroid into
CanonicalCookie. Presumably ImportantFileWriterAndroid in unit tests. Added a
comment about that.

I've renamed saveToStream to saveListToStream, since it already collides with a
member method.

https://codereview.chromium.org/1894213003/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java:173:
true /* httpOnly */, 2 /* sameSite */, 0 /* priority */));
On 2016/04/28 03:43:10, Yaron wrote:
> This seems a little strange. 2 couldn't exist in legacy format so you're sorta
> testing the test code. I guess you're trying to show true->1 ?

Oh good point. true -> 1 case is covered above already above. Removed cases
where we try to write sameSite > 1 in legacy format.

[boliu, 2016-04-28 17:58:20]

Question.. why is ChromeTabbedActivity saving/restoring cookies?
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

Isn't this only supposed to run for incognito activities? Or is incognito part
of ChromeTabbedActivity in the non-hera world?

(me trying to manuall test this now)

[boliu, 2016-04-28 18:49:49]

hmm, I can't get the restore logic to run at all after adb install -r -d . Does
incognito ever persist over chrome upgrade I wonder..

[boliu, 2016-04-28 20:35:28]

On 2016/04/28 18:49:49, boliu wrote:
> hmm, I can't get the restore logic to run at all after adb install -r -d .
Does
> incognito ever persist over chrome upgrade I wonder..

Yeah I can't get Profile.getLastUsedProfile().hasOffTheRecordProfile() to
persist after adb install. That bit definitely is persisted if I just adb kill
and restart chrome though. Yaron, do you know how that bit is supposed to be
persisted?

[Yaron, 2016-04-28 20:37:46]

On 2016/04/28 17:58:20, boliu wrote:
> Question.. why is ChromeTabbedActivity saving/restoring cookies?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> Isn't this only supposed to run for incognito activities? Or is incognito part
> of ChromeTabbedActivity in the non-hera world?
> 
> (me trying to manuall test this now)

yes, incognito is party of chrome tabbed in non-hera

[Yaron, 2016-04-28 20:37:50]

On 2016/04/28 17:58:20, boliu wrote:
> Question.. why is ChromeTabbedActivity saving/restoring cookies?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> Isn't this only supposed to run for incognito activities? Or is incognito part
> of ChromeTabbedActivity in the non-hera world?
> 
> (me trying to manuall test this now)

yes, incognito is party of chrome tabbed in non-hera

[Yaron, 2016-04-28 20:38:23]

On 2016/04/28 18:49:49, boliu wrote:
> hmm, I can't get the restore logic to run at all after adb install -r -d .
Does
> incognito ever persist over chrome upgrade I wonder..

not sure how to test it, unfortunately

[Yaron, 2016-04-28 20:52:30]

On 2016/04/28 20:38:23, Yaron wrote:
> On 2016/04/28 18:49:49, boliu wrote:
> > hmm, I can't get the restore logic to run at all after adb install -r -d .
> Does
> > incognito ever persist over chrome upgrade I wonder..
> 
> not sure how to test it, unfortunately

Overall, the change lgtm

Sorry I'm not more helpful about how that bit is persisted

[boliu, 2016-04-28 21:24:54]

Asked dan about persistence. So the encryption key for the incognito cookies is
persisted through the android bundle in onCreate/onDestroy etc. And anecdotally,
that bundle doesn't (never?) persists through app installs. Which means
incognito tags never persist through app installs, which means all this effort
was for naught and PS1 would have been fine probably..

It's kind of surprising to me that bundles don't persist through app install
though. So I'm checking more first..

[boliu, 2016-04-28 22:47:54]

On 2016/04/28 21:24:54, boliu wrote:
> Asked dan about persistence. So the encryption key for the incognito cookies
is
> persisted through the android bundle in onCreate/onDestroy etc. And
anecdotally,
> that bundle doesn't (never?) persists through app installs. Which means
> incognito tags never persist through app installs, which means all this effort
> was for naught and PS1 would have been fine probably..
> 
> It's kind of surprising to me that bundles don't persist through app install
> though. So I'm checking more first..

Ok, I guess it kind of makes sense that the bundle doesn't persist across app
installs, since the activities in th eapp could have in theory changed
completely after the app changes. Meh, that means gonna be scaling back this CL
a lot. Meh..

[boliu, 2016-04-28 23:13:41]

Description was changed from

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

Implement new format that's easier to support migrations:
* Add an explicit version number that's just a date
* Explicitly encode the length so don't need EOF

Add migration logic from old format.

Add unit tests for serialization and migration logic. This
required all of that logic into CanonicalCookie.

BUG=459154
==========

to

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

Also update the format:
* Add an explicit version number that's just a date
* Explicitly encode the length so don't need EOF

Note the cookies can never persist across app updates, so no
need for any migration logic.

BUG=459154
==========

[boliu, 2016-04-28 23:13:56]

migration removed, ptal

[Yaron, 2016-04-29 14:20:06]

On 2016/04/28 23:13:56, boliu wrote:
> migration removed, ptal

still lgtm

[boliu, 2016-04-29 14:34:45]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2016-04-29 14:34:54]

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

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

Description was changed from

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

Also update the format:
* Add an explicit version number that's just a date
* Explicitly encode the length so don't need EOF

Note the cookies can never persist across app updates, so no
need for any migration logic.

BUG=459154
==========

to

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

Also update the format:
* Add an explicit version number that's just a date
* Explicitly encode the length so don't need EOF

Note the cookies can never persist across app updates, so no
need for any migration logic.

BUG=459154
==========

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

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-30 17:26:00]

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

[commit-bot: I haz the power, 2016-04-30 18:00:45]

Description was changed from

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

Also update the format:
* Add an explicit version number that's just a date
* Explicitly encode the length so don't need EOF

Note the cookies can never persist across app updates, so no
need for any migration logic.

BUG=459154
==========

to

==========
android: Fix CanonicalCookie same_site field

This field became an enum in r381201, but the java side was never
updated. Update java side to int instead.

Also update the format:
* Add an explicit version number that's just a date
* Explicitly encode the length so don't need EOF

Note the cookies can never persist across app updates, so no
need for any migration logic.

BUG=459154

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