Last Comment Bug 738511 - Date constructor doesn't work when passed a String object, apparently
: Date constructor doesn't work when passed a String object, apparently
Status: RESOLVED FIXED
[good first bug][mentor=Waldo][lang=c...
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Marten Schilstra
:
Mentors:
Depends on: 803472 868496 911081
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-22 17:50 PDT by Boris Zbarsky [:bz] (TPAC)
Modified: 2013-08-30 04:27 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected


Attachments
Patch to check for objects passed into js_Date. (1.04 KB, patch)
2012-04-30 08:50 PDT, Sriram [:sriramr]
no flags Details | Diff | Splinter Review
Patches new date to work like ES5 spec. (971 bytes, patch)
2012-05-29 09:53 PDT, Marten Schilstra
no flags Details | Diff | Splinter Review
Bug 738511 - Parses date string according to ES5 spec + source order (28.38 KB, patch)
2012-06-07 13:22 PDT, Marten Schilstra
no flags Details | Diff | Splinter Review
Bug 738511 - Parses date string according to ES5 spec + source order (1.63 KB, patch)
2012-06-07 13:26 PDT, Marten Schilstra
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (TPAC) 2012-03-22 17:50:17 PDT
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 [:bz] (TPAC) 2012-03-22 17:51:57 PDT
Simpler testcase:

  print(new Date(new String("2012-01-31")))
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-03-22 19:25:48 PDT
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 2012-03-23 05:04:24 PDT
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] 2012-03-23 14:14:09 PDT
(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 2012-03-23 17:41:01 PDT
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] 2012-03-23 18:05:06 PDT
(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] 2012-03-23 18:16:36 PDT
(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 2012-03-23 18:53:22 PDT
Thanks. Debugging it now ...
Comment 9 David Mandelin [:dmandelin] 2012-03-30 18:29:52 PDT
(In reply to Vinay from comment #8)
> Thanks. Debugging it now ...

How's it going? Need any help?
Comment 10 Vinay 2012-03-30 20:21:45 PDT
I think I'm close. Give me unto today.
Comment 11 Vinay 2012-04-02 19:39:57 PDT
Sorry, haven't been able to spend time on it. I'll work on it this (long) weekend.
Comment 12 David Mandelin [:dmandelin] 2012-04-12 11:48:15 PDT
(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] 2012-04-27 23:14:38 PDT
(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] 2012-04-30 08:50:52 PDT
Created attachment 619582 [details] [diff] [review]
Patch to check for objects passed into js_Date.
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-02 16:44:25 PDT
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] 2012-05-15 13:37:24 PDT
Sriram thanks for looking into this. Do you need any help from us to get this moving forward?
Comment 17 Marten Schilstra 2012-05-29 09:53:50 PDT
Created attachment 628000 [details] [diff] [review]
Patches new date to work like ES5 spec.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-03 10:32:14 PDT
[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] (remove +bmo to email) 2012-06-07 11:41:21 PDT
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 2012-06-07 13:22:13 PDT
Created attachment 631102 [details] [diff] [review]
Bug 738511 - Parses date string according to ES5 spec + source order

Added the source order as requested!
Comment 21 Marten Schilstra 2012-06-07 13:26:00 PDT
Created attachment 631105 [details] [diff] [review]
Bug 738511 - Parses date string according to ES5 spec + source order

I accidentally the patch.
Comment 22 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-07 16:52:30 PDT
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 2012-06-08 01:53:36 PDT
(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] 2012-06-08 04:18:50 PDT
https://hg.mozilla.org/mozilla-central/rev/51dcc42f3f95

(Merged by Ed Morley)

Note You need to log in before you can comment on or make changes to this bug.