410647 - js_*Date* APIs need assertion and comment about Java 0-based month

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Moening]

Using js_NewDateObject to return a date object to JS from native C++ code results in an invalid date.

This native call:

*rval = OBJECT_TO_JSVAL(js_NewDateObject(cx, 2007, 12, 21, 13, 42,15);

Produces the following output on JS side when .toString is used on the returned object:
Mon Jan 21 2008 13:42:15 GMT-0600 (Central Standard Time)

The returned date is 1 month into the future,
Expected:
Fri Dec 21 2007 13:42:15 GMT-0600 (Central Standard Time)

When quickly looking into the problem it appears that the MakeDay() function may be to blame.  I don't really understand what this function is trying to do but the output seems wrong.

The line:
year += floor(month / 12);  
is producing the year 2008??

Any help would be appreciated.

Comment 1

[Bob Clary [:bc] (inactive)]

The arguments are 0 based. 11 would be December while 12 moves it to the next month, January. See <http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Date>

invalid.

Comment 2

[Mike Moening]

In the documentation link (not in the code anywhere) it states that the month is zero based.  Are any of the other arguments zero based?

Is this left over from some old design decision from way back when?

People using JS code like this:  new Date(2007, 12, 21, 13, 42, 15)
will hit this too...

Comment 3

[Brendan Eich [:brendan]]

It's an ancient botch copied exactly from java.util.Date. We can't change the JS Date object except by adding to it, and there's not much gain given the installed base argument. Changing the C API to differ from how the JS API works here would be as confusing or morseso. We could have a new C API with a different name ("js_NewDateObjectUsingSaneAPI" :-P) but I think that's overkill. This is a pain point to be treated by docs.

/be

Comment 4

[Bob Clary [:bc] (inactive)]

(In reply to comment #2)

Just the month. Think of the date being calculated from the arguments by adding the number of years, number of months, number of days, etc to arrive at a specific date. This dates back to the ECMAScript standard and all browsers agree on the behavior.

Comment 5

[Mike Moening]

Brendan, I agree with your comments.  We are stuck with it.

The only thing that would be **nice** is to add a small comment in the code itself for unwary coders to see this.  I googled on the API and and docs never came up in the search results.
Sorry for wasting your time.
Thanks!

Comment 6

[Brendan Eich [:brendan]]

Agree on comment, or developer.mozilla.org doc, or both -- we're wrestling with how to keep API docs fresh in code (or near it) but also wikified and i18n'ized at MDC. Not sure where that discussion left off.

/be

Comment 7

[Brendan Eich [:brendan]]

We could also JS_ASSERT(mon < 12); in js_NewDateObject and catch the December case. I like that. Reopening for that and a comment -- thanks for provoking this thought, Mike!

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: fix

Bob, could you try a debug shell with this patch, I don't want to assertbotch the testsuite. DEBUG browser build would be helpful too. I'm a bit swamped but hope this change can just land quietly, yet unwilling to roll the dice. Thanks,

/be

Comment 9

[Mike Moening]

>>We could also JS_ASSERT(mon < 12); in js_NewDateObject and catch the December
case.

Yes! Excellent! That would have tipped me off immediately and saved a lot of head scratching!

Comment 10

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)

k. eta this pm.

Comment 11

[Bob Clary [:bc] (inactive)]

ecma/Date/15.9.3.1-1.js should have asserted if any test was going to. It didn't. Is that right?

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> ecma/Date/15.9.3.1-1.js should have asserted if any test was going to. It
> didn't. Is that right?

Duh, we really are not testing the JS_FRIEND_API entry points by running JS tests. We need some embeddings that use the C "friend" API exported from jsdate.h since those API entry points (js_NewDateObject, js_DateSetMonth) are the only places where the assertions were added.

I'll commit when the tree's open if you can stamp r+. Thanks,

/be

Comment 13

[Bob Clary [:bc] (inactive)]

Comment on attachment 295242 [details] [diff] [review]
fix

bc wields the rubber stamp, r+

Comment 14

[Brendan Eich [:brendan]]

Fixed:

js/src/jsdate.c 3.96
js/src/jsdate.h 3.15

/be

Comment 15

[Jason Orendorff [:jorendorff]]

(In reply to comment #6)
> Agree on comment, or developer.mozilla.org doc, or both -- we're wrestling with
> how to keep API docs fresh in code (or near it) but also wikified and i18n'ized
> at MDC. Not sure where that discussion left off.

For reference, bug 409205 is relevant to this.