Cookie expire times reported twice / bug in FormatDateTime routine

VERIFIED FIXED in mozilla1.0

Status

()

VERIFIED FIXED
17 years ago
15 years ago

People

(Reporter: caillon, Assigned: shanjian)

Tracking

({intl})

Trunk
mozilla1.0
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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

17 years ago
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

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

Comment 3

17 years ago
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.
Assignee: morse → yokoyama
Component: Cookies → Internationalization
QA Contact: tever → ruixu
Summary: Cookie expire times reported twice → Cookie expire times reported twice / bug in FormatDateTime routine

Comment 4

17 years ago
Not sure who should own this "FormatDateTime - Linux only" bug.
Try ftang.
Please reassign to appropriate developer if you are not a right owner.
Assignee: yokoyama → ftang

Updated

17 years ago
Keywords: intl
QA Contact: ruixu → ylong

Comment 5

17 years ago
Can we get a target milestone on this?

Updated

17 years ago
Keywords: nsbeta1

Comment 6

17 years ago
give this to shanjian
Assignee: ftang → shanjian

Comment 7

17 years ago
nsbeta1- . dup information but no lost information. 
Keywords: nsbeta1 → nsbeta1-

Comment 8

17 years ago
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

17 years ago
Re-nominating for nsbeta1.
Keywords: nsbeta1- → nsbeta1
(Assignee)

Comment 10

17 years ago
Created attachment 72093 [details] [diff] [review]
patch
(Assignee)

Comment 11

17 years ago
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
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 12

17 years ago
naoki, sspitzer, could you r/sr

Comment 13

17 years ago
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

17 years ago
nsbeta1+
Keywords: nsbeta1 → nsbeta1+
(Assignee)

Comment 15

17 years ago
Created attachment 72429 [details] [diff] [review]
new patch
Attachment #72093 - Attachment is obsolete: true
(Assignee)

Comment 16

17 years ago
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

17 years ago
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
Attachment #72429 - Flags: review+
(Assignee)

Comment 18

17 years ago
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. )

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

17 years ago
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.
(Assignee)

Comment 21

17 years ago
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

17 years ago
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.
(Assignee)

Comment 23

17 years ago
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

17 years ago
Created attachment 72639 [details] [diff] [review]
OS/2 fix

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

Comment 25

17 years ago
Created attachment 72674 [details] [diff] [review]
additional patch to add more explaination in comment
(Assignee)

Comment 26

17 years ago
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?
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
(Assignee)

Updated

17 years ago
Attachment #72639 - Flags: superreview+
Attachment #72639 - Flags: review+
(Assignee)

Updated

17 years ago
Attachment #72429 - Flags: superreview+
(Assignee)

Updated

17 years ago
Attachment #72674 - Flags: superreview+
Attachment #72674 - Flags: review+

Comment 29

17 years ago
Comment on attachment 72429 [details] [diff] [review]
new patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72429 - Flags: approval+

Comment 30

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72639 - Flags: approval+

Comment 31

17 years ago
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
Attachment #72674 - Flags: approval+

Comment 32

17 years ago
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.
(Assignee)

Comment 33

17 years ago
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. 
from mkaply's comments, is the right fix to replace "%c" with "%D"?
(Assignee)

Comment 35

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

Comment 36

17 years ago
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.

(Assignee)

Comment 37

17 years ago
fix checked in. 
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
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
(Assignee)

Comment 39

17 years ago
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

17 years ago
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.
Status: RESOLVED → VERIFIED

Updated

15 years ago
Blocks: 231757
You need to log in before you can comment on or make changes to this bug.