202019 - Built-in functions should not define constructor automatically

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Igor Bukanov]

From jseng newsgroup:

------- Original Message --------
Subject: Re: ECMAScript standard and new parseInt("100")
Date: 14 Apr 2003 18:50:57 GMT
From: [e-mail] (Waldemar Horwat)
Organization: Another Netscape Collabra Server User
To: [e-mail]
Newsgroups: netscape.public.mozilla.jseng
References: <b7ekac$[e-mail]>

At 5:43 PM +0200 4/14/03, Igor Bukanov wrote:
>Does ECMAScript standard says anything about calling function 
>properties of the standard objects as constructors? For example, 
>does it specify in any way what should be the result of new 
>parseInt("100"), new "1".valueOf() ?

Remarkably, it does.  These should produce errors, as stated on the 
first page of ECMAScript Edition 3, chapter 15: "None of the built-in 
functions described in this section shall implement the internal 
[[Construct]] method unless otherwise specified ...."

------- End of Original Message --------

But currently is SM:

js> new parseInt  
[object Object]
js> new "".concat
[object Object]

In Rhino:

js> typeof(new parseInt)
undefined
js> typeof(new "".concat)
undefined

which is not what the standard says.

Are there any particular reasons not to follow the standard and have
[[Constructor]] implemented for built-ins?

Comment 1

[Phil Schwartau]

Reassigning; cc'ing Brendan, Waldemar -

Comment 2

[Phil Schwartau]

Compare the results of these statements in the current SpiderMonkey shell:

js> obj = new this;
5: TypeError: object is not a constructor

js> obj = new Math;
6: TypeError: Math is not a constructor


--------------------------------  vs.  --------------------------------

js> obj = new eval;
[object Object]
js> obj.toSource();
({})

js> obj = new Date.parse;
[object Object]
js> obj.toSource();
({})

Comment 3

[Brendan Eich [:brendan]]

Does that verbiage from chapter 15 trump chapter 16, the second exception?

SpiderMonkey has been like this forever; it never seemed worth the code bloat to
make errors out of such new expressions.

/be

Comment 4

[Igor Bukanov]

As far as I can tell MSIE 6.0 behaves as required by the standard. Although I
agree with Brendan that the standard should not state it since it introduces one
more feature that that can not be implemented in JS itself, but still it is the
standard.

Comment 5

[Phil Schwartau]

Attachment: HTML testcase

Comment 6

[Phil Schwartau]

The HTML testcase try...catches these two statements:

obj = new eval;
obj = new Date.parse;

On each one, IE6 gives the error, "Object doesn't support this action"

Comment 7

[x0]

Built-in native objects also have a "prototype" property which they should not have.

(In reply to comment #3)
> Does that verbiage from chapter 15 trump chapter 16, the second exception?

I don't think the second exception of chapter 16 applies here. The implementation doesn't provide additional types, values, objects, properties, or functions here; it just returns (or defines) a standard one where it should not.

If this won't fix, then it should probably resolved as such after 5 years without activity.

Comment 8

[Brendan Eich [:brendan]]

x0, care to take it? A very lightweight patch would be something to consider (yet another function flag, to opt out of construct while remaining call-able).

/be

Comment 9

[x0]

If the function had no prototype, the |new| would fail automatically, I think. But I don't know where built-in functions get it and have enough bugs at the moment.

And that won't work if somebody adds a prototype property later.

Comment 10

[Jeff Walden [:Waldo]]

The class methods (Object, Number, etc.) get their prototype in js_InitClass.  Built-in functions get it in fun_resolve.  It would be sufficient to add a FUN_INTERPRETED(fun) check to the if-is-prototype-atom code sequence, except that Function.prototype is an interpreted function (see js_InitFunctionClass), and it seems likely to be important to ensure that doesn't have a prototype property either -- else it would be the case that:

assertEq(parseInt.prototype, Function.prototype.prototype);
assertEq(parseFloat.prototype, Function.prototype.prototype);
assertEq(parseInt.prototype, parseFloat.prototype);
assertEq("prototype" in parseInt, true);
assertEq("prototype" in parseFloat, true);
assertEq(Function.prototype.prototype !== undefined, true);

The in-tests are probably the worst of it, but the sharing-via-shared-proto hazard is worrisome too.

Comment 11

[Jason Orendorff [:jorendorff]]

This bug is the cause of a bunch of failures on Sputnik conformance tests. I think they test every stdlib function separately to make sure you can't use the new operator on it. Awesome. :)

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 13

[Jason Orendorff [:jorendorff]]

Attachment: v2 - new comment, use B for built-in instead of S for stdlib

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> Created an attachment (id=432205) [details]
> v2 - new comment, use B for built-in instead of S for stdlib

What about making the fast native non-constructable by default? Or, given that a fast native cannot detect if it was called as a constructor unless it analyzes caller's bytecode, what about simply making then unconditionally non-constructable?

Comment 15

[Jeff Walden [:Waldo]]

(In reply to comment #14)
> Or, given that a fast native cannot detect if it was called as a constructor
> unless it analyzes caller's bytecode, what about simply making then
> unconditionally non-constructable?

Your ideas intrigue me and I wish to subscribe to your newsletter!

Comment 16

[Brendan Eich [:brendan]]

Comment 14 is a good idea if all ECMA-262-prescribed built-ins can be implemented as JSFastNatives. IIRC we changed all built-in natives for non-constructors to be fast when we introduced fast natives. I'm too tired to check atm.

/be

Comment 17

[Igor Bukanov]

(In reply to comment #16)
> Comment 14 is a good idea if all ECMA-262-prescribed built-ins can be
> implemented as JSFastNatives. 

I do not suggest to implement all built-ins as fast natives. Rather from the patch I see is that all fast natives in our code is going to have JSNFUN_NO_CONSTRUCT. Given this and that a fast native effectively cannot learn that it was called as a constructor, the idea is to simply imply that a fast native have JSNFUN_NO_CONSTRUCT.

For non-fast natives we still need such flag. But than again, at some point we should introduce a fast call convention for the constructor call. One possibility for that would be to move JSFRAME_CONSTRUCTING from JSStackFrame::flags into JSContext field and keep the fast native signature as it is now. With this in place what would be necessary is a flag like JSNFUN_CONSTRUCT to denote fast natives that could be used in a call context, an opposite to JSNFUN_NO_CONSTRUCT.

Of cause, having 2 flags like JSNFUN_CONSTRUCT and JSNFUN_NO_CONSTRUCT is jus bad. Hence I think for now in this bug we should not add JSNFUN_NO_CONSTRUCT to the jsapi.h. Rather we should keep it internal and use it for few non-fast-natives that should not be constructable according to ECMA 262. When we convert all these to be fast natives, we simply remove the flag.

Comment 18

[Jason Orendorff [:jorendorff]]

It's a reasonable idea, but I'd rather not change the behavior for fast natives outside js/src, such as XPConnect quick stubs. I don't know what would be affected, but it would be a much larger set of functions and therefore a riskier change.

Comment 19

[Brendan Eich [:brendan]]

Re: comment 18: what is the risk? Fast natives can't be constructors already. If we impute JSNFUN_NO_CONSTRUCT for all fast natives, what breaks?

/be

Comment 20

[Jason Orendorff [:jorendorff]]

It looks like nothing breaks. I'll make the change suggested in comment 14.

Comment 21

[Brendan Eich [:brendan]]

Slow natives going away, time to fix this spec-conformance issue as proposed?

/be

Comment 22

[Brendan Eich [:brendan]]

Note that JSFUN_NO_CONSTRUCT can't be 0x0001 any longer. Next value free is 0x4 IIRC (odd; may warrant reordering slightly in jsapi.h and jsfun.h; wish for a better public/private split and mechanism).

The patch in bug 445319 uses 0x0800 for JSFUN_PROTOTYPE (which flags any function object instance that is created as a Function.protoype value, which must not have a .prototype property).

/be

Comment 23

[Jason Orendorff [:jorendorff]]

Untaking.

Comment 24

[Tom S [:evilpie]]

Attachment: Dont allow |new| for built-ins methods and Function.prototype

Tiny patch, don't know if there is some corner case which doesn't work, but the testcase from jorendorf passes.

Comment 25

[Igor Bukanov]

(In reply to comment #24)
> Tiny patch, don't know if there is some corner case which doesn't work, but the
> testcase from jorendorf passes.

This is nice, but does it affects any tests? If not, we need one.

Comment 26

[Jeff Walden [:Waldo]]

I will go one step further: even if it does affect tests, we still need one.  If those affected tests are changed or removed at some future time (uncommon, but it does happen), we could regress and not notice it.  It's best to have a test specifically for each individual issue as it arises, making the feature-is-tested-properly property more readily apparent.

Comment 27

[Tom S [:evilpie]]

Igor: Passes, only one failure, because of an bad written test:
  //bug579740.js
  for (a = 0; a < 4; a++) {
     new Math.round(0).t
  }
Jeff Walden: I think Jason Orendorff's test is pretty good.

Comment 28

[Jeff Walden [:Waldo]]

Ah, yes, that does look quite good (what else would we expect from jorendorff?).  Can you fold it into the patch so as to be explicit about what parts of the patch are tested, and to make the eventual checkin process simpler?

Comment 29

[Igor Bukanov]

(In reply to comment #26)
> I will go one step further: even if it does affect tests, we still need one. 

Right. I would even suggest to test all native methods defined in the standard host objects and use something like:

var list = [Object.prototype, 
            String.prototype, 
            Date.prototype, 
            Number.prototype, 
            RegExp.prototype,
            Math];

for (var i = 0; i != list.length; ++i) {
    var obj = list[i]; 
    var names = Object.getOwnPropertyNames(list[i]);
    for (var j = 0; j != names.length; ++i) {
        var name = names[i];
        if (name == "constructor")
            continue;
        var method = obj[name];
        if (typeof method == "function") 
            CheckCannotConstruct(method);
    }
}

and then do the same for some global object properties like parseFloat, isFinite etc.

Comment 30

[Igor Bukanov]

(In reply to comment #29)
> Right. I would even suggest to test all native methods defined in the standard
> host objects and use something like:

And the test from the patch in the comment 13 does that and more :)

Comment 31

[Brendan Eich [:brendan]]

This is getting in our hair (bug 607243) and it's an ECMA-262 violation. It is also pretty easy to fix, although all natives are fast now (but fast natives, I mean natives, can be constructors). Taking.

/be

Comment 32

[Tom S [:evilpie]]

I had an working patch for this, would have uploaded it tomorow as its late now. I made Function.prototype a native function, sowe would not need the extra flag for it anymore.

Comment 33

[Brendan Eich [:brendan]]

One bogus trace-test doesn't like Tom's patch:

/Users/brendaneich/Hacking/hg.mozilla.org/tracemonkey/js/src/jit-test/tests/basic/bug579740.js:3: TypeError: Math.round is not a constructor

but this is a test bug. Running jsreftests now...

/be

Comment 34

[Brendan Eich [:brendan]]

Tom: do not change Function.prototype, it must decompile as an empty scripted function, not as a native function.

Your patch is good, it passes jsreftests. I will fix the jit-test that wrongly tries to construct Math.round, test some more, and try to land your patch with credit. Thanks,

/be

Comment 35

[Brendan Eich [:brendan]]

Attachment: proposed fix

Based directly on Tom's. I just fixed the jit-test and added a reftest. I'm rebuilding Minefield now, will test harder before pushing.

/be

Comment 36

[Brendan Eich [:brendan]]

I sorted tests/ecma_5/Function/jstests.list, it was getting long enough that finding things was harder than trivial. Any sufficiently long file list should be sorted; historical add order is irrelevant (but in hg).

/be

Comment 37

[Brendan Eich [:brendan]]

jstestbrowser passed

Looking good. Will try more tests or push to try (I have a hard time not touching my laptop during the sensitive mochitests -- great skill at moving the cursor at just the wrong time...).

/be

Comment 38

[Tom S [:evilpie]]

There is an bug in my patch, which was the reason i wanted to convert Function.prototype to an native function. Because currently its an interpreted function and in some cases the interpreter or tracing jit, don't call InvokeConstructor for these function types. Your test needs an fix too.

new Function.prototype // should throw TypeError

Comment 39

[Tom S [:evilpie]]

Attachment: my idea

Comment 40

[Tom S [:evilpie]]

Attachment: my idea 2

Comment 41

[Igor Bukanov]

Comment on attachment 486179 [details] [diff] [review]
proposed fix

>+function checkMethod(method) {
>+    try {
>+        new method();
>+    } catch (e) {
>+        assertEq(e.message, "method is not a constructor");
>+    }
>+}

The method must assert that the code right after the new is not reachable.

>+
>+function checkMethods(proto) {
>+    var names = Object.getOwnPropertyNames(proto);
>+    for (var i = 0; i < names.length; i++) {
>+        var prop = proto[names[i]];
>+        if (typeof prop === "function")
>+            checkMethod(prop);
>+    }
>+}

The loop must skip names[i] when it is "constructor".

>+
>+var builtin_ctors = [
>+    Object, Function, Array, String, Boolean, Number, Date, RegExp, Error,
>+    EvalError, RangeError, ReferenceError, SyntaxError, TypeError, URIError
>+];
>+
>+for (var i = 0; i < builtin_ctors.length; i++) {
>+    checkMethods(builtin_ctors[i].prototype);
>+}
>+
>+var builtin_funcs = [
>+    eval, isFinite, isNaN, parseFloat, parseInt,
>+    decodeURI, decodeURIComponent, encodeURI, encodeURIComponent
>+];
>+
>+for (var i = 0; i < builtin_funcs.length; i++) {
>+    checkMethod(builtin_funcs[i]);
>+}
>+
>+checkMethods(Math);

JSON is missed here.

r+ with this fixed

Comment 42

[Brendan Eich [:brendan]]

Comment on attachment 486316 [details] [diff] [review]
my idea 2

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>--- a/js/src/jsfun.cpp
>+++ b/js/src/jsfun.cpp
>@@ -2127,16 +2127,20 @@ fun_toStringHelper(JSContext *cx, JSObje
>                              js_Function_str, js_toString_str,
>                              "object");
>         return NULL;
>     }
> 
>     JSFunction *fun = GET_FUNCTION_PRIVATE(cx, obj);
>     if (!fun)
>         return NULL;
>+    if (fun->isFunctionPrototype()) {
>+        fun = js_NewFunction(cx, NULL, NULL, 0, JSFUN_INTERPRETED, NULL, NULL);
>+        fun->u.i.script = JSScript::emptyScript();
>+    }
>     return JS_DecompileFunction(cx, fun, indent);

This is too much code to accomplish the goal of making Function.prototype not constructable, but worse: this bloat is bad precedent and a soft spot some tester or ACID4 test will probe to our detriment in the future. We shouldn't generate temporary garbage like this just to get a toString result.

/be

Comment 43

[Luke Wagner [:luke]]

Comment on attachment 486316 [details] [diff] [review]
my idea 2

>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp
>+++ b/js/src/jsinterp.cpp
>     if (clasp == &js_FunctionClass) {
>         fun = callee->getFunctionPrivate();
>         if (fun->isConstructor()) {
>             args.thisv().setMagicWithObjectOrNullPayload(NULL);
>             return CallJSNativeConstructor(cx, fun->u.n.native, args.argc(), args.base());
>         }
>+        if (fun->isNative() || fun->isFunctionPrototype()) {
>+            js_ReportIsNotFunction(cx, &args.callee(), JSINVOKE_CONSTRUCT);
>+            return false;
>+        }
>     } else if (clasp->construct) {
>         args.thisv().setMagicWithObjectOrNullPayload(NULL);
>         return CallJSNativeConstructor(cx, clasp->construct, args.argc(), args.base());
>     }

Was it your intention to allow non-function objects with clasp->call (but not clasp->construct) to continue to be called as constructors?  If so, then bug 608311 is not quite a dup.

Comment 44

[Tom S [:evilpie]]

I didn't even known such classes can/do exists. But new ("".indexof) should throw anyway, as it did with my patch.

Comment 45

[Luke Wagner [:luke]]

(In reply to comment #44)
> I didn't even known such classes can/do exists. But new ("".indexof) should
> throw anyway, as it did with my patch.

Its something embeddings can do.  xpconnect (Firefox) does.

Comment 46

[Brendan Eich [:brendan]]

(In reply to comment #43)
> Comment on attachment 486316 [details] [diff] [review]
> my idea 2
> 
> >diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
> >--- a/js/src/jsinterp.cpp
> >+++ b/js/src/jsinterp.cpp
> >     if (clasp == &js_FunctionClass) {
> >         fun = callee->getFunctionPrivate();
> >         if (fun->isConstructor()) {
> >             args.thisv().setMagicWithObjectOrNullPayload(NULL);
> >             return CallJSNativeConstructor(cx, fun->u.n.native, args.argc(), args.base());
> >         }
> >+        if (fun->isNative() || fun->isFunctionPrototype()) {
> >+            js_ReportIsNotFunction(cx, &args.callee(), JSINVOKE_CONSTRUCT);
> >+            return false;
> >+        }
> >     } else if (clasp->construct) {
> >         args.thisv().setMagicWithObjectOrNullPayload(NULL);
> >         return CallJSNativeConstructor(cx, clasp->construct, args.argc(), args.base());
> >     }
> 
> Was it your intention to allow non-function objects with clasp->call (but not
> clasp->construct) to continue to be called as constructors?  If so, then bug
> 608311 is not quite a dup.

This is a long-standing separate bug. It violates ES5 11.2.2, or extends it at the least (licitly by Clause 16? Dunno, get another language lawyer). Not this bug and not bug 608311, which is only about builtins, or *a* builtin function object.

Here's a shell session showing an object that has clasp->call but does not have clasp->construct:

js> r = /hi/
/hi/
js> RegExp.input = "hihi"
"hihi"
js> t = new r
["hi"]
js> typeof t
"object"

This needs filing.

/be

Comment 47

[Luke Wagner [:luke]]

Filed as bug 608421.

Comment 48

[Brendan Eich [:brendan]]

This is a JS API change: callers of JS_DefineFunction who want the native function they are defining to be callable as a constructor must now pass JSFUN_CONSTRUCTOR in the flags parameter.

Likewise, JS_DefineFunctions callers must set JSFUN_CONSTRUCTOR in the flags member of JSFunctionSpec *fs elements intended to define constructors.

I thought about imputing this flag for backward compatibility, but it is not what ECMA-262 wants, and it seems we're already using JS_DefineFunction{,s} to define non-constructors and constructors, with the JSFUN_CONSTRUCTOR flag telling the difference.

/be

Comment 49

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Comment 50

[Brendan Eich [:brendan]]

Comment on attachment 487089 [details] [diff] [review]
proposed fix, v2

>@@ -1253,16 +1253,20 @@ InvokeConstructor(JSContext *cx, const C
>     Class *clasp = callee->getClass();
>     JSFunction *fun = NULL;
>     if (clasp == &js_FunctionClass) {
>         fun = callee->getFunctionPrivate();
>         if (fun->isConstructor()) {
>             args.thisv().setMagicWithObjectOrNullPayload(NULL);
>             return CallJSNativeConstructor(cx, fun->u.n.native, args.argc(), args.base());
>         }
>+        if (fun->isNative() || fun->isFunctionPrototype()) {
>+            js_ReportIsNotFunction(cx, &args.callee(), JSINVOKE_CONSTRUCT);
>+            return false;
>+        }
>     } else if (clasp->construct) {
>         args.thisv().setMagicWithObjectOrNullPayload(NULL);
>         return CallJSNativeConstructor(cx, clasp->construct, args.argc(), args.base());
>     }
> 
>     /* Scripts create their own |this| in JSOP_BEGIN */
>     if (!fun || !fun->isInterpreted()) {
>         JSObject *obj = js_CreateThis(cx, callee);

It would be nicer (and match the flag name) if JSFUN_CONSTRUCTOR applied to scripted as well as native functions. Then the essential part of the above would change to

>+        if (!fun->isConstructor()) {
>+            js_ReportIsNotFunction(cx, &args.callee(), JSINVOKE_CONSTRUCT);
>+            return false;
>+        }
>         if (fun->isNative()) {
>             args.thisv().setMagicWithObjectOrNullPayload(NULL);
>             return CallJSNativeConstructor(cx, fun->u.n.native, args.argc(), args.base());
>         }

and in the following:

>@@ -4590,17 +4594,17 @@ BEGIN_CASE(JSOP_NEW)
>     JS_ASSERT(vp >= regs.fp->base());
> 
>     /*
>      * Assign lval, callee, and newfun exactly as the code at inline_call: expects to
>      * find them, to avoid nesting a js_Interpret call via js_InvokeConstructor.
>      */
>     if (IsFunctionObject(vp[0], &callee)) {
>         newfun = callee->getFunctionPrivate();
>-        if (newfun->isInterpreted()) {
>+        if (newfun->isInterpreted() && !newfun->isFunctionPrototype()) {

We would test newfun->isInterpreted() and newfun->isConstructor(), and JSFUN_PROTOTYPE set would imply JSFUN_CONSTRUCTOR not set.

The two-bit test would be done in one instruction without any branch for && -- perhaps GCC and MSVC are smart enough to do that with the >+ version from the patch, though.

Luke, thoughts?

/be

Comment 51

[Luke Wagner [:luke]]

Comment on attachment 487089 [details] [diff] [review]
proposed fix, v2

This looks good.  I'm assuming you've audited all those natives-now-JSFUN_CONSTRUCTORs to ensure that, viz., they don't expect a this value.

Comment 52

[Brendan Eich [:brendan]]

(In reply to comment #51)
> Comment on attachment 487089 [details] [diff] [review]
> proposed fix, v2
> 
> This looks good.  I'm assuming you've audited all those
> natives-now-JSFUN_CONSTRUCTORs to ensure that, viz., they don't expect a this
> value.

Kind of. Got sleepy, lotta natives.

Any thoughts on the idea in comment 50?

/be

Comment 53

[Luke Wagner [:luke]]

(In reply to comment #52)
> Any thoughts on the idea in comment 50?

That sounds good.  Combined with bug 608421, InvokeConstructor would be much simpler.

Comment 54

[Luke Wagner [:luke]]

Care to land?

Comment 55

[Brendan Eich [:brendan]]

This needs rebasing and more testing. I was going to universalize JSFUN_CONSTRUCTOR as proposed, too.

/be

Comment 56

[Jeff Walden [:Waldo]]

The patch here doesn't seem to touch the tracer at all, which seems presumptively wrong.

Comment 58

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #57)
> *** Bug 621420 has been marked as a duplicate of this bug. ***

Renominating because there is an almost-finished r+ patch, and it fixes asserts that fuzzers hit.

Comment 59

[David Mandelin [:dmandelin]]

(In reply to comment #58)
> (In reply to comment #57)
> > *** Bug 621420 has been marked as a duplicate of this bug. ***
> 
> Renominating because there is an almost-finished r+ patch, and it fixes asserts
> that fuzzers hit.

Comments 55 and 56 imply that it's not really finished. Brendan, is this in fact close?

Comment 60

[Brendan Eich [:brendan]]

There's more to do, and I don't recommend we delay Firefox 4 for this bug.

/be

Comment 61

[Brendan Eich [:brendan]]

I'll get this for the next major release.

/be

Comment 62

[Tom S [:evilpie]]

Attachment: v1 2011

This patch aims to change the least possible amount of code. I think we should do two followup bugs:
1. Fold isNative || isFunctionPrototype in some check like hasPrototypeProperty
2. Fix var x = parseInt.bind(); new x, the error message should say "x is not a constructor" instead of "new x is not a constructor"

I didn't need to touch the tracer, because it seems to be already fixed by Bug 630865.
I only made a small change to the methodjit, David, please have look if this is enough.
I removed some test, because they only tested wrong behavior.

Comment 63

[Brendan Eich [:brendan]]

Comment on attachment 518058 [details] [diff] [review]
v1 2011

This looks good to me. File the followups and cite them in FIXME: comments in the relevant code?

>+        if (fun->isNative() || fun->isFunctionPrototype())
>+            goto error;
>+
>+        if (!Invoke(cx, args, JSINVOKE_CONSTRUCT))
>+            return false;
>+
>+        JS_ASSERT(args.rval().isObject());
>+        JS_RUNTIME_METER(cx->runtime, constructs);
>+        return true;
>+    }

Blank line here.

>+    if (clasp->construct) {

dvander should weigh in on JM. I seem to recall an optimized construct-this path but maybe it doesn't need changing.

/be

Comment 64

[Tom S [:evilpie]]

Attachment: v2 - 2011

Hope i got the patch right this time. Fixed nits, and removed check for null parent (aka global check), to maintain similarity to Invoke.

Comment 66

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 518390 [details] [diff] [review]
v2 - 2011

r=me on methodjit changes

Comment 67

[Tom S [:evilpie]]

Field Bug 642576 for the XXX. And Bug 642579 on the error message.

Comment 68

[Tom S [:evilpie]]

Review ping

Comment 69

[Jason Orendorff [:jorendorff]]

Comment on attachment 518390 [details] [diff] [review]
v2 - 2011

This doesn't compile in GCC, at least the version I have:

../jsinterp.cpp: In function ‘bool js::InvokeConstructor(JSContext*, const js::CallArgs&)’:
../jsinterp.cpp:1286: error: jump to label ‘error’
../jsinterp.cpp:1256: error:   from here
../jsinterp.cpp:1259: error:   crosses initialization of ‘js::Class* clasp’
../jsinterp.cpp:1258: error:   crosses initialization of ‘JSObject* callee’

But apart from that it looks fine. I'll land it tonight or tomorrow with the obvious fix.

Comment 70

[Luke Wagner [:luke]]

Good work, great to see this landing!

One request: this
  fun->isInterpreted() && !fun->isFunctionPrototype()
predicate shows up in three places.  Before this gets lost in time, could we have that factored-out predicate, perhaps in JSFunction?  But not hasNoPrototype, as the FIXME says, but something relevant to construction like JSFunction::canCallWithNew() that has a comment citing the relevant ES verses?

Comment 71

[Jason Orendorff [:jorendorff]]

I made the change requested in comment 70.

I called it fun->isInterpretedConstructor(). My hope is that bug 642576 will get rid of it again. :)

Anyway, then I chickened out on actually landing this, since I don't have time to watch the tree right now, and pushed it to Try Server instead, along with a bundle of other changes. Now I plan to push this tomorrow morning.

Not sure if evilpies is familiar with Try Server. Basically watch for a build to show up here (it'll have my name on it -- i'll put your name on the final check-in): http://tbpl.mozilla.org/?tree=MozillaTry When it does, green is good. Orange and red are bad.

Comment 72

[Jason Orendorff [:jorendorff]]

It looks like this is causing some mochitests to fail with "XPCNativeWrapper is not a constructor".

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1302804805.1302809067.9421.gz#err1

See js/src/xpconnect/src/XPCWrapper.cpp line 112.

Comment 73

[Tom S [:evilpie]]

Attachment: v3

Comment 74

[Tom S [:evilpie]]

Attachment: v4

I suck at hg, hopefully this works.

Comment 75

[Jason Orendorff [:jorendorff]]

No, something is broken.

>+        if (clasp->construct) {
>+            return CallJSNativeConstructor(cx, clasp->construct, args.argc(), args.base());
>+    }

I am going to fix it up and push it.

Comment 76

[Tom S [:evilpie]]

Fixed that locally, but forgot to actually attach the patch here again...

Comment 77

[Jason Orendorff [:jorendorff]]

No worries. Running the js tests now. I will push it if these pass.

Comment 78

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #77)
> No worries. Running the js tests now. I will push it if these pass.

I think this got pushed at:

http://hg.mozilla.org/tracemonkey/rev/e02df4a05968

jorendorff, perhaps you'd like to r+ v4 of the patch?

Comment 79

[Tom S [:evilpie]]

Got backed-out, we need to fix something in NewXMLHttpRequest and NewChromeWorker.

Comment 80

[Jason Orendorff [:jorendorff]]

Comment on attachment 526354 [details] [diff] [review]
v4

I should have marked this r+ when I landed it, but it bounced, so there's no point marking it r+ now. Clearing the review flag.

I think evilpies has everything he needs to put together an updated patch.

Comment 81

[Tom S [:evilpie]]

Attachment: v5 Dissallow new on objects without [[Construct]] and with [[Call]]

Comment 82

[Jason Orendorff [:jorendorff]]

Comment on attachment 528167 [details] [diff] [review]
v5 Dissallow new on objects without [[Construct]] and with [[Call]]

Review of attachment 528167 [details] [diff] [review]:

Looks good apart from the whitespace change. r=me.

::: dom/src/threads/nsDOMWorker.cpp
@@ +696,5 @@
   nsCOMPtr<nsIXPConnectJSObjectHolder> workerWrapped;
   jsval v;
+  rv = nsContentUtils::WrapNative(aCx, obj, 
+                                  static_cast<nsIWorker*>(newWorker), &v, 
+                                  getter_AddRefs(workerWrapped));

Undo this whitespace change.

Comment 83

[Tom S [:evilpie]]

https://hg.mozilla.org/tracemonkey/rev/91c167c210fe

Phew, first own commit.

Comment 84

[Paul Wright]

Congrats Tom!

Comment 85

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/91c167c210fe

Comment 86

[David Bruant]

There is the dev-doc-needed keyword. What is to be documented?
Built-in functions used as constructors isn't something people were widely doing as far as I know.

Comment 87

[Eric Shepherd [:sheppy]]

Is this on Mozilla 6 or Mozilla 5?

Comment 88

[Jason Orendorff [:jorendorff]]

(In reply to comment #86)
> There is the dev-doc-needed keyword. What is to be documented?
> Built-in functions used as constructors isn't something people were widely
> doing as far as I know.

It is a change in behavior that could break someone's JS code. It would be nice to have something clear ("the old behavior was non-standard; it never worked that way in other browsers") and helpful ("you can fix your code by removing the 'new' keyword") to point that person to when they complain.

Comment 89

[Tom S [:evilpie]]

In practice most of the code using |new|, probably never really worked, so this unlikely to be an issue for most developers.

Comment 90

[Eric Shepherd [:sheppy]]

This doesn't appear to be documented anywhere, so I've just added a note to Firefox 6 for developers about it. It will migrate to a New in JavaScript X page when one exists for the corresponding JS version.

https://developer.mozilla.org/en/Firefox_6_for_developers#JavaScript

Comment 91

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on WinXP, Mac OS X 10.6, Win 7 x86, Ubuntu 11.04 x86 using the attached testcase. Got the following two messages:

UNEXPECTED ERROR:
eval is not a constructor

UNEXPECTED ERROR:
Date.parse is not a constructor

Setting status to Verified Fixed.