Closed Bug 350 Opened 22 years ago Closed 11 years ago

PR_ExplodeTime() works only if given a PRTime argument between year 1901-2099

Categories

(NSPR :: NSPR, defect, P3, minor)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: nelson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 8 obsolete files)

Created by Wan-Teh Chang (wtc@netscape.com) on Wednesday, May 13, 1998 6:20:07 PM PDT
Additional Details :
This problem is first reported by Christopher Shaulis
<cjs2895@la-paz.org> in the netscape.public.mozilla.general
newsgroup.  He noticed a comment in
mozilla/nsprpub/pr/src/misc/prtime.c saying that
the leap-year calculation used in ComputeGMT()
is only correct for 1901-2099.  ComputeGMT() is
an internal function used by PR_ExplodeTIme().

This means that PR_ExplodeTime returns an incorrect
result if given a PRTime value that represents a
date before 1900 or after 2100.
Updated by Wan-Teh Chang (wtc@netscape.com) on Wednesday, May 13, 1998 6:21:33 PM PDT
Additional Details :
Marked the bug assigned with priority P5.
per leger, assigning QA contacts to all open bugs without QA contacts according
to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser
NSPR now has its own Bugzilla product.  Moving this bug to the NSPR product.
phillip gone, removing him from qa contact, sorry for spam
Target Milestone: --- → Future
remove self
dfsgdfsgdg
Attached file test (obsolete) —
Attachment #113577 - Attachment is obsolete: true
No longer blocks: 172908
test
Blocks: 238632
&#28204;&#35430;&#20013;&#25991;&#39023;&#31034;
Shanmu, since you are working on the related bug 164070,
would you like to take a stab at this bug, too?
Assignee: wtchang → shanmus
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
test
This is the oldest surviving bug on the database.

Have a nice Thanksgiving!
I'll take this, I have a patch.
Assignee: shanmus → samuel
Status: ASSIGNED → NEW
Attachment #204200 - Flags: review?(wtchang)
Wan-Teh, any chance you could review the patch in the nearby future?
QA Contact: nspr
Is there anyone else that can review this patch?
Attachment #204200 - Flags: review?(wtc) → review?(nelson)
Comment on attachment 204200 [details] [diff] [review]
can handle any year from 1 to whatever

r=wtc.

Thank you for the patch, Samuel, and I'm terribly sorry
that this fell through the cracks for almost two years.

Nelson, you're welcome to review this patch, but you can
also cancel the review request.

I have some suggested changes to improve the patch.

>+    numDays += 719162;       /* days up to 1970 */

The comment should note the beginning of the range of
years.  I was wondering whether it was from 1900 or 0.
It turns out to be from 1 -- 719162 is the number of
days in years 1-1969.  If we ignore the fact that year
0 didn't exist for now, the choice of 1 as the beginning
of range is important, because it makes the exceptions
for 4-year, 100-year, and 400-year periods happen at the
ends.  This allows us to use / and % easily.

>+    gmt->tm_year = tmp * 400;

This can be written as

      gmt->tm_year = tmp * 400 + 1;

to eliminate the off-by-one issue.  1 is the beginning
of your range (see above), so it makes sense to see 1
there.  (Cf. tmp = (tmp * 4) + 1970; in the original code.)

Note: we could choose any year 400*k + 1 as the beginning
of our range.  I guess this must be one of the reasons why
Windows' time epoch is January 1, 1601.

>+    if (rem >= 365) {
>         tmp++;
>         rem -= 365;
>-        if (rem >= 365) {                        /* 1972, etc. */
>+        if (rem >= 365) {
>             tmp++;
>             rem -= 365;
>-            if (rem >= 366) {                        /* 1973, etc. */
>+            if (rem >= 365) {
>                 tmp++;
>-                rem -= 366;
>-            } else {
>-                isLeap = 1;
>+                rem -= 365;
>             }
>         }
>     }

Since the exception (leap year) now happens at the end
of the 4-year period, we can use / and % and handle the
exception like this (assuming we take care of the off-by-one
issue as I suggested above):

    tmp = rem % 365;
    rem %= 365;
    if (tmp == 4) {
        tmp = 3;
        rem = 365;
    }

I also wanted to note that the code you removed:

-    if (rem < 0) {
-        tmp--;
-        rem += (4 * 365 + 1);
-    }

was intended to handle a negative numDays.  In your patch,
numDays is only negative if the year was B.C.  But that's
already outside the range of years in which our formula
is valid.
Attachment #204200 - Flags: review+
Samuel, please let me know if you agree with the
changes I made.  Could you also test this version
of your patch?  Thanks.
Attachment #204200 - Attachment is obsolete: true
Attachment #279492 - Flags: superreview?(nelson)
Attachment #279492 - Flags: review?(samuel)
Attachment #204200 - Flags: review?(nelson)
Comment on attachment 279492 [details] [diff] [review]
can handle any year from 1 to whatever, v2

r=nelson.  This looks correct, although I haven't tested it.
I'd like to see a few more-or-less cosmetic changes.

1) The variable numDays changes its definition in the middle of 
the function.  Initially it is the number of days since some relatively 
recent date (Jan 1, 1970?), but then it changes in the middle of the
function when  a large constant is added to it. I'd like to see comments
stating the definition of day 0 at the declaration, and again at the 
point where it is changed.  e.g. something like:
"Day 0 is Jan 1, year 1" or whatever is correct.
This will help future reviewers.  (too late for me, this time :)

2) I'd like to see the expression "4 * 365 + 1" disappear from this code.
Either replace it with a decimal constant and a comment explaining what
that number is (as is done in numerous other places in the code), or 
declare a static const variable and initialize it with this expression,
and then use that variable name in place of this expression.
Attachment #279492 - Flags: superreview?(nelson) → superreview+
wtc, is this still on the radar?
Samuel, can you review the patch?
Blocks: 391038
happy tenth anniversary!!
It appears that Samuel has been unable to review this patch (yet).

It appears that nobody had time (yet) to work on the cosmetic changes that Nelson has proposed.

Nelson, Wan-Teh, as this is a patch with reviews, could it get checked in, despite the lack of the cosmetic change?
OK, I've updated the patch to take Nelson's comments into account. Regarding comment #1, I added a comment mentioning the need to convert numDays from being based on 1970 to being based on year 1. Regarding comment #2, I decided to just add an integer with a comment. I'm not a huge fan of magic numbers, but I don't want to mess with the prevailing style of the function.

I also added an unrelated whitespace fix that happened to catch my eye.

wtc, kaie, nelson, who should review this so we can get this in and close it out? Also, are tests needed for this somewhere? I don't know what protocol is for NSPR.
Attachment #279492 - Attachment is obsolete: true
Attachment #361089 - Flags: superreview+
Attachment #361089 - Flags: review?
Attachment #279492 - Flags: review?(samuel)
So, when trying to run this patch on the tryserver, I found out that both v2 and v3 were missing a semicolon, making the compilers rather angry. v3a fixes the missing semicolon.

Tryserver builds are available here:
https://build.mozilla.org/tryserver-builds/2009-02-07_14:28-jdolske@mozilla.com-try-c0552e9f41c/

Could someone try these builds out to verify that the fix works as expected?
Attachment #361089 - Attachment is obsolete: true
Attachment #361098 - Flags: superreview+
Attachment #361098 - Flags: review?
Attachment #361089 - Flags: review?
To facilitate review, I converted this patch to a CVS diff.
This is because
a) the "master" upstream NSPR repository is CVS, and 
b) bonsai's interdiff patch reader only works with CVS diffs (bug 386251).
Patch v3 appears to have resurrected some code that was correctly deleted 
in v2.  Thus, my sr+ for v2 does not apply to v3.  That's one reason why
we don't allow the practice of "carrying forward" r+/sr+ from old patches 
to new ones for NSPR and NSS.  

I think this patch fixes it again.  Will know soon.
Attachment #361098 - Attachment is obsolete: true
Attachment #361184 - Attachment is obsolete: true
Attachment #361098 - Flags: review?
Comment on attachment 361089 [details] [diff] [review]
v3: can handle any year from 1 to whatever

removing "carried forward" sr+.
Attachment #361089 - Flags: superreview+ → superreview-
Comment on attachment 361098 [details] [diff] [review]
v3a: can handle any year from 1 to whatever

removing "carried forward" sr+.
Attachment #361098 - Flags: superreview+ → superreview-
Whoops, sorry about re-adding the code. v4 is missing a comment regarding 1461 days being a 4 year span, though, since I'd added it to the block being deleted. It should probably be added further down where 1461 first appears.
Attachment #361186 - Flags: review?(julien.pierre.boogz)
Comment on attachment 361186 [details] [diff] [review]
patch v4 - remove resurrected code

Since so many of us have now contributed to this patch, I'm going to ask
someone who has not previously participated in this bug to review the
latest diffs.  

Julien, please review the diffs between v2 and v4 of this patch, using
https://bugzilla.mozilla.org/attachment.cgi?oldid=279492&action=interdiff&newid=361186
(You are, of course, free to review the whole patch, if you wish.)
Attached patch patch v5 - readd deleted comment (obsolete) — Splinter Review
Thanks for catching the lost comment.  I readded it.
Julien, please review this patch, comparing it to patch v2.  
I will commit this if/when it gets r+ giving credit to all contributors.
Assignee: samuel → nelson
Attachment #361186 - Attachment is obsolete: true
Attachment #361188 - Flags: review?(julien.pierre.boogz)
Attachment #361186 - Flags: review?(julien.pierre.boogz)
Priority: P5 → P3
Target Milestone: Future → 4.8
I checked in this patch on the NSPR trunk (NSPR 4.8).

Checking in prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.36; previous revision: 3.35
done

The comment "We first do the usec, sec, min, hour thing
so that we do not have to do LL arithmetic" means that
it is OK for numDays to be a PRInt32 and to be manipulated
with 32-bit arithmetic.  Since the new code still has that
property, I kept this comment.
Attachment #361188 - Attachment is obsolete: true
Attachment #361188 - Flags: review?(julien.pierre.boogz)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 361319 [details] [diff] [review]
patch v6 (checked in)

I just pushed a new NSPR snapshot (CVS tag NSPR_HEAD_20090209)
with this patch to mozilla-central in changeset b21b71e9b0b1.
http://hg.mozilla.org/mozilla-central/rev/b21b71e9b0b1
Dependent bug 238632 seems to be fixed in Thunderbird 3.1a1pre (20090407) after this fix. Is it planned to make the fix available in mozilla-1.9.1 too for the upcoming Thunderbird 3.0 release?
This is a very minor bug, so I don't plan to make the fix
available in mozilla-1.9.1.
(In reply to comment #34)
Stefan, this fix is now in mozilla-1.9.1.
You need to log in before you can comment on or make changes to this bug.