119013 - Cookie expire times reported twice / bug in FormatDateTime routine

Status: VERIFIED FIXED

(Core :: Internationalization, defect)

Description

[Christopher Aillon (sabbatical, not receiving bugmail)]

Linux 2002010906:

Just a few samples of the cookie expire times in cookie manager:

Tue 01 Jan 2036 01:01:30 AM MST 01:01:30 AM
Wed 01 May 2002 09:15:02 PM MDT 09:15:02 PM
Fri 29 Jun 2029 05:59:09 PM MDT 05:59:09 PM


Times should only be reported once.

Comment 1

[Stephen P. Morse]

Can you please give a sequence of steps, starting with a fresh profile, that 
will result in such entries appearing in the cookie-manager dialog.  Thanks.

Comment 2

[Stephen P. Morse]

Ignore last comment.  I am able to reproduce.  It's a linux-only problem -- does 
not occur on winNT.

Comment 3

[Stephen P. Morse]

Problem is in the routine that the cookie manager calls to format the date/time 
string.  That routine is in nsScriptableDateTimeFormat.

The call is occuring in extensions/wallet/cookieviewer/cookieviewer.js at the 
following routin:

function GetExpiresString(expires) {
  if (expires) {
    var date = new Date(1000*expires);
    return gDateService.FormatDateTime("", gDateService.dateFormatLong,
                                       gDateService.timeFormatSeconds,
                                       date.getFullYear(),
                                       date.getMonth()+1, date.getDate(),
                                       date.getHours(),
                                       date.getMinutes(), date.getSeconds());
  }
  return cookieBundle.getString("AtEndOfSession");
}

If I put a dump statement in this routine, I see that the value returned from 
FormatDateTime is correct (has the time only once) on an NT platform but 
incorrect (time is doubled up) on linux.

Reassigning to intl group who is responsible for the FormatDateTime routine.

Comment 4

[Roy Yokoyama]

Not sure who should own this "FormatDateTime - Linux only" bug.
Try ftang.
Please reassign to appropriate developer if you are not a right owner.

Comment 5

[Stephen P. Morse]

Can we get a target milestone on this?

Comment 6

[Frank Tang]

give this to shanjian

Comment 7

[Frank Tang]

nsbeta1- . dup information but no lost information. 

Comment 8

[Stephen P. Morse]

Frank, please reconsider that nsbeta1- decision.  True there is no lost 
information.  But the format of the string, with the time twice, is an 
embarrasement in the product.  That's the sort of thing we want to clean up in 
such a major release as 1.0.

Comment 9

[Andreas Becker]

Re-nominating for nsbeta1.

Comment 10

[Shanjian Li]

Attachment: patch

Comment 11

[Shanjian Li]

Date time formatted string was generated in "nsDateTimeFormatUnix::FormatTime"
using library function strftime. We process the date and time separately in the
function, but format specifier "%c" was incorrectly used there. According to
documentation, "%c" will generate BOTH the preferred date and time string. "%x"
will only generate preferred date string, and time will be controled by later
part of the code. 

I search all the instance of strftime, and it seems that changes in this place
is complete. 

asking for r/sr

Comment 12

[Shanjian Li]

naoki, sspitzer, could you r/sr

Comment 13

[nhottanscp]

Does this mean no difference between kDateFormatLong and kDateFormatShort?
Then you can do like this.

 case kDateFormatLong:
 case kDateFormatShort:
       PL_strncpy(fmtD, "%x", NSDATETIME_FORMAT_BUFFER_LEN);

Comment 14

[Frank Tang]

nsbeta1+

Comment 15

[Shanjian Li]

Attachment: new patch

Comment 16

[Shanjian Li]

sorry, I forgot to update my patch. 
As for why kDateFormatLong and kDateFormatShort resolve the same format, this
should not be the concern of this bug. We just want to eliminate the extra time
field and that's what I did here. Any additional change of the format will
surely change the existing behavior and thus bring certain risk. In my
understanding of the code, what we really want when asking for kDateFormatLong
is the prefered date, and "%x" return the preferred date format. 

Comment 17

[nhottanscp]

Comment on attachment 72429 [details] [diff] [review]
new patch

r=nhotta

Please make sure the other place which uses the long format works fine with the
patch.
/xpfe/browser/resources/content/pageInfo.js, line 750

Comment 18

[Shanjian Li]

That's the only other place where FormatTime is called, and it is also asking
for kDateFormatLong.
There should be no problem there. (Since FormatTime is just a implementation for
unix platform, and we have other platforms' implementations that works well, it
is safe to assume that the callers are alright. So as long as the implementation
for unix stay with API specification, I would not bother checking every calling
instance of FormatTime. As you raise the questions, the answer is yes. )

Comment 19

[(not reading, please use [e-mail] instead)]

sr=sspitzer

1) it doesn't look like kDateFormatLong is used all that much.  if you switch 
to define both as the same thing, then wants the point to having them?

should we just replace the two usages (excluding the test.cpp file) of 
DateFormatLong with DateFormatShort and remove support for it?

/extensions/wallet/cookieviewer/CookieViewer.js
/xpfe/browser/resources/content/pageInfo.js

or....

2) is there another string, besides "%x" that would be more correct for long 
format?  what's the difference between short and long on mac or windows?
(or have you checked, and this is a windows thing, so the best we can do for 
long format on unix is to re-use short format.)

3)  do any other platforms have this problem?  It looks like OS/2 might, cc 
mkaply.  if all platforms get dateformatshort right, but not dateformatlong, 
this is another reason to remove dateformatlong.

4)  note about cleanup that we can spin out to another bug:

http://lxr.mozilla.org/mozilla/source/intl/locale/idl/nsIScriptableDateFormat.id
l#45

we don't need those two enums.  we could remove them and the places in C++ 
where we do kDateFormatLong, we could just do 
nsIScriptableDateFormat::dateFormatLong, etc.

Comment 20

[Mike Kaply [:mkaply]]

Based on a cursory glance, it would appear that the difference between LONG and 
SHORT date format on Windows is whether or not the month is spelled out (Jan vs. 
January)

So, most platforms implemented this incorrectly.

This is yet another case where the spec (the names of the enums) were designed 
to match Windows APIs, and the implementors on other platforms had to guess what 
the correct results were.

Maybe nsIScriptableDateFormat.idl should be updated to have examples of the time 
. Here's what it says about long vs. short:

kDateFormatLong,                // provides the long date format for the given 
locale
kDateFormatShort,               // provides the short date format for the given 
locale

No useful info at all.

Comment 21

[Shanjian Li]

answer to seth's questions:
1)kDateFormatLong and kDateFormatShort makes difference for windows at least. So 
we need to keep them.
2)Not as I know. On unix, "%x" in fact mean preferred. We cann't find a more 
appropriate item. In fact, the final result is what we really want on unix 
platform.
3)There is no right or wrong as long as the output string can be interpreted by 
a human. 
4)For above reasons, I am not planning to do any cleanup.

to mkaply,
IMHO, it should not be our goal to achieve exact same output. The output is for 
a human reader and we should just do what we are allowed to do in certain 
platform. A fuzzy descript (long and short) works well in this sense. 

As a conclusion, I think our current solution is good enough. Unless a user 
has a strong reason to complain in certain platform, we should stick with our 
current approach. 

Comment 22

[Mike Kaply [:mkaply]]

I agree that in looking at the spec for strftime, we cannot do anything 
equivalent on unix or OS/2.

We need an additional diff to fix this on OS/2 (same fix as Unix).

Note that Mac is broken in this regard as well. See:

http://developer.apple.com/techpubs/mac/Text/Text-339.html

It should be using abbrevDate and longDate not shortDate and abbrevDate (again, 
to be consistent with Windows)

And I still think we need to update the header to be more clear.

Comment 23

[Shanjian Li]

mkaply, 
Thanks for checking out mac. I will file a bug on mac, and please file a bug for 
OS2. I will put more explaination in header. How about that?

Comment 24

[Mike Kaply [:mkaply]]

Attachment: OS/2 fix

Since the OS/2 fix is EXACTLY like the Unix fix, can we just fix it here?

Comment 25

[Shanjian Li]

Attachment: additional patch to add more explaination in comment

Comment 26

[Shanjian Li]

Bug 129146 is filed for Mac. Now we are ready to take care of this bug. 

Seth, could you sr mkaply's patch for OS/2 and my comment modifications?

Comment 27

[(not reading, please use [e-mail] instead)]

sr=sspitzer for the three patches (unix, os/2 and additional comment patch).

I'm going to spin up a new bug for shanjian about the code cleanup that we can
make to nsIScriptableDateFormat.idl

Comment 28

[(not reading, please use [e-mail] instead)]

code clean up bug is http://bugzilla.mozilla.org/show_bug.cgi?id=129514

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 72429 [details] [diff] [review]
new patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 72639 [details] [diff] [review]
OS/2 fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 72674 [details] [diff] [review]
additional patch to add more explaination in comment

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 32

[Mike Kaply [:mkaply]]

OK, I'm confused.

In my comments on short vs long on Windows, I said it was the length of the 
month.

But if you believe short vs long is 2/4/2002 vs Feb 4, 2002, you CAN do this on 
Unix.

%D does this.

Comment 33

[Shanjian Li]

mkaply, 
The comment I made for windows was based on my observation of my windows 
behavior (NT4.0). As long as I do not hear complain about short date format on 
unix platforms, I really don't want to change it, even though that means 
inconsistency behavior from windows. There are so many places where short format 
is used. 

Comment 34

[(not reading, please use [e-mail] instead)]

from mkaply's comments, is the right fix to replace "%c" with "%D"?

Comment 35

[Shanjian Li]

No, what we care about here is kDateFormatLong, and mkaply is talking about 
kDateFormatShort. 

Comment 36

[Mike Kaply [:mkaply]]

I think the issue is that long should be:

%x

and short should be

%D

If you make long the same as short, and someone comes along and fixes short to 
properly use %D, then long will be broke again.

Comment 37

[Shanjian Li]

fix checked in. 

Comment 38

[(not reading, please use [e-mail] instead)]

what got checked in?  I didn't realize we were done discussing %c vs %x vs %D

from http://bugzilla.mozilla.org/show_bug.cgi?id=119013#c36

Comment 39

[Shanjian Li]

I change the long format from %c to %x for kDateFormatLong. Does anybody
disagree with that? I don't think so. Because kDateFormatShort is also using %x,
they happen to be the same. If somebody think %D is better for kDateFormatShort,
they can change that without affecting kDateFormatLong. That is a separated
issue. IMO, we should not change the currect behavior unless somebody is
complaining about it. 

Comment 40

[Yuying Long]

Verified on 03-11 trunk build, the time no more appears twice, only one short
format exists.

Mark as verified.  Please re-open or file a seperate bug in case want to make
some other changes.