278236 - calDateTime.jsDate fails after 2037 and before 1970

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

when the year > 2037, calDateTime.jsDate fails, and return 1970

testcase:

var d = Components.classes["@mozilla.org/calendar/datetime;1"]
                  .createInstance(Components.interfaces.calIDateTime);
d.year=2040;
dump(d.jsDate+"\n");

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v1

should use the exploded date, instead of msec

Comment 2

[Mike Shaver (:shaver emeritus)]

Comment on attachment 171153 [details] [diff] [review]
patch v1

I think that's right; vlad?

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The msec is always time in UTC; mYear/etc. might not be, depending on the
setting of mIsUtc or not.  The jsdate always assumes local timezone, so using
the exploded version could be wrong.  Not sure I understand why using msec
fails?  Does it roll over? (Shouldn't it be a PRInt64?)

Comment 4

[Jungshik Shin]

You're hitting Year 2038 problem. See http://www.deepsky.com/~merovech/2038.html

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

also see bug 147926 comment 9

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

I now think the problem isn't in getting the jsDate, but in setting mNativeTime.
Some places (like SetTimeInTimezone and FromIcalTime) use time_t, which isn't
safe. That should be avoided.
Does nspr have safe functions create a prtime from an exploded time?

Comment 7

[Joey Minta]

*** Bug 304164 has been marked as a duplicate of this bug. ***

Comment 8

[Simon Paquet [:sipaq]]

(In reply to comment #6 by mvl)
> I now think the problem isn't in getting the jsDate, but in setting 
> mNativeTime. Some places (like SetTimeInTimezone and FromIcalTime) use 
> time_t, which isn't safe. That should be avoided.
> Does nspr have safe functions create a prtime from an exploded time?

Are you looking for PR_ImplodeTime
http://www.mozilla.org/projects/nspr/reference/html/prtime.html#21333

PR_ImplodeTime
==============
Converts a clock/calendar time to an absolute time.

Syntax
======
#include <prtime.h>
PRTime PR_ImplodeTime(const PRExplodedTime *exploded);

Parameters
==========
The function has this parameter:
exploded - A pointer to the clock/calendar time to be converted.

Returns
=======
An absolute time value.

Description
===========
This function converts the specified clock/calendar time to an absolute time and
returns the converted time value.

Comment 9

[Joey Minta]

*** Bug 315510 has been marked as a duplicate of this bug. ***

Comment 10

[Simon Paquet [:sipaq]]

Here are the places in the code (without libical), where we use time_t:
http://lxr.mozilla.org/mozilla/source/calendar/base/src/calDateTime.cpp#315
http://lxr.mozilla.org/mozilla/source/calendar/base/src/calDateTime.cpp#545

mvl, see also comment 8 for an answer to your question in comment 6.

Comment 11

[Simon Paquet [:sipaq]]

*** Bug 319270 has been marked as a duplicate of this bug. ***

Comment 12

[Joey Minta]

*** Bug 320179 has been marked as a duplicate of this bug. ***

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2

This patch removes the usage of time_t, and instead uses PRTime. I had to port some libical functions to make this work.

Comment 14

[Stefan Sitter [:ssitter]]

(In reply to comment #13)
Good news: I did a build on Windows and it doesn't crash anymore. I can enter dates before 1970 and after 2038. Nice work mvl!

Bad news: It seem that some of the other components can't handle this right.

For example if i try to navigate the views to 2040 or 1968 I get errors like:
  Error: gridrows is not defined
  Source File: chrome://calendar/content/calendar-month-view.xml Line: 915

If I try to select an event (30-Nov-1994 to 07-Feb-2006) I get
  Error: uncaught exception: [Exception... "Component returned failure code: 
  0x80004005 (NS_ERROR_FAILURE) [calIDateTime.hour]"  nsresult: "0x80004005 
  (NS_ERROR_FAILURE)"  location: "JS frame :: 
  chrome://calendar/content/calendar-multiday-view.xml :: 
  getStartEndMinutesForOccurrence :: line 537"  data: no]

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

Let's track those problems in different bugs, like bug 325459

Comment 16

[kosherjava]

I do not know if this is the same issue or a new bug, but iCalendar import of events such as birthdays with dates before 1970 do not get imported, and no error is displayed to the user.
For example the following iCalendar event will not import:

BEGIN:VEVENT
DTSTAMP:20060316T102449Z
DTSTART;VALUE=DATE:19690101
DTEND;VALUE=DATE:19690101
SUMMARY:John Doe's Birthday
DESCRIPTION:Born on 1 January 1969 in Toronto
 \, Ontario\, Canada\nhttp://www.someurl.com/phpgedview/individual.php?pid
 =I1&ged=gedcom.ged
RRULE:FREQ=YEARLY
CLASS:CONFIDENTIAL
TRANSP:TRANSPARENT
UID:PhpGedView-12f99be2-ede2-4b3a-9bae-e1d4d0d783cd
CATEGORIES:PhpGedView Events
URL:http://www.someurl.com/phpgedview/individual.php?pid=I1&ged=gedcom.ged
END:VEVENT

Comment 17

[Chris Charabaruk]

That's never happened to me. Whenever I try to import events prior to 1970, they crash Sunbird after which I need to play with sqlite to remove some created records from the database which cause crash-on-launch after the import.

Comment 18

[kosherjava]

I started with a clean database and the latest nightly release, and delete the db after each test (I was only trying to test the format used by the PhpGedView.net iCalendar export). It is possible that the other entries in your DB cause problems with the pre 1970 dates.

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

Please move the discussion to some other place. The problem that this bug handles is clear and well defined, and has a patch. Let's not diverge into speculations about problems that maybe vaguely related.

Comment 20

[Stefan Sitter [:ssitter]]

Chris, kosherjava: No need to do further investigation.
The problem is known. It is also known that it leads to crash on Windows. This is also stated in the Sunbird 0.3a1 Release Notes. This bug has a patch waiting for review. Until that happens the problem exists. So please stop adding pointless comments.

Comment 21

[Joey Minta]

*** Bug 327311 has been marked as a duplicate of this bug. ***

Comment 22

[Andrew N Dowden]

This should be linked to Bug 147926 (not repeat-forever, past Jan-2038)

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 210273 [details] [diff] [review]
patch v2

mvl: is there any easy way to refactor this such that the "ported" code stays in libical and the PRTimezone references stay in base?  The reason I ask is because base is tri-licensed, and libical is dual-licensed...

Comment 24

[Michiel van Leeuwen (email: mvl+moz@)]

The only other way is see, i copying the PRExplodedTime definition into libical, and moveing the code into there. But I that means we really forked libical into a way that's not useful for anybody else. (All our changes we made so far are just general bugfixes, no real forks)

Are you sure we can't use lgpl/mpl code inside gpl/lgpl/mpl code? We could move libical out of other-licenses, so i think copying this code is ok too.
And besides, this code is so simple, (copying data out of one struct into a different struct), does it really fall under this sort of licensing issues? If it does, maybe somebody else should 'cleanroom' it.

Comment 25

[Michiel van Leeuwen (email: mvl+moz@)]

*** Bug 350397 has been marked as a duplicate of this bug. ***

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

At some level, not being a lawyer, I'm not really qualified to make a call here.  

That said, given that the libical guys specifically went out of their way to relicense this code so that _we_ could use it, and given that it would certainly seem to be within the spirit of the current licensing, and presumably even technically legal given the LGPL/GPL dropthrough, a reasonable argument could be made that we should just go ahead with your patch, and if anyone complains (which seems very unlikely), cross that bridge when we come to it.

As per our IRC discussion, I'd like to hear shaver's thoughts on the issue.

Comment 27

[Mike Shaver (:shaver emeritus)]

I think you're fine in terms of licensing; you could get Gerv to run it up the legal flagpole if you want an actual lawyer to say the same thing. :)

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

I'm comfortable going ahead as is; as I say, I think it's wildly unlikely that anyone is going to get bent out of shape about this.

Comment 29

[cmtalbert]

Comment on attachment 210273 [details] [diff] [review]
patch v2

This looks good. One note: you will have a hard time submitting this patch due to changes in calDateTime::FromIcalTime. Here's how to merge it by hand: Find the comment in that method stating "mNativeTime: not moving..." Comment out everything from there through the end of the method. Then insert the line: mNativeTime = IcaltimeToPRTime(icalt, icaltimezone_get_utc_timezone());
at the top of your new commented block.  Note that if you now create events in the deep past (before 1970) then you will walk off the edge of our timezone definitions and those events will be converted to UTC. We will want to relnote that, and we might want to consider converting those times to floating time instead of UTC so that the user really doesn't see that anything has changed.

Comment 30

[Ian Pottinger]

(In reply to comment #29)
> in the deep past (before 1970) 

LOL LOL LOL!  Oh you youngin's!  You have such a sense of humour!!  LOL LOL LOL!

Comment 31

[Stefan Sitter [:ssitter]]

*** Bug 353050 has been marked as a duplicate of this bug. ***

Comment 32

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: un-rotted patch

Patch is now un-rotted. I also fixed .SubstractDate to not use time_t, but use the already calculated nativeTime directly.

Comment 33

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 239079 [details] [diff] [review]
un-rotted patch

Nice work; this is definitely good cleanup in addition to fixing the bug.  Please be sure to add any appropriate libical developers to the license header.

>+    PRTime t2t;
>+    aDate->GetNativeTime(&t2t);
>+    // for a duration, need to convert the difference in microseconds (prtime)
>+    // to seconds (libical), so divide by one million.
>+    icaldurationtype idt = 
icaldurationtype_from_int((int)((mNativeTime-t2t)/1000000));

Using PR_USEC_PER_SEC in the above statement will ensure that the compiler uses the correct type for the constant as well as being stylistically nice.  Also, as long as you're here, how about switching to a C++-style cast?

>+PRTime
>+calDateTime::IcaltimeToPRTime(icaltimetype *icalt, struct _icaltimezone *tz)
>+{
>+    struct icaltimetype tt = *icalt;
>+    struct PRExplodedTime et;
>+
>+    /* If the time is the special null time, return 0. */
>+    if (icaltime_is_null_time(*icalt)) {
>+        return 0;
>+    }
>+
>+    if (tz != NULL) {
>+        tt = icaltime_convert_to_zone(*icalt, tz);
>+    }
>+

Stylistically, |if (tz)| would be a bit nicer.

>+    /* Empty the destination */
>+    memset (&et, 0, sizeof(struct PRExplodedTime));

No need for a space before the parens.

Both of the above comments apply to at least one other spot in the patch as well.

> 
>+    PRTime IcaltimeToPRTime(icaltimetype *icalt, struct _icaltimezone *tz);
>+    void PRTimeToIcaltime(PRTime time, PRBool isdate, struct _icaltimezone *tz, icaltimetype *icalt);

Any particular reason one of these returns its result, while the other uses an out param?

I suspect some or even most of the pointers being passed could be doubly |const|ed (e.g. |const icaltimetype *const icalt| for compiler optimization goodness.

You might want to add WithZone or something like that to those function names, so that it's more obvious to folks new to the code why these functions are necessary. 

Please file a separate bug to track the floating/UTC issue that ctalbert mentioned both w.r.t. relnoting it and possibly modifying the behavior in the future.

r2=dmose with the various nits addressed, as appropriate.

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

Also, it would be nice to have a comment explaining why, in IcaltimeToPRTime, it's the right thing to do the tz conversion using libical rather than having PR_ImplodeTime do it.

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

This has been fixed, though it caused regressions bug 353400 and bug 353401.  Additionally, there is still some sort of view problem: bug 353405.

Comment 36

[gekacheka]

(In reply to comment #32)
> I also fixed .SubstractDate to not use time_t, but use
> the already calculated nativeTime directly.
> 

How can it be sure nativeTime has already been calculated?  
See Bug 327935

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

While the suggestion in that bug 327935 sounds reasonable to me (on the surface, without having given it a lot of thought), the current state of the world is that users of calDateTime are expected to renormalize after doing something that might be denormalizing.  So if there are cases in the tree where that invariant is not maintained, I bet we've got bigger problems than just this patch, and they would have likely reared their heads long before now.

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: operator precedence fix

Somewhere in the final edits before the patch for this bug landed, a pair of parens that were needed to override default operator precedence got lost.  Patch was suggested by ctalbert, r1/r2=dmose.  The patch appears to fix all three regressions that ssitter found.

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

Precedence fix landed on the trunk and branch.

Comment 40

[Stefan Sitter [:ssitter]]

(In reply to comment #39)
> Precedence fix landed on the trunk and branch.

Using Sunbird/0.3a2+ (2006-09-19-21) on Windows 2000 and Linux creating new event on current day crashes application now.

Comment 41

[Damian Szczepanik]

(In reply to comment #40)
> (In reply to comment #39)
> > Precedence fix landed on the trunk and branch.
> 
> Using Sunbird/0.3a2+ (2006-09-19-21) on Windows 2000 and Linux creating new
> event on current day crashes application now.
I can create new events on WinXP, so for me looks good - please reopen if you can reproduce (checked after 2037 and before 1970)

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060923 Calendar/0.3a2+

Comment 42

[Damian Szczepanik]

Litmus testcase 2600 created

Comment 43

[Damian Szczepanik]

Litmus testcase 2601 created