Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: Marten Schilstra)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 affected)

Details

(Whiteboard: [good first bug][mentor=Waldo][lang=c++][js:p1])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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...
(Reporter)

Comment 1

5 years ago
Simpler testcase:

  print(new Date(new String("2012-01-31")))
Summary: Weird behavior with "this" on methods of String.prototype → Date constructor doesn't work when passed a String object, apparently
status-firefox14: --- → affected
tracking-firefox14: --- → +
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.
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug][mentor=Waldo][lang=c++]

Comment 3

5 years ago
Im new here, and was looking for something to get started. I'll look into this, and get back soon.
(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

5 years ago
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 ?
(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
(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

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

How's it going? Need any help?

Comment 10

5 years ago
I think I'm close. Give me unto today.

Comment 11

5 years ago
Sorry, haven't been able to spend time on it. I'll work on it this (long) weekend.
(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

5 years ago
(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

5 years ago
Created attachment 619582 [details] [diff] [review]
Patch to check for objects passed into js_Date.
Attachment #619582 - Flags: review?

Updated

5 years ago
Attachment #619582 - Flags: review? → review?(jwalden+bmo)
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.
Attachment #619582 - Flags: review?(jwalden+bmo)
Sriram thanks for looking into this. Do you need any help from us to get this moving forward?
Whiteboard: [good first bug][mentor=Waldo][lang=c++] → [good first bug][mentor=Waldo][lang=c++][js:p1][js:ni]
(Assignee)

Comment 17

5 years ago
Created attachment 628000 [details] [diff] [review]
Patches new date to work like ES5 spec.
(Assignee)

Updated

5 years ago
Attachment #628000 - Flags: review?(jwalden+bmo)
[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.
tracking-firefox14: + → ---
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.
Attachment #628000 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 20

5 years ago
Created attachment 631102 [details] [diff] [review]
Bug 738511 - Parses date string according to ES5 spec + source order

Added the source order as requested!
Attachment #628000 - Attachment is obsolete: true
Attachment #631102 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 21

5 years ago
Created attachment 631105 [details] [diff] [review]
Bug 738511 - Parses date string according to ES5 spec + source order

I accidentally the patch.
Attachment #631102 - Attachment is obsolete: true
Attachment #631102 - Flags: review?(jwalden+bmo)
Attachment #631105 - Flags: review?(jwalden+bmo)
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?
Attachment #631105 - Flags: review?(jwalden+bmo) → review+
Assignee: general → martenschilstra
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla16
(Assignee)

Comment 23

5 years ago
(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!
https://hg.mozilla.org/mozilla-central/rev/51dcc42f3f95

(Merged by Ed Morley)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=Waldo][lang=c++][js:p1][js:ni] → [good first bug][mentor=Waldo][lang=c++][js:p1]

Updated

5 years ago
Depends on: 803472

Updated

4 years ago
Depends on: 868496

Updated

4 years ago
Depends on: 911081
You need to log in before you can comment on or make changes to this bug.