Closed Bug 925943 Opened 8 years ago Closed 4 years ago

Cookie inspector lists wrong dates.

Categories

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: mythmon, Assigned: geoffroy)

Details

Attachments

(3 files)

Attached image bar.png
Running todays UX nightly.

I used `cookie list` in the developer toolbar to view a cookie. It said that it expire on January 16th, 1970. The browser clearly treated it as valid though. Out of curiosity, i assumed that the date was stored in seconds since the epoch. The result of this was a date in 2014, which makes much more sense.

In other words, the devtools are intepreting the dates in cookies as three orders of magnitude off, milliseconds since the epoch vs seconds since the epoch. A screenshot showing this is attached.

STR: Run `cookie list` in the developer console.
Expected: The expiration date is correct.
Actual: It is not.
Thanks for the bug report.

This seems to be a bug about the GCLI 'cookie' command.
Component: Developer Tools: Console → Developer Tools: Graphic Commandline and Toolbar
I may tackle this bug soon. Here are a few hints in the meantime :

The issue lies here, in the cookie gcli command :
http://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/BuiltinCommands.jsm#925

The cookie.expires property belongs to nsICookie but is deprecated :
http://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsICookie.idl#52

cookie.expiry, a more recent property introduced in nsICookie2 should be used :
http://dxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsICookie2.idl#34

This value is returned in seconds, but the js Date constructor needs this value in milliseconds, so a x 1000 is missing in the translateExpires function as Mike described well :
http://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/BuiltinCommands.jsm#953


So the goal is to rewrite the command line code using .expiry & .isSession and turning the value into milliseconds.
I'm working on this, but I'll also have to update cookie set as it uses nsICookie.expires and not nsICookie.expiry / isSession.

Thanks :Delapouite for the indications ;)
Using nsICookie2's expiry and isSession methods, as well as computing the expire date correctly.
Attachment #8577235 - Flags: review?(pbrosset)
Assignee: nobody → geoffroy
Status: NEW → ASSIGNED
Added a test case for this bug.

Testing for the default expires value of cookie set, which is Jan 17, 2038.
Attachment #8577271 - Flags: review?(pbrosset)
Comment on attachment 8577271 [details] [diff] [review]
Bug-925943-Testing-cookie-set-default-expiry-time.patch

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

This looks good to me, but I'm still going to forward this review to jwalker for 2 reasons:
- Does this new test need a new HTML support page? It looks to me like it could just reuse another existing HTML page in the test dir.
- Should the test remove the cookie at the end, to avoid influencing other tests in the suite?
Attachment #8577271 - Flags: review?(pbrosset) → review?(jwalker)
I used a new html page because the other one sets some cookies. And I'm not sure but I think the cookies are relative to the test and thus don't need to be deleted.

I'll wait for jwalker review anyway, thanks ;)
Comment on attachment 8577271 [details] [diff] [review]
Bug-925943-Testing-cookie-set-default-expiry-time.patch

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

I think Patrick is right on both counts.
Probably the best thing to do is to insert the important part of the test, i.e.

    // default expiry is Jan 17, 2038
    output: [ /1\/17\/2038/ ]

into browser_cmd_cookie.js, around line 98.

The following might work:

    output: [ /zap=zep/, /zip=zop/, /Edit/, /1\/17\/2038/ ]
Attachment #8577271 - Flags: review?(jwalker)
Comment on attachment 8577235 [details] [diff] [review]
Bug-925943-using-nsICookie2-expiry-isSession.patch

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

LGTM.
Attachment #8577235 - Flags: review?(pbrosset) → review+
Is this still something worth fixing in regard of bug 1429421?

Sebastian
Flags: needinfo?(pbrosset)
Probably not. And this bug hasn't moved in the past 3 years. Let's wontfix for now.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(pbrosset)
Resolution: --- → WONTFIX
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.