Last Comment Bug 202019 - Built-in functions should not define constructor automatically
: Built-in functions should not define constructor automatically
Status: VERIFIED FIXED
fixed-in-tracemonkey
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P2 normal with 1 vote (vote)
: mozilla6
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
: 621420 641606 (view as bug list)
Depends on: 445319
Blocks: test262 sputnik 608311 608421 621420
  Show dependency treegraph
 
Reported: 2003-04-14 14:41 PDT by Igor Bukanov
Modified: 2011-07-28 05:13 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
HTML testcase (264 bytes, text/html)
2003-04-15 20:35 PDT, Phil Schwartau
no flags Details
v1 (59.75 KB, patch)
2010-03-12 10:29 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 - new comment, use B for built-in instead of S for stdlib (60.62 KB, patch)
2010-03-12 12:30 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Dont allow |new| for built-ins methods and Function.prototype (1.03 KB, patch)
2010-10-26 08:23 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
proposed fix (3.47 KB, patch)
2010-10-26 14:35 PDT, Brendan Eich [:brendan]
igor: review+
Details | Diff | Splinter Review
my idea (2.91 KB, patch)
2010-10-27 03:09 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
my idea 2 (3.12 KB, patch)
2010-10-27 03:33 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
proposed fix, v2 (8.09 KB, patch)
2010-10-29 16:34 PDT, Brendan Eich [:brendan]
igor: review+
luke: review+
Details | Diff | Splinter Review
v1 2011 (16.73 KB, patch)
2011-03-09 07:25 PST, Tom Schuster [:evilpie]
brendan: review+
Details | Diff | Splinter Review
v2 - 2011 (18.73 KB, patch)
2011-03-10 08:14 PST, Tom Schuster [:evilpie]
jorendorff: review+
dvander: review+
Details | Diff | Splinter Review
v3 (19.29 KB, patch)
2011-04-15 06:56 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v4 (18.14 KB, patch)
2011-04-15 13:20 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v5 Dissallow new on objects without [[Construct]] and with [[Call]] (18.13 KB, patch)
2011-04-25 13:52 PDT, Tom Schuster [:evilpie]
jorendorff: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2003-04-14 14:41:31 PDT
From jseng newsgroup:

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

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 2003-04-14 15:38:27 PDT
Reassigning; cc'ing Brendan, Waldemar -
Comment 2 Phil Schwartau 2003-04-14 16:43:30 PDT
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] 2003-04-14 21:02:25 PDT
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 2003-04-15 17:42:59 PDT
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 2003-04-15 20:35:36 PDT
Created attachment 120670 [details]
HTML testcase
Comment 6 Phil Schwartau 2003-04-15 20:38:14 PDT
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 2008-04-27 22:10:05 PDT
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] 2008-04-27 22:41:33 PDT
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 2008-04-27 23:58:09 PDT
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] (remove +bmo to email) 2009-09-04 16:21:37 PDT
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] 2010-03-11 12:33:11 PST
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] 2010-03-12 10:29:35 PST
Created attachment 432176 [details] [diff] [review]
v1
Comment 13 Jason Orendorff [:jorendorff] 2010-03-12 12:30:10 PST
Created attachment 432205 [details] [diff] [review]
v2 - new comment, use B for built-in instead of S for stdlib
Comment 14 Igor Bukanov 2010-03-12 22:04:11 PST
(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] (remove +bmo to email) 2010-03-13 22:00:48 PST
(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] 2010-03-14 03:43:42 PDT
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 2010-03-14 05:16:49 PDT
(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] 2010-03-23 13:17:06 PDT
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] 2010-03-23 15:18:22 PDT
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] 2010-03-24 12:51:39 PDT
It looks like nothing breaks. I'll make the change suggested in comment 14.
Comment 21 Brendan Eich [:brendan] 2010-08-29 21:06:47 PDT
Slow natives going away, time to fix this spec-conformance issue as proposed?

/be
Comment 22 Brendan Eich [:brendan] 2010-10-04 02:21:15 PDT
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] 2010-10-20 14:14:27 PDT
Untaking.
Comment 24 Tom Schuster [:evilpie] 2010-10-26 08:23:00 PDT
Created attachment 486061 [details] [diff] [review]
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 2010-10-26 10:42:11 PDT
(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] (remove +bmo to email) 2010-10-26 10:48:51 PDT
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 Schuster [:evilpie] 2010-10-26 11:03:06 PDT
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] (remove +bmo to email) 2010-10-26 11:13:17 PDT
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 2010-10-26 12:05:33 PDT
(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 2010-10-26 12:08:07 PDT
(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] 2010-10-26 12:39:50 PDT
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 Schuster [:evilpie] 2010-10-26 12:54:33 PDT
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] 2010-10-26 13:57:08 PDT
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] 2010-10-26 13:59:01 PDT
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] 2010-10-26 14:35:18 PDT
Created attachment 486179 [details] [diff] [review]
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] 2010-10-26 14:44:16 PDT
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] 2010-10-26 17:33:51 PDT
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 Schuster [:evilpie] 2010-10-27 02:49:27 PDT
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 Schuster [:evilpie] 2010-10-27 03:09:18 PDT
Created attachment 486314 [details] [diff] [review]
my idea
Comment 40 Tom Schuster [:evilpie] 2010-10-27 03:33:20 PDT
Created attachment 486316 [details] [diff] [review]
my idea 2
Comment 41 Igor Bukanov 2010-10-27 04:18:06 PDT
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] 2010-10-29 12:00:50 PDT
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] 2010-10-29 12:17:47 PDT
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 Schuster [:evilpie] 2010-10-29 13:11:01 PDT
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] 2010-10-29 15:14:47 PDT
(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] 2010-10-29 15:24:09 PDT
(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] 2010-10-29 15:35:34 PDT
Filed as bug 608421.
Comment 48 Brendan Eich [:brendan] 2010-10-29 16:32:50 PDT
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] 2010-10-29 16:34:03 PDT
Created attachment 487089 [details] [diff] [review]
proposed fix, v2
Comment 50 Brendan Eich [:brendan] 2010-10-29 16:40:42 PDT
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] 2010-10-29 22:42:27 PDT
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] 2010-10-30 09:54:35 PDT
(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] 2010-10-31 18:14:46 PDT
(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] 2010-11-18 10:31:13 PST
Care to land?
Comment 55 Brendan Eich [:brendan] 2010-11-18 18:02:20 PST
This needs rebasing and more testing. I was going to universalize JSFUN_CONSTRUCTOR as proposed, too.

/be
Comment 56 Jeff Walden [:Waldo] (remove +bmo to email) 2010-12-25 09:40:58 PST
The patch here doesn't seem to touch the tracer at all, which seems presumptively wrong.
Comment 57 Andrew Drake [:adrake] 2010-12-27 11:35:16 PST
*** Bug 621420 has been marked as a duplicate of this bug. ***
Comment 58 Gary Kwong [:gkw] [:nth10sd] 2011-01-18 04:01:20 PST
(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] 2011-01-18 11:59:48 PST
(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] 2011-01-20 19:50:08 PST
There's more to do, and I don't recommend we delay Firefox 4 for this bug.

/be
Comment 61 Brendan Eich [:brendan] 2011-02-01 18:49:46 PST
I'll get this for the next major release.

/be
Comment 62 Tom Schuster [:evilpie] 2011-03-09 07:25:18 PST
Created attachment 518058 [details] [diff] [review]
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] 2011-03-09 20:23:51 PST
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 Schuster [:evilpie] 2011-03-10 08:14:36 PST
Created attachment 518390 [details] [diff] [review]
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 65 Jeff Walden [:Waldo] (remove +bmo to email) 2011-03-14 12:41:31 PDT
*** Bug 641606 has been marked as a duplicate of this bug. ***
Comment 66 David Anderson [:dvander] 2011-03-16 11:30:59 PDT
Comment on attachment 518390 [details] [diff] [review]
v2 - 2011

r=me on methodjit changes
Comment 67 Tom Schuster [:evilpie] 2011-03-17 13:42:07 PDT
Field Bug 642576 for the XXX. And Bug 642579 on the error message.
Comment 68 Tom Schuster [:evilpie] 2011-03-24 06:45:41 PDT
Review ping
Comment 69 Jason Orendorff [:jorendorff] 2011-04-12 17:48:06 PDT
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] 2011-04-12 21:41:25 PDT
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] 2011-04-13 18:25:19 PDT
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] 2011-04-14 12:46:25 PDT
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 Schuster [:evilpie] 2011-04-15 06:56:18 PDT
Created attachment 526240 [details] [diff] [review]
v3
Comment 74 Tom Schuster [:evilpie] 2011-04-15 13:20:45 PDT
Created attachment 526354 [details] [diff] [review]
v4

I suck at hg, hopefully this works.
Comment 75 Jason Orendorff [:jorendorff] 2011-04-20 06:39:38 PDT
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 Schuster [:evilpie] 2011-04-20 06:50:35 PDT
Fixed that locally, but forgot to actually attach the patch here again...
Comment 77 Jason Orendorff [:jorendorff] 2011-04-20 10:55:34 PDT
No worries. Running the js tests now. I will push it if these pass.
Comment 78 Gary Kwong [:gkw] [:nth10sd] 2011-04-21 02:45:27 PDT
(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 Schuster [:evilpie] 2011-04-21 02:49:42 PDT
Got backed-out, we need to fix something in NewXMLHttpRequest and NewChromeWorker.
Comment 80 Jason Orendorff [:jorendorff] 2011-04-25 12:39:02 PDT
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 Schuster [:evilpie] 2011-04-25 13:52:42 PDT
Created attachment 528167 [details] [diff] [review]
v5 Dissallow new on objects without [[Construct]] and with [[Call]]
Comment 82 Jason Orendorff [:jorendorff] 2011-04-25 13:59:08 PDT
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 Schuster [:evilpie] 2011-04-25 15:45:36 PDT
https://hg.mozilla.org/tracemonkey/rev/91c167c210fe

Phew, first own commit.
Comment 84 Paul Wright 2011-04-25 15:52:37 PDT
Congrats Tom!
Comment 85 Chris Leary [:cdleary] (not checking bugmail) 2011-04-26 15:11:09 PDT
http://hg.mozilla.org/mozilla-central/rev/91c167c210fe
Comment 86 David Bruant 2011-04-26 15:42:21 PDT
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] 2011-04-26 19:24:16 PDT
Is this on Mozilla 6 or Mozilla 5?
Comment 88 Jason Orendorff [:jorendorff] 2011-04-27 09:37:04 PDT
(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 Schuster [:evilpie] 2011-04-27 09:43:15 PDT
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] 2011-04-28 07:08:41 PDT
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 2011-07-28 05:13:37 PDT
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.

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