925943 - Cookie inspector lists wrong dates.

Status: RESOLVED WONTFIX

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

Description

[Michael Cooper [:mythmon]]

Attachment: 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.

Comment 1

[Mihai Sucan [:msucan]]

Thanks for the bug report.

This seems to be a bug about the GCLI 'cookie' command.

Comment 2

[Bruno Heridet [:Delapouite]]

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.

Comment 3

[Geoffroy Planquart]

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 ;)

Comment 4

[Geoffroy Planquart]

Attachment: Bug-925943-using-nsICookie2-expiry-isSession.patch

Using nsICookie2's expiry and isSession methods, as well as computing the expire date correctly.

Comment 5

[Geoffroy Planquart]

Attachment: Bug-925943-Testing-cookie-set-default-expiry-time.patch

Added a test case for this bug.

Testing for the default expires value of cookie set, which is Jan 17, 2038.

Comment 6

[Patrick Brosset <:pbro>]

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?

Comment 7

[Geoffroy Planquart]

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 8

[Joe Walker [:jwalker]]

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/ ]

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8577235 [details] [diff] [review]
Bug-925943-using-nsICookie2-expiry-isSession.patch

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

LGTM.

Comment 10

[Sebastian Zartner [:sebo]]

Is this still something worth fixing in regard of bug 1429421?

Sebastian

Comment 11

[Patrick Brosset <:pbro>]

Probably not. And this bug hasn't moved in the past 3 years. Let's wontfix for now.