1187233 - Date constructor when called with Date object should create copy

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

`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

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: 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 2

[[ex-Mozilla] Agi Sferro | :agi]

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 3

[Jeff Walden [:Waldo]]

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

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

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 5

[[ex-Mozilla] Agi Sferro | :agi]

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 6

[Jeff Walden [:Waldo]]

(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 7

[Jeff Walden [:Waldo]]

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.

Comment 8

[Jeff Walden [:Waldo]]

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!

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

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 10

[[ex-Mozilla] Agi Sferro | :agi]

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 11

[Jeff Walden [:Waldo]]

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 12

[[ex-Mozilla] Agi Sferro | :agi]

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 13

[[ex-Mozilla] Agi Sferro | :agi]

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

Thanks Jeff! :-)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[[ex-Mozilla] Agi Sferro | :agi]

Sorry Ryan! Will do.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=674b07ff06d0

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e057fc16e2e0

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Pulsebot]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Jeff Walden [:Waldo]]

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.

Comment 21

[[ex-Mozilla] Agi Sferro | :agi]

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.

Comment 22

[Jan de Mooij [:jandem]]

What's the status of this? :)

Comment 24

[payments.otsme]

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); // [phone]T00: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); // [phone]T00: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)

Comment 25

[Jan de Mooij [:jandem]]

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

Comment 26

[Jan de Mooij [:jandem]]

Attachment: Rebased patch

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

Comment 27

[Jeff Walden [:Waldo]]

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.

Comment 28

[Jan de Mooij [:jandem]]

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

Comment 29

[Jeff Walden [:Waldo]]

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

Comment 30

[Jan de Mooij [:jandem]]

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

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce75fb8d5a8b

Comment 32

[Jan de Mooij [:jandem]]

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

Comment 33

[Jan de Mooij [:jandem]]

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

Comment 34

[Jan de Mooij [:jandem]]

Attachment: Fix b2g xpcshell

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 35

[Jeff Walden [:Waldo]]

Comment on attachment 8675801 [details] [diff] [review]
Fix b2g xpcshell

Review of attachment 8675801 [details] [diff] [review]:
-----------------------------------------------------------------

Kinda rs=-y, but eh.

Comment 36

[Pulsebot]

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

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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