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

remove self

dfsgdfsgdg

test

&#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?

test

This is the oldest surviving bug on the database. Have a nice Thanksgiving!

I'll take this, I have a patch.

Wan-Teh, any chance you could review the patch in the nearby future?

Is there anyone else that can review this patch?

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.

Samuel, please let me know if you agree with the changes I made. Could you also test this version of your patch? Thanks.

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.

wtc, is this still on the radar?

Samuel, can you review the patch?

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.

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?

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.

Comment on attachment 361089 [details] [diff] [review] v3: can handle any year from 1 to whatever removing "carried forward" sr+.

Comment on attachment 361098 [details] [diff] [review] v3a: can handle any year from 1 to whatever removing "carried forward" sr+.

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.

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.)

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.

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.

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.