1217594 - Cookies are lost when switching recent Nightlies

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

I've had this happen to me twice in the last couple of weeks. Both cases involved switching between older builds and the current one. This also happened to Benoit Girard.

Comment 1

[(no longer active)]

FWIW as far as I can tell, bug 1165267 has destroyed the possibility of going back by getting rid of the original database fields, so we can probably not fix this bug for users on Nightly.

We should not let this bug get to Aurora otherwise all Developer Edition users will be affected too.  I think we should find a safe way to back out bug 1165267 before the next uplift.

Comment 2

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

You mean before the merge November 2?  We should try to fix this for 44 aurora before this weekend, then.
 
Tracking for 44 since this is a regression.

Comment 3

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

Putting needinfo on this for myself as an extra reminder (covering for Ritu who is the 44 release owner)

Comment 4

[(no longer active)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #2)
> You mean before the merge November 2?

Yes.

> We should try to fix this for 44 aurora before this weekend, then.

I think we really want to back out the original bug.  We went through several iterations for solving the exact same problem in bug 1165263 and I doubt that we can do better here and come up with a correct fix in ~3 days.

Comment 5

[Ethan Tseng [:ethan]]

Sorry to cause this regression.
I will do my best to fix it (or at least do a safe backout as Ehsan suggested).

Yes. I didn't consider about Database downgrade in the patch of bug 1165267.
First, I'd like to confirm with Jeff:
Are cookies lost only when you switch from newer to older Nightlies?
Do cookies persist when you switch in the opposite way (older to newer)?

If the answer is no, which means cookies corrupt totally, we certainly have to backout that patch.
But if the answer is yes, perhaps I can try to fix the downgrade issue.

Comment 6

[Ethan Tseng [:ethan]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #1)
> FWIW as far as I can tell, bug 1165267 has destroyed the possibility of
> going back by getting rid of the original database fields, so we can
> probably not fix this bug for users on Nightly.

Ehsan, my intuitive idea to resolve this issue is to keep the original columns of the previous DB version instead of removing them. What do you think?
Could you suggest the ideal way to deal with DB downgrade situation?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Ethan Tseng [:ethan] from comment #5)
> Sorry to cause this regression.
> I will do my best to fix it (or at least do a safe backout as Ehsan
> suggested).
> 
> Yes. I didn't consider about Database downgrade in the patch of bug 1165267.
> First, I'd like to confirm with Jeff:
> Are cookies lost only when you switch from newer to older Nightlies?
> Do cookies persist when you switch in the opposite way (older to newer)?

I'm not in a rush to lose my cookies again. Can you not reproduce the data loss?

Comment 8

[Ethan Tseng [:ethan]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> I'm not in a rush to lose my cookies again. Can you not reproduce the data
> loss?

Apologies. I don't mean to request you to reproduce it again. :)
I will reproduce it myself.

Comment 9

[(no longer active)]

(In reply to Ethan Tseng [:ethan] from comment #6)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #1)
> > FWIW as far as I can tell, bug 1165267 has destroyed the possibility of
> > going back by getting rid of the original database fields, so we can
> > probably not fix this bug for users on Nightly.
> 
> Ehsan, my intuitive idea to resolve this issue is to keep the original
> columns of the previous DB version instead of removing them. What do you
> think?
> Could you suggest the ideal way to deal with DB downgrade situation?

Yes, that's the correct thing to do.

Look at this code (this is from the revision before your changes): <https://hg.mozilla.org/mozilla-central/annotate/966edcb029a7/netwerk/cookie/nsCookieService.cpp#l1129>

Here the code is looking to see if all of the columns listed there exist.  If the answer is no, it drops the existing table and creates a new one.  What went wrong in your patch before is you removed the appId and inBrowserElement columns and added originAttributes.  That means that the SQL statement I linked to above will fail and as a result, as soon as you downgrade, all of your cookies are gone.

Furthermore, if during migration you change any of the existing data, it would be a good idea to keep a backup of the original data in case something goes wrong.

Comment 10

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

I like the idea of backing this out and trying again on nightly 45 next week.
 
Let's not risk moving this into aurora, where we would make many web developers very sad if we tossed their cookies.

Comment 11

[Ethan Tseng [:ethan]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #9)
> Yes, that's the correct thing to do.
...
> Furthermore, if during migration you change any of the existing data, it
> would be a good idea to keep a backup of the original data in case something
> goes wrong.

Ehsan, thanks for your advice!
We are backing out the changeset 558925a1e26c right now, and I reopened bug 1165267.
I will fix my patch and make it land after Nightly 44.

Sorry again for causing the trouble.

Comment 12

[(no longer active)]

FWIW it seems that the backout attempt was untested.  See my comments on that bug...

Comment 13

[Ethan Tseng [:ethan]]

(In reply to Ehsan Akhgari (don't ask for review please) [Away Nov 3-19] from comment #12)
> FWIW it seems that the backout attempt was untested.  See my comments on
> that bug...

Sorry about my carelessness.
I uploaded a new patch in bug 1165267 which was manually tested (cookie data is not lost by switching between different Nightlies).

Comment 14

[(no longer active)]

Have you also tested going from Aurora 43 to Nightly with your patch applied directly, and then back to Aurora 43?  That is really what we need to care about the most, I think, for users on the Aurora channel.

We should also think how this impacts people who have been already affected by this on Nightly.  I _think_ your new patch fixes things for them as well, as you're re-populating the appId and inBrowserElement columns with the correct info, but I haven't spent enough time thinking about this, so please double check!  :-)

Comment 15

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

Andrei, this seems like something we want to test as best we can. I think you already do some smoketesting for upgrades  but I wanted you to be aware of the possible issues with losing cookies.

Comment 16

[Ethan Tseng [:ethan]]

(In reply to Ehsan Akhgari (don't ask for review please) [Away Nov 3-19] from comment #14)
> Have you also tested going from Aurora 43 to Nightly with your patch applied
> directly, and then back to Aurora 43?  That is really what we need to care
> about the most, I think, for users on the Aurora channel.

I see your comments about my patch in bug 1165267.
After I enhance the patch and add test cases, I will test this most important scenario to make sure it's correct.
Thanks for reminding me this.
 
> We should also think how this impacts people who have been already affected
> by this on Nightly.  I _think_ your new patch fixes things for them as well,
> as you're re-populating the appId and inBrowserElement columns with the
> correct info, but I haven't spent enough time thinking about this, so please
> double check!  :-)

Okay. I'll keep it in mind and will fix it ASAP.
Thanks!

Comment 17

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15)
> Andrei, this seems like something we want to test as best we can. I think
> you already do some smoketesting for upgrades  but I wanted you to be aware
> of the possible issues with losing cookies.

We don't usually perform any manual tests on upgrades and at this point we won't be able to accommodate them, as we're caught up with signing off DevEdition44. Best we can do right now is try and test it later this week.

Liz, please let me know if you have any concerns on this matter.

Comment 18

[Ethan Tseng [:ethan]]

Hi Liz and Andrei,

Cookie lost problem is mostly fixed by bug 1165267, however, I just found another corner case by myself that could also cause cookie lost, which is tracked by bug 1220570.

There is already a patch and waiting for review now.
That patch has to be uplifted to Aurora 44 as well.
Sorry for making trouble.

Comment 19

[Ethan Tseng [:ethan]]

This bug is resolved fixed by bug 1165267 and bug 1220570.

Comment 20

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

Based on comment 19 and also after looking at bug 1165267 and bug 1220570, updating status-firefox44 to fixed.

Comment 21

[Paul Silaghi, QA [:pauly]]

I've reproduce the initial problem on http://www.html-kit.com/tools/cookietester/ when switching nightlies from 44.0a1 (2015-10-06) to 43.0a1 (2015-09-01) Win 7.
Verified fixed when switching from:
46.0a1 (2016-01-17) to 43.0a1 (2015-09-01)
46.0a1 (2016-01-17) to 45.0a2 (2016-01-18)
45.0a2 (2016-01-18) to 44.0a2 (2015-12-14)
45.0a2 (2016-01-18) to 43.0a2 (2015-10-01)
44b9 to 43b9