Closed Bug 1220570 Opened 4 years ago Closed 4 years ago

Potential cookie lost while downgrading from Aurora 44 to 43

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 --- unaffected
firefox44 + fixed
firefox45 + fixed
b2g-v2.5 --- fixed

People

(Reporter: ethan, Assigned: ethan)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → ettseng
Status: NEW → ASSIGNED
Attached patch bug-1220570-fix.patch (obsolete) — Splinter Review
nsCookieService::CreateTable() should create appId and inBrowserElement columns.
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.
Flags: needinfo?(jduell.mcbugs)
Blocks: 1217594
Attached patch bug-1220570-fix.patch (obsolete) — Splinter Review
We need a specific function for cookie schema version 6.
There are more than two-lines code changes.
Attachment #8682316 - Attachment is obsolete: true
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?
Attachment #8682424 - Flags: review?(jduell.mcbugs)
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.
Attachment #8682424 - Flags: review?(jduell.mcbugs) → review+
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)
Flags: needinfo?(jduell.mcbugs)
(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.
Keywords: checkin-needed
Refresh commit message "r=jduell".
Attachment #8682424 - Attachment is obsolete: true
Request to check in the patch attachment 8683088 [details] [diff] [review].
https://hg.mozilla.org/mozilla-central/rev/dd120ab937ed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(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.
Attachment #8683088 - Flags: approval‑mozilla‑b2g44?
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
Attachment #8683088 - Flags: approval-mozilla-aurora?
Comment on attachment 8683088 [details] [diff] [review]
bug-1220570-fix.patch

Given that this is a data loss scenario, let's uplift to Aurora44.
Attachment #8683088 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Does this still affect 43?
Flags: needinfo?(jduell.mcbugs)
Flags: needinfo?(ettseng)
(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.
Flags: needinfo?(ettseng)
Flags: needinfo?(jduell.mcbugs)
Comment on attachment 8683088 [details] [diff] [review]
bug-1220570-fix.patch

Setting the approval flag. Patch landed earlier today. 

Thanks
Attachment #8683088 - Flags: approval‑mozilla‑b2g44? → approval‑mozilla‑b2g44+
You need to log in before you can comment on or make changes to this bug.