738511 - Date constructor doesn't work when passed a String object, apparently

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Consider this snippet:

  (function() {
    String.prototype.toDate = function() {
        return new Date(this);
    };
    print("2012-01-31".toDate());
  })();

this prints "Invalid Date" in SpiderMonkey but prints a useful date in V8.  I think V8 is right here.

Doing (new String("2012-01-31")).toDate() also prints "Invalid Date", so this is not strictly to do with the primitive string stuff...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Simpler testcase:

  print(new Date(new String("2012-01-31")))

Comment 2

[Jeff Walden [:Waldo]]

js_Date in js/src/jsdate.cpp in the one-argument case (at least) doesn't match the spec algorithm.  Should be an easy fix, just follow the spec steps.  This should be a pretty easy first bug for someone -- happy to review a patch.  I've put myself down to mentor here, but I'm also kinda swamped at the moment, so others should feel free to chime in if they can answer any questions that get asked -- but I'll do the best I can if needed.

Comment 3

[Vinay]

Im new here, and was looking for something to get started. I'll look into this, and get back soon.

Comment 4

[David Mandelin [:dmandelin]]

(In reply to Vinay from comment #3)
> Im new here, and was looking for something to get started. I'll look into
> this, and get back soon.

Cool! Let us know if you need any help.

Comment 5

[Vinay]

I have a problem running the build (trunk). Get a seg fault. Here's what I see in gdb ... http://pastebin.mozilla.org/1532508

How can I build/run only the JS code. How can I get a repl to test the JS process ?

Comment 6

[David Mandelin [:dmandelin]]

(In reply to Vinay from comment #5)
> I have a problem running the build (trunk). Get a seg fault. Here's what I
> see in gdb ... http://pastebin.mozilla.org/1532508
> 
> How can I build/run only the JS code. How can I get a repl to test the JS
> process ?

See:

https://developer.mozilla.org/En/SpiderMonkey/Build_Documentation

That will tell you how to build SpiderMonkey standalone, which includes a repl. Docs on the repl options and builtin functions:

https://developer.mozilla.org/En/SpiderMonkey/Introduction_to_the_JavaScript_shell

Comment 7

[David Mandelin [:dmandelin]]

(In reply to Vinay from comment #5)
> I have a problem running the build (trunk). Get a seg fault. Here's what I
> see in gdb ... http://pastebin.mozilla.org/1532508
> 
> How can I build/run only the JS code. How can I get a repl to test the JS
> process ?

Oh, and I forgot before: You can always jump on our IRC channel #jsapi on irc.mozilla.org to ask questions or talk about what you're doing.

Comment 8

[Vinay]

Thanks. Debugging it now ...

Comment 9

[David Mandelin [:dmandelin]]

(In reply to Vinay from comment #8)
> Thanks. Debugging it now ...

How's it going? Need any help?

Comment 10

[Vinay]

I think I'm close. Give me unto today.

Comment 11

[Vinay]

Sorry, haven't been able to spend time on it. I'll work on it this (long) weekend.

Comment 12

[David Mandelin [:dmandelin]]

(In reply to Vinay from comment #11)
> Sorry, haven't been able to spend time on it. I'll work on it this (long)
> weekend.

How's it going?

Comment 13

[Sriram [:sriramr]]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #2)
> js_Date in js/src/jsdate.cpp in the one-argument case (at least) doesn't
> match the spec algorithm.  Should be an easy fix, just follow the spec
> steps.  This should be a pretty easy first bug for someone -- happy to
> review a patch.  I've put myself down to mentor here, but I'm also kinda
> swamped at the moment, so others should feel free to chime in if they can
> answer any questions that get asked -- but I'll do the best I can if needed.

Hey Jeff, I'm new here. Is this bug assigned to someone?? Can I work on this? Because I think I already have a solution..Let me know. 

Thanks
Sriram

Comment 14

[Sriram [:sriramr]]

Attachment: Patch to check for objects passed into js_Date.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 619582 [details] [diff] [review]
Patch to check for objects passed into js_Date.

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

::: js/src/jsdate.cpp
@@ +2618,5 @@
>      if (args.length() == 0) {
>          d = NowAsMillis();
>      } else if (args.length() == 1) {
> +        
> +        if (!args[0].isString() && !args[0].isObject()) {

This isn't quite right.

http://es5.github.com/#x15.9.3.2

Per that algorithm, we should be calling ToPrimitive on the argument, then checking for a string.  Here, you're (if I invert the checks a little) checking for a string or object.  You're probably best off just implementing the algorithm directly from the steps -- ToPrimitive (in jsobjinlines.h), then v.isString() and splitting accordingly, then letting the resulting value flow out of the |else if| and into the |js_NewDateObjectMsec| call.

Does that make sense?  I'm happy to explain further if necessary.  It's probably easiest to catch me on IRC, or you could ask in #jsapi and a variety of people there should be able to help you.

Comment 16

[Naveed Ihsanullah [:naveed]]

Sriram thanks for looking into this. Do you need any help from us to get this moving forward?

Comment 17

[Marten Schilstra]

Attachment: Patches new date to work like ES5 spec.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]
Not clear why this is being tracked for 14, please make your case when there's a fix if it's being nominated for uplift.

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 628000 [details] [diff] [review]
Patches new date to work like ES5 spec.

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

This looks like it implements the right behavior.  For better readability, would you mind inverting the order of the |if (is a string) parseAsDate() else ToNumber()| code just below what you've added?  Then our code will have all the steps in source order.  I had to twist my brain around a little to double-check that everything matched up.

Comment 20

[Marten Schilstra]

Attachment: Bug 738511 - Parses date string according to ES5 spec + source order

Added the source order as requested!

Comment 21

[Marten Schilstra]

Attachment: Bug 738511 - Parses date string according to ES5 spec + source order

I accidentally the patch.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 631105 [details] [diff] [review]
Bug 738511 - Parses date string according to ES5 spec + source order

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

Hmm, Splinter fails to work on this patch -- odd...

Patch looks good.  I belatedly noticed some things I should have commented on earlier.  When we write comments that are complete sentences, we capitalize them, punctuate them, and so on -- make 'em all grammatically correct.  And when implementing spec algorithms, we generally put a spec reference in a comment like /* ES5 1.2.3. */ and /* Step 1. */ and so on on all the steps in the source.  Also, we include tests in our patches when we fix bugs.  :-)  Since I should have noted this stuff earlier, I just went ahead and did it, rather than throw the ball back at you here.  Feel free to take a look at the whole patch as committed to see the exact changes I made:

https://hg.mozilla.org/integration/mozilla-inbound/rev/51dcc42f3f95

Thanks for the patch!  You need help finding any other interesting bugs to fix?

Comment 23

[Marten Schilstra]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #22)

> Thanks for the patch!  You need help finding any other interesting bugs to
> fix?

Noted your comments, and yes, throw me another interesting bug if you can!

Comment 24

[Graeme McCutcheon [:graememcc]]

https://hg.mozilla.org/mozilla-central/rev/51dcc42f3f95

(Merged by Ed Morley)