599799 - Cookies are doubled when switching between FF3.6 and FF4beta

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Jaroslav Benc]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b6) Gecko/20100101 Firefox/4.0b6

Setting cookies from FF4 creates duplicate cookies when swithing back to FF3.6. This is actually quite serious as servers would use first cookie from the request which might be invalid. Cookies from with same path shouldn't be doubled anyway. 

Reproducible: Always

Steps to Reproduce:
1. Open FF3 and set cookie (with expire date)
2. Close FF3 and open FF4 and set the same cookie
3. Go back to FF3 and read cookies 
Actual Results:  
Will have two/multiple records with the same name=value and same path. 

Expected Results:  
Will have only one record

See attached example for more info

Comment 1

[Jaroslav Benc]

Attachment: Run this example in FF3 then FF4 and then back in FF3

Comment 2

[[e-mail]]

This should work. In FF4, we added a baseDomain column to the db. When setting a cookie in FF3, this column doesn't exist. When switching to 4, we perform a migration and add the baseDomain column. When going back to 3, it will ignore the column and should work fine.

Investigating.

Comment 3

[[e-mail]]

I can't reproduce this as described. There is, however, a similar problem:

1) Create a profile and start FF4; set a cookie.
2) Load the profile in FF3, delete the cookie, and set a new one.
3) Load the profile in FF4, observe that the cookie doesn't show up, then set the same cookie.
4) Load the profile in FF3, and observe that two cookies exist.

The reason is that a row with a blank baseDomain column is ignored in FF4, leading to another entry being added. Having duplicate cookies is bad. I don't think we need to fix up the entries, but we do need to handle it gracefully: purging rows with empty baseDomain and host columns would work fine. Fixing up the entries would be a bit tricky to do asynchronously; because we now perform database mutation operations by (name, host, path) triplet, we could potentially get two results coming back, which is bad. This necessitates that we perform the adjustment before queueing any mutation ops, but we can't do that without completing the read and figuring out what needs fixing up. Which gets us back to a complicated transaction queue approach.

Comment 4

[[e-mail]]

This needs to depend on bug 598196, since without WAL opening a sync db connection can easily fail when the other thread is writing.

This should also block betaN, since I think many users (out of 400 million) will run into this.

Comment 5

[[e-mail]]

Attachment: drop null baseDomains

Easy fix -- just exclude rows with NULL baseDomain from the global SELECT, and then explicitly kill them. (We can't just leave them hanging around, because when going back to FF3 they'll show up again as duplicates.)

The logging love is to be safe and catch any cases where WAL betrays us.

Comment 6

[Jaroslav Benc]

(In reply to comment #4)
> This should also block betaN, since I think many users (out of 400 million)
> will run into this.

I've just found that login to Yahoo Mail dosn't work anymore when switching between 3 and 4beta. Try to login/logout in FF3, then login/logout in FF4 and go back to FF3.

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 479203 [details] [diff] [review]
drop null baseDomains

r=sdwilsh, but you need to make a test for this too please!

Comment 8

[[e-mail]]

Attachment: part 2: migration tests

Migration tests for schema 2 (1.9.3) -> 4 (2.0) and back, and 3 (2.0pre) -> 4 (2.0).

Briefly:
1) Going back and forth between 2 and 4 should work ad infinitum. Going forward, we migrate; going back, any cookies added will have a NULL baseDomain; going forward, they'll be deleted on read. It's also worth noting that, in 2, it's possible to attempt to INSERT a cookie over an already-expired one. The INSERT will fail, since we now have a UNIQUE constraint. This just means the new cookie won't be saved, and the expired cookie will be deleted on next start. We can trivially patch the 3.6 branch to fix this -- it just requires using INSERT OR REPLACE with no other changes.

2) Going from 3 --> 4 is much the same as 2 --> 4, except baseDomain isn't an issue. When going 4 --> 3, added cookies will have a NULL creationTime, which is nonfatal. I don't expect that switching back and forth between current trunk and an older trunk will be common...

Comment 9

[[e-mail]]

Attachment: part 3: eviction test fixes

My patches in the various bugs have started tickling a timing problem in test_eviction.js: if setting 2000 cookies takes longer than the eviction threshold, we fail. Windows also has sucky timer resolution, which compounds the problem.

For kicks, I rewrote the test to be much smarter. We now:

* Use do_timeout with a generator instead of a blocking sleep()-like call.
* Set smaller amounts of cookies.
* When setting batches of cookies, we make sure the elapsed duration is less than the eviction threshold.
* If it's close to the threshold, we double it and rerun the entire test.
* After we've doubled a bunch of times, we hard fail.

AFAICT, this is the best we can do given that the test is inherently dependent on timing. It also means that the test will run fast on fast builds (opt) or OSes with good timer resolution (Linux/Mac), and will slow down only when necessary. Note that we won't rerun the test if there's a legitimate failure (i.e. the eviction results were unexpected), only if the timing is borked.

This was also a lot of fun to write!

Comment 10

[[e-mail]]

Attachment: branch patch

Per comment 8. Say someone backmigrates from a schema 4 build to schema 2. With this patch, when running indefinitely on schema 2, INSERTing cookies that replace same-(name, host, path) expired cookies will overwrite them. The expired cookies will still remain in the in-memory table. Subsequent purge operations will attempt to delete them from the database, which will fail, but it's nonfatal and will just log a warning.

Note that we don't need any conflict clauses on trunk, because it's invariant that we should never have such a conflict -- and if we do, we definitely want to log it.

If we don't take this patch, people who run on schema 2 will indefinitely be unable to update cookies in the database when they expire within a session. (Restarting the browser will purge them.) I don't think we want to ship with that regression.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 483057 [details] [diff] [review]
part 2: migration tests

I skimmed this, but didn't do a thorough review since it's test only changes.

rs=sdwilsh

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 483060 [details] [diff] [review]
part 3: eviction test fixes

I skimmed this, but didn't do a thorough review since it's test only changes.

rs=sdwilsh

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 483063 [details] [diff] [review]
branch patch

r=sdwilsh

Comment 14

[[e-mail]]

Comment on attachment 483063 [details] [diff] [review]
branch patch

Looking for approval1.9.2.12. See comment 10 for details, but in a nutshell: this is a low-risk change that won't affect the function of 1.9.2-based browsers, except under conditions where a user backmigrates from a 2.0-based profile to 1.9.2. With this patch, things won't break horribly.

We should land this ASAP, independent of the other patches landing on trunk, since we want this to already be in place when people think about backmigrating.

Comment 15

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/3966326fc682
http://hg.mozilla.org/mozilla-central/rev/c96694afbbfa
http://hg.mozilla.org/mozilla-central/rev/902b50f42bca

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Could this have caused bug 605835?

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 483063 [details] [diff] [review]
branch patch

Approved for 1.9.2.12, a=dveditz for release-drivers

tests?

Comment 18

[[e-mail]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b127de9b5a5f

I've tested it manually and it works fine, and it's covered by migration tests on trunk, so I think we're OK there.