`new Date(dateObj)` should create a copy of the input Date object instead of calling ToPrimitive and parsing the date-time string. Spec: ES2015, 20.3.2.2 Date, step 3.a Related: bug 868496, bug 810973

Bug 1187233 - Date constructor creates a copy when called with a Date object.

This is my first patch in the Javascript module, I have looked around a bit and it looks like this should work (I did some testing & ran the test suite). jorendorff@ please excuse me if this is totally incorrect :-). Thanks!

Comment on attachment 8638984 [details] MozReview Request: Bug 1187233 - Date constructor creates a copy when called with a Date object. r=jwalden This is on the right track! A few comments. IsDate sadly isn't the right test. It works if the Date constructor and provided Date object are from the same global object (window). Date.prototype.toString = Date.prototype.valueOf = null; var d = new Date(new Date(8675309)); // throws now, would work with your patch assertEq(d.getTime(), 8675309); Date.prototype.valueOf = () => 42; d = new Date(new Date(8675309)); assertEq(d.getTime(), 8675309); But it won't work if the Date constructor and Date object are from different globals. var D = newGlobal().Date; // requires js/src/test/browser.js's newGlobal, in the browser D.prototype.toString = D.prototype.valueOf = null; var d = new Date(new D(3141592654)); assertEq(d.getTime(), 3141592654); D.prototype.valueOf = () => 525600; d = new Date(new D(3141592654)); assertEq(d.getTime(), 3141592654); ECMAScript's JSON.stringify converts Boolean objects from any global to their true/false value. The ObjectClassIs Boolean case in js/src/json.cpp is also the right trick to handle this. I'd use DateObject::clippedTime() rather than TimeClip(dateObj->utcTime()). What tests did you run? I thought I added a test for the old behavior in js/src/tests or js/src/jit-test, but maybe I'm misremembering. If nothing broke, we need a test. Copy js/src/tests/ecma_5/Date/constructor-one-argment.js to js/src/tests/ecma_6/Date/constructor-one-Date-argument.js and put the two tests above in it. (The harness provides newGlobal when run in the browser, so no worries about providing it yourself.)

Thanks for your comments Jeff (and for the test cases)! > I'd use DateObject::clippedTime() rather than TimeClip(dateObj->utcTime()). I looked for clippedTime in DXR and couldn't find it. Looking at js/src/vm/DateObject.h it looks that there's no function that returns a ClippedTime, is there anything I'm missing here? > What tests did you run? I ran ./mach check-spidermonkey but now I'm thinking ./mach jsbrowsertest is the right one? The jstestbrowser passes now.

Comment on attachment 8638984 [details] MozReview Request: Bug 1187233 - Date constructor creates a copy when called with a Date object. r=jwalden Bug 1187233 - Date constructor creates a copy when called with a Date object.

(In reply to Giovanni Sferro [:agi] from comment #4) > I looked for clippedTime in DXR and couldn't find it. Looking at > js/src/vm/DateObject.h it looks that there's no function that returns a > ClippedTime, is there anything I'm missing here? [jwalden@find-waldo-now src]$ grep -r clippedTime ../../.hg/patches/ ../../.hg/patches/clip-more-places.diff:+ inline JS::ClippedTime clippedTime() const { ../../.hg/patches/clip-more-places.diff:+ clone = JS::NewDateObject(cx, selfHostedObject->as<DateObject>().clippedTime()); Uh...never mind. :-) I'll fix up afterward whenever I get to landing that patch. I really thought I'd been checking my tree with that patch *not* in place... > I ran ./mach check-spidermonkey but now I'm thinking ./mach jsbrowsertest is > the right one? The jstestbrowser passes now. I...have never heard of that before! According to word on the street, that does indeed run everything I would expect would matter here.

Comment on attachment 8638984 [details] MozReview Request: Bug 1187233 - Date constructor creates a copy when called with a Date object. r=jwalden https://reviewboard.mozilla.org/r/14157/#review12893 ::: js/src/jsdate.cpp:3023 (Diff revision 2) > + } SpiderMonkey style doesn't brace single-line ifs like this. See https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style for more. ::: js/src/jsdate.cpp:3026 (Diff revision 2) > + return NewDateObject(cx, args, t); Mild preference for NewDateObject(cx, args, TimeClip(unboxed.toNumber())), and moving the |ClippedTime t| down in this function to just before the isString() test. ::: js/src/tests/ecma_6/Date/constructor-one-argument.js:19 (Diff revision 2) > + "Date constructor passed a String object should parse it"); This bit from the original test should be removed. ::: js/src/tests/ecma_6/Date/constructor-one-argument.js:9 (Diff revision 2) > + "string-ness"; The bug number and summary here should be updated. The summary doesn't have to be *exactly* this bug's summary, just summarize in some sort of way that says what the bug was about -- say, "Passing a Date object to |new Date()| should copy it, not convert it to a primitive and create from that" or something.

Oh, also -- name the new test file constructor-one-Date-argument.js, not constructor-one-argument.js. More precise name about what's being tested there. Thanks!

https://reviewboard.mozilla.org/r/14157/#review12893 > SpiderMonkey style doesn't brace single-line ifs like this. See https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style for more. oh I'm sorry I just assumed it was the same as Gecko. I'll read that carefully :)

Comment on attachment 8638984 [details] MozReview Request: Bug 1187233 - Date constructor creates a copy when called with a Date object. r=jwalden Bug 1187233 - Date constructor creates a copy when called with a Date object.

Comment on attachment 8638984 [details] MozReview Request: Bug 1187233 - Date constructor creates a copy when called with a Date object. r=jwalden https://reviewboard.mozilla.org/r/14157/#review13111 Good stuff. Tack " r=jwalden" onto the end of that commit message and this is good to land. \o/

Comment on attachment 8638984 [details] MozReview Request: Bug 1187233 - Date constructor creates a copy when called with a Date object. r=jwalden Bug 1187233 - Date constructor creates a copy when called with a Date object. r=jwalden

Comment on attachment 8638984 [details] MozReview Request: Bug 1187233 - Date constructor creates a copy when called with a Date object. r=jwalden Thanks Jeff! :-)

Please post a link to passing Try run when requesting checkin.

Sorry Ryan! Will do. https://treeherder.mozilla.org/#/jobs?repo=try&revision=674b07ff06d0

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12416786&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c890f043b794

This caused B2G xpcshell failures as well. https://treeherder.mozilla.org/logviewer.html#?job_id=12424363&repo=mozilla-inbound

Looks like the failing Gaia test is: https://github.com/mozilla-b2g/gaia/blob/master/apps/calendar/test/unit/views/month_day_agenda_test.js and specifically this line: assert.deepEqual(new Date(currentDate.dataset.date), new Date(now)); where we have |var now = new Date();| and |currentDate.dataset.date| looks like a |data-date="..."| attribute on an element with id 'event-list-date' in the Gaia frontend code. Or I guess something set by JS, as well -- namely, here: https://github.com/mozilla-b2g/gaia/blob/92ce25e46faf3d3c66aa884759ff84ede039f71d/apps/calendar/js/views/month_day_agenda.js#L80 The incoming date being set there looks to be from the createDay() method, defined here: https://github.com/mozilla-b2g/gaia/blob/master/apps/calendar/js/common/calc.js#L322 I think what's happening is that Gaia computes the date |now|, with millisecond precision. Then it compares |new Date(now)| -- which before was |new Date(now.toString())| but now is effectively |new Date(now.valueOf())|, preserving millisecond precision -- with |new Date(now.toString())|, a date that strips off the milliseconds. As to what to do to fix this. I'm inclined to say we should assign |this.currentDate.dataset.date = date.getTime();| and then in the test do |assert.deepEqual(new Date(+currentDate.dataset.date), new Date(now.getTime()));|. Then once this patch has landed we can replace that latter with just |new Date(now)| again. But it's kind of fragile requiring that people interpreting .dataset.date *must* know it's a timestamp, *and* must manually convert it to number themselves. So I'm not confident. Probably worth poking Gaia people -- ask for help in #gaia -- to see what to do and how to do it.

Thanks for looking into that Jeff! Given that month_day_agenda_test.js is comparing a date built from a string and a date build from another date object, wouldn't it be better to do assert.deepEqual(new Date(currentDate.dataset.date), Date(now.toString())) so that we are comparing two Date objects built in the same way? This way we don't need to change application code. Anyway I'll ask in #gaia what they think about it tomorrow (PST). Thanks.

What's the status of this? :)

I think that my problem is related to this bug: ----------------------------------------------- http://es5.github.io/#x15.9.1.1 Minimum/maximum time in microseconds is: -8640000000000000/8640000000000000 It works fine when I try to create objects: var x = new Date(8640000000000000); // +275760-09-13T00:00:00.000Z var y = new Date(8640000000000000); // -271821-04-20T00:00:00.000Z but when some library tries to construct Date from my values: var x_2 = new Date(x); // +275760-09-13T00:00:00.000Z var y_2 = new Date(y); // Invalid Date It fails at 'y_2' (Invalid Date). Even if I remove 2 zeros it still fails. I had to remove 3 zeros to fix it (some lower values works too, so it's not seconds-milisecond mistake). Code to reproduce (in Chrome works fine): var x = new Date(8640000000000000); var y = new Date(-8640000000000000); var x_2 = new Date(x); var y_2 = new Date(y); console.log(x, y, x_2, y_2)

(In reply to payments.otsme from comment #24) > I think that my problem is related to this bug: Yup, note that you get the same "Invalid Date" in Chrome if you do this: var y_2 = new Date(y.toString()); Looks like people actually run into this (see also bug 810973) and a needinfo for a month with no response => I'll take this.

Waldo, you reviewed an earlier version of this patch. I just had to change ObjectClassIs to the now-fallible GetBuiltinClass.

Comment on attachment 8672611 [details] [diff] [review] Rebased patch Review of attachment 8672611 [details] [diff] [review]: ----------------------------------------------------------------- Relying on toString() behavior as you do in the b2g bug is IMO pretty unideal -- lots of extra string-computation/creation work that's sort of wasteful, dependence on implementation-defined behavior of .toString(), etc. But to get this landed, temporarily, I guess it's okay. I would like to see a followup bug that changes this to use .getTime(), fixed fairly promptly.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #27) > I would like to see > a followup bug that changes this to use .getTime(), fixed fairly promptly. In the non-test Gaia code, you mean? I've no idea where and how those values are used. I don't mind fixing things and I can do some more digging, but this is just a pre-existing issue with the Gaia code; I'm not sure we have to fix all their Date code and tests...

Sure. We should be making them do it, if we can. (Tho for my part, I regularly try to leave other people's code better than I found it, when I have to touch it.)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #29) > Sure. We should be making them do it, if we can. (Tho for my part, I > regularly try to leave other people's code better than I found it, when I > have to touch it.) Fair enough, I'm also not a fan of "Not my code, not my problem". I've done a lot of refactoring, cleanup and bug fixing of code I've never touched before (and I usually enjoy it), probably more than most. But here I'm just changing one line in a test and the suggested change would require fixing the actual Calendar code and other tests. Also this code being untyped doesn't help; if I had a compiler backing me up for a string -> date/number change I'd feel differently probably... Anyways I'll file a b2g bug before I land this, let's see what they think.

FWIW after discussing the test change with Gaia people, we decided to change the line: assert.deepEqual(new Date(currentDate.dataset.date), new Date(now)); to just: assert.equal(currentDate.dataset.date, now.toString());

Bleh I'm stupid. I backed this out because I realized I forgot about the other failures: (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #19) > This caused B2G xpcshell failures as well. > https://treeherder.mozilla.org/logviewer.html#?job_id=12424363&repo=mozilla- > inbound And these don't look completely trivial to fix so I'd rather not fix on inbound without a try run. https://hg.mozilla.org/integration/mozilla-inbound/rev/106e5ed4c5b2

This fixes the failing b2g xpcshell test. The test did: do_check_eq(new Date(cachedStats[key1].date).getTime() / 1000, Math.floor(timestamp / 1000)); It looks like they added the Math.floor and "/ 1000" to workaround the old behavior. I changed it to just: do_check_eq(cachedStats[key1].date.getTime(), timestamp);

Comment on attachment 8675801 [details] [diff] [review] Fix b2g xpcshell Review of attachment 8675801 [details] [diff] [review]: ----------------------------------------------------------------- Kinda rs=-y, but eh.

https://hg.mozilla.org/integration/mozilla-inbound/rev/daeb62169623 https://hg.mozilla.org/integration/mozilla-inbound/rev/2e1180c9e5ce

https://hg.mozilla.org/mozilla-central/rev/daeb62169623 https://hg.mozilla.org/mozilla-central/rev/2e1180c9e5ce