Closed Bug 1154472 Opened 9 years ago Closed 9 years ago

Migration from IE sets expiry time of cookies 1000 times too high

Categories

(Firefox :: Migration, defect)

35 Branch
x86
Windows
defect
Not set
normal
Points:
1

Tracking

()

RESOLVED FIXED
Firefox 40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox40 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [fxgrowth])

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #1153900 +++

At least for me on Win8, the expiry time is turned into a date which we pass to Services.cookies.add, which turns it into a timestamp, which we then multiply by 1000 before turning it back into a date, leading to it being really high (year 40000-odd).

I'm not sure if this is a regression or not, but from the code I suspect it's been like this for a long time, and nobody noticed...
Status: NEW → ASSIGNED
Flags: qe-verify?
Flags: firefox-backlog+
I guess the usual timestamp vs PRTime...

Btw, I have ctypes working for the test, I can set a cookie with an expire time, so this should be testable too.
Tested, upcoming patch fixes things on my local Win8 IE11 machine. I'll report back with a WinXP test
in a bit, but it seems likely that this is the correct fix everywhere - if the format had changed on the IE side, the new Date() call would never have worked in the first place, I assume...
Attached file MozReview Request: bz://1154472/Gijs (obsolete) —
/r/7057 - Bug 1154472 - fix expiry dates in IE cookie imports, r?mak

Pull down this commit:

hg pull -r bdb3880ff357cd57640928aca89979ac5a257ee9 https://reviewboard-hg.mozilla.org/gecko/
Actually... while we're here, I think this gets timezones wrong still? Maybe? Let me doublecheck...
(In reply to :Gijs Kruitbosch from comment #4)
> Actually... while we're here, I think this gets timezones wrong still?
> Maybe? Let me doublecheck...

Yup, both debugging and https://msdn.microsoft.com/en-us/library/windows/desktop/ms724280%28v=vs.85%29.aspx indicate that this will be returning UTC, and the new Date() constructor will use local time. I'll update the patch.
Comment on attachment 8592728 [details]
MozReview Request: bz://1154472/Gijs

/r/7057 - Bug 1154472 - fix expiry dates in IE cookie imports, r?mak

Pull down this commit:

hg pull -r e6dc44a067161a17c517b87edda490bb84b1011f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8592728 - Flags: review?(mak77)
Comment on attachment 8592728 [details]
MozReview Request: bz://1154472/Gijs

https://reviewboard.mozilla.org/r/7055/#review5869

::: browser/components/migration/IEProfileMigrator.js
(Diff revision 2)
> -  fileTimeToDate: function CH_fileTimeToDate(aTimeHi, aTimeLo) {
> +  fileTimeToSecondsSinceEpoch: function CH_fileTimeToSecondsSinceEpoch(aTimeHi, aTimeLo) {

nit: I'm fine if you wish to shorthand to

fileTimeToSecondsSinceEpoch(aTimeHi, aTimeLo) {
Attachment #8592728 - Flags: review?(mak77) → review+
https://hg.mozilla.org/integration/fx-team/rev/f18502092330

has an automated test in bug 1154294
Flags: qe-verify?
Flags: qe-verify-
Flags: in-testsuite+
Target Milestone: --- → Firefox 40
https://hg.mozilla.org/mozilla-central/rev/f18502092330
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #8592728 - Attachment is obsolete: true
Attachment #8620040 - Flags: review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: