calDateTime.jsDate fails after 2037 and before 1970

VERIFIED FIXED

Status

Calendar
Internal Components
VERIFIED FIXED
13 years ago
10 years ago

People

(Reporter: Michiel van Leeuwen (email: mvl+moz@), Assigned: Michiel van Leeuwen (email: mvl+moz@))

Tracking

Details

Attachments

(2 attachments, 2 obsolete attachments)

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");
(Assignee)

Comment 1

13 years ago
Created attachment 171153 [details] [diff] [review]
patch v1

should use the exploded date, instead of msec
(Assignee)

Updated

13 years ago
Attachment #171153 - Flags: first-review?(shaver)
Comment on attachment 171153 [details] [diff] [review]
patch v1

I think that's right; vlad?
Attachment #171153 - Flags: second-review?(vladimir)
Attachment #171153 - Flags: first-review?(shaver)
Attachment #171153 - Flags: first-review+
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

13 years ago
You're hitting Year 2038 problem. See http://www.deepsky.com/~merovech/2038.html
(Assignee)

Comment 5

13 years ago
also see bug 147926 comment 9
(Assignee)

Comment 6

13 years ago
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?
Attachment #171153 - Flags: second-review?(vladimir)

Comment 7

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

Comment 8

12 years ago
(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

12 years ago
*** Bug 315510 has been marked as a duplicate of this bug. ***
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.

Updated

12 years ago
Keywords: relnote
Summary: calDateTime.jsDate fails after 2037 → calDateTime.jsDate fails after 2037 and before 1970

Updated

12 years ago
Keywords: dataloss
*** Bug 319270 has been marked as a duplicate of this bug. ***

Comment 12

12 years ago
*** Bug 320179 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

12 years ago
Blocks: 324293
(Assignee)

Comment 13

12 years ago
Created attachment 210273 [details] [diff] [review]
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.
Attachment #171153 - Attachment is obsolete: true
Attachment #210273 - Flags: first-review?(dmose)

Comment 14

12 years ago
(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]
(Assignee)

Comment 15

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

Comment 16

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

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

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

Comment 19

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

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

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

Comment 22

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

Updated

11 years ago
Flags: blocking0.3+

Updated

11 years ago
Whiteboard: [patch in hand]
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...

Updated

11 years ago
Attachment #210273 - Flags: first-review?(dmose)
(Assignee)

Comment 24

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

Comment 25

11 years ago
*** Bug 350397 has been marked as a duplicate of this bug. ***
Whiteboard: [patch in hand] → [patch in hand][needs input from dmose]
Whiteboard: [patch in hand][needs input from dmose] → [patch in hand][needs input from dmose][no l10n impact]
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.
Whiteboard: [patch in hand][needs input from dmose][no l10n impact] → [patch in hand][needs input from shaver]
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. :)
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.

Updated

11 years ago
Attachment #210273 - Flags: second-review?(dmose)
Attachment #210273 - Flags: first-review?(michael.buettner)

Updated

11 years ago
Whiteboard: [patch in hand][needs input from shaver] → [patch in hand][needs review tbe dmose]

Updated

11 years ago
Whiteboard: [patch in hand][needs review tbe dmose] → [patch in hand][needs review mickey dmose]

Comment 29

11 years ago
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.
Attachment #210273 - Flags: first-review?(michael.buettner) → first-review+

Comment 30

11 years ago
(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

11 years ago
*** Bug 353050 has been marked as a duplicate of this bug. ***
Whiteboard: [patch in hand][needs review mickey dmose] → [patch in hand][needs review dmose]

Updated

11 years ago
Whiteboard: [patch in hand][needs review dmose] → [patch in hand][needs unbitrotted patch]
(Assignee)

Comment 32

11 years ago
Created attachment 239079 [details] [diff] [review]
un-rotted patch

Patch is now un-rotted. I also fixed .SubstractDate to not use time_t, but use the already calculated nativeTime directly.
Attachment #239079 - Flags: second-review?(dmose)
Attachment #239079 - Flags: first-review+
Whiteboard: [patch in hand][needs unbitrotted patch] → [unbitrotted patch in hand][needs review 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.
Attachment #239079 - Flags: second-review?(dmose) → second-review+
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.
Whiteboard: [unbitrotted patch in hand][needs review dmose] → [patch in hand][needs updated patch and checkin]
This has been fixed, though it caused regressions bug 353400 and bug 353401.  Additionally, there is still some sort of view problem: bug 353405.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [patch in hand][needs updated patch and checkin]
Attachment #210273 - Attachment is obsolete: true
Attachment #210273 - Flags: second-review?(dmose)

Comment 36

11 years ago
(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

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.
Created attachment 239291 [details] [diff] [review]
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.
Attachment #239291 - Flags: first-review+
Precedence fix landed on the trunk and branch.

Comment 40

11 years ago
(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

11 years ago
(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+
Status: RESOLVED → VERIFIED

Updated

11 years ago
Whiteboard: [litmus testcase wanted]

Comment 42

11 years ago
Litmus testcase 2600 created

Updated

11 years ago
Whiteboard: [litmus testcase wanted]

Comment 43

11 years ago
Litmus testcase 2601 created

Updated

10 years ago
Keywords: dataloss, relnote
You need to log in before you can comment on or make changes to this bug.