1220570 - Potential cookie lost while downgrading from Aurora 44 to 43

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Ethan Tseng [:ethan]]

Bug 1165267 (Use OriginAttributes for nsCookieService) caused a regression, bug 1217594 (Cookies are lost when switching recent Nightlies).

This is almost fixed by the patch in bug 1165267 except for one case:
If the user runs Aurora 44 without cookie profile existed, navigates to the web (so cookies are stored) and then downgrades to Aurora 43, his/her cookie data will still be lost.

Comment 1

[Ethan Tseng [:ethan]]

Attachment: bug-1220570-fix.patch

nsCookieService::CreateTable() should create appId and inBrowserElement columns.

Comment 2

[Ethan Tseng [:ethan]]

Honza and Ehsan,

It's embarrassing but I have to admit that I missed one place to change while fixing bug 1165267.
The deprecated columns appId and inBrowserElement must also be added while creating the cookie table.

This must be fixed ASAP and uplifted to Aurora 44 as well.


Hi Jason,
Honza and Ehsan are both on PTO until Nov 19.
Could you help to review the patch? There are only two lines added.

Comment 3

[Ethan Tseng [:ethan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e70a394cb57

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 43+ since this is a regression.

Comment 5

[Ethan Tseng [:ethan]]

Attachment: bug-1220570-fix.patch

We need a specific function for cookie schema version 6.
There are more than two-lines code changes.

Comment 6

[Ethan Tseng [:ethan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8f2c1f425bf

Comment 7

[Ethan Tseng [:ethan]]

Comment on attachment 8682424 [details] [diff] [review]
bug-1220570-fix.patch

Review of attachment 8682424 [details] [diff] [review]:
-----------------------------------------------------------------

Jason, can you please help review this patch?

Comment 8

[Jason Duell]

Comment on attachment 8682424 [details] [diff] [review]
bug-1220570-fix.patch

Review of attachment 8682424 [details] [diff] [review]:
-----------------------------------------------------------------

I looked over this (and the previous bugs' patches) and AFAICT it all looks good to me.

It would be lovely to have a test case (I assume you tested manually?) but given time constraints I'm OK to leave that for bug 1220361.

Comment 9

[Jason Duell]

Note that this bug only affects users who create a completely new profile using 44 and then downgrade to an older browser.  That's not a common case so I don't consider this mission-critical for uplift to aurora. (That said it would definitely be nice)

Comment 10

[Ethan Tseng [:ethan]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #8)
> Comment on attachment 8682424 [details] [diff] [review]
> bug-1220570-fix.patch
> -----------------------------------------------------------------
> I looked over this (and the previous bugs' patches) and AFAICT it all looks
> good to me.
> It would be lovely to have a test case (I assume you tested manually?) but
> given time constraints I'm OK to leave that for bug 1220361.

Jason, thanks for you review!
Yes, I tested the patch manually and I am working on the test case in bug 1220361, which should be completed soon.

Comment 11

[Ethan Tseng [:ethan]]

Attachment: bug-1220570-fix.patch

Refresh commit message "r=jduell".

Comment 12

[Ethan Tseng [:ethan]]

Request to check in the patch attachment 8683088 [details] [diff] [review].

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd120ab937ed

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dd120ab937ed

Comment 15

[Ethan Tseng [:ethan]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #9)
> Note that this bug only affects users who create a completely new profile
> using 44 and then downgrade to an older browser.  That's not a common case
> so I don't consider this mission-critical for uplift to aurora. (That said
> it would definitely be nice)

I still suggest to uplift this fix to firefox 44 and b2g v2.5.
So that we won't have inconsistent cookie database schema for the same schema version.

Comment 16

[Ethan Tseng [:ethan]]

Comment on attachment 8683088 [details] [diff] [review]
bug-1220570-fix.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1217594 - Cookies are lost when switching recent Nightlies
[User impact if declined]: Potential cookie data lost while downgrading
[Describe test coverage new/current, TreeHerder]:
  1. Manual test for migration between the current and older schema versions.
  2. Automation test cases are being developed in bug 1220361.
[Risks and why]: No risk.
[String/UUID change made/needed]: N/A

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8683088 [details] [diff] [review]
bug-1220570-fix.patch

Given that this is a data loss scenario, let's uplift to Aurora44.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8fed0d29cde8

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this still affect 43?

Comment 20

[Ethan Tseng [:ethan]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #19)
> Does this still affect 43?

No.
The cookie lost issue only occurs while the user downgrades from 44 to 43.

And the patch which caused this regression was landed in 44. So we should not uplift anything to 43.

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, thanks, untracking this bug then.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/8fed0d29cde8

Comment 23

[Mahendra Potharaju [:mahe]]

Comment on attachment 8683088 [details] [diff] [review]
bug-1220570-fix.patch

Setting the approval flag. Patch landed earlier today. 

Thanks