The default bug view has changed. See this FAQ.

Built-in functions should not define constructor automatically

VERIFIED FIXED in mozilla6

Status

()

Core
JavaScript Engine
P2
normal
VERIFIED FIXED
14 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: evilpie)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla6
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 attachments, 10 obsolete attachments)

264 bytes, text/html
Details
8.09 KB, patch
Igor Bukanov
: review+
luke
: review+
Details | Diff | Splinter Review
18.13 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
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

14 years ago
Reassigning; cc'ing Brendan, Waldemar -
Assignee: rogerl → khanson

Comment 2

14 years ago
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();
({})
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
(Reporter)

Comment 4

14 years ago
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

14 years ago
Created attachment 120670 [details]
HTML testcase

Comment 6

14 years ago
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"

Updated

11 years ago
Assignee: khanson → general
QA Contact: pschwartau → general

Comment 7

9 years ago
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.
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

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

Updated

9 years ago
Depends on: 445319
(Reporter)

Updated

9 years ago
Summary: Built-in functions should not define constructor automatically? → Built-in functions should not define constructor automatically
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.
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. :)
Created attachment 432176 [details] [diff] [review]
v1
Attachment #432176 - Flags: review?(igor)
Created attachment 432205 [details] [diff] [review]
v2 - new comment, use B for built-in instead of S for stdlib
Assignee: general → jorendorff
Attachment #432176 - Attachment is obsolete: true
Attachment #432205 - Flags: review?(igor)
Attachment #432176 - Flags: review?(igor)
(Reporter)

Comment 14

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

Comment 17

7 years ago
(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.
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.
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
It looks like nothing breaks. I'll make the change suggested in comment 14.
Slow natives going away, time to fix this spec-conformance issue as proposed?

/be
blocking2.0: --- → ?

Updated

7 years ago
blocking2.0: ? → -
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
Attachment #432205 - Flags: review?(igor)
Untaking.
Assignee: jorendorff → general
(Assignee)

Comment 24

7 years ago
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.
Attachment #486061 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #486061 - Flags: review? → review?(igor)
(Reporter)

Comment 25

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

Comment 27

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

Comment 29

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

Comment 30

7 years ago
(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 :)
(Assignee)

Updated

7 years ago
Attachment #486061 - Flags: review?(igor)
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
Assignee: general → brendan
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla2.0
(Assignee)

Comment 32

7 years ago
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.
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
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
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
Attachment #486061 - Attachment is obsolete: true
Attachment #486179 - Flags: review?(igor)
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
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
(Assignee)

Comment 38

7 years ago
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
(Assignee)

Comment 39

7 years ago
Created attachment 486314 [details] [diff] [review]
my idea
(Assignee)

Comment 40

7 years ago
Created attachment 486316 [details] [diff] [review]
my idea 2
Attachment #486314 - Attachment is obsolete: true
(Reporter)

Updated

7 years ago
Attachment #486316 - Attachment is patch: true
Attachment #486316 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 41

7 years ago
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
Attachment #486179 - Flags: review?(igor) → review+

Updated

7 years ago
Blocks: 608311
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

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

Comment 44

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

Comment 45

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

7 years ago
Filed as bug 608421.
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
Keywords: dev-doc-needed
Created attachment 487089 [details] [diff] [review]
proposed fix, v2
Attachment #432205 - Attachment is obsolete: true
Attachment #486179 - Attachment is obsolete: true
Attachment #487089 - Flags: review?(igor)
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
Attachment #487089 - Flags: review?(lw)

Comment 51

7 years ago
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.
Attachment #487089 - Flags: review?(lw) → review+
(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
(Reporter)

Updated

7 years ago
Attachment #487089 - Flags: review?(igor) → review+

Comment 53

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

Updated

7 years ago
Blocks: 608421

Comment 54

6 years ago
Care to land?
This needs rebasing and more testing. I was going to universalize JSFUN_CONSTRUCTOR as proposed, too.

/be
(Assignee)

Updated

6 years ago
Blocks: 501227
The patch here doesn't seem to touch the tracer at all, which seems presumptively wrong.

Updated

6 years ago
Blocks: 621420

Updated

6 years ago
Duplicate of this bug: 621420
(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.
blocking2.0: - → ?
(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?
blocking2.0: ? → .x
There's more to do, and I don't recommend we delay Firefox 4 for this bug.

/be
I'll get this for the next major release.

/be
Target Milestone: mozilla2.0 → Future
Target Milestone: Future → ---
(Assignee)

Updated

6 years ago
Assignee: brendan → evilpies
(Assignee)

Comment 62

6 years ago
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.
Attachment #518058 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #518058 - Flags: review? → review?(brendan)
(Assignee)

Updated

6 years ago
Attachment #518058 - Flags: review?(dvander)
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
Attachment #518058 - Flags: review?(brendan) → review+
(Assignee)

Comment 64

6 years ago
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.
Attachment #486316 - Attachment is obsolete: true
Attachment #518058 - Attachment is obsolete: true
Attachment #518058 - Flags: review?(dvander)
(Assignee)

Updated

6 years ago
Attachment #518390 - Flags: review?(brendan)
(Assignee)

Updated

6 years ago
Attachment #518390 - Flags: review?(dvander)
Duplicate of this bug: 641606
Comment on attachment 518390 [details] [diff] [review]
v2 - 2011

r=me on methodjit changes
Attachment #518390 - Flags: review?(dvander) → review+
(Assignee)

Comment 67

6 years ago
Field Bug 642576 for the XXX. And Bug 642579 on the error message.
(Assignee)

Comment 68

6 years ago
Review ping
(Assignee)

Updated

6 years ago
Attachment #518390 - Flags: review?(brendan) → review?(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.
Attachment #518390 - Flags: review?(jorendorff) → review+

Comment 70

6 years ago
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?
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.
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.
(Assignee)

Comment 73

6 years ago
Created attachment 526240 [details] [diff] [review]
v3
Attachment #518390 - Attachment is obsolete: true
Attachment #526240 - Flags: review?(jorendorff)
(Assignee)

Comment 74

6 years ago
Created attachment 526354 [details] [diff] [review]
v4

I suck at hg, hopefully this works.
Attachment #526240 - Attachment is obsolete: true
Attachment #526240 - Flags: review?(jorendorff)
(Assignee)

Updated

6 years ago
Attachment #526354 - Flags: review?(jorendorff)
Attachment #526354 - Attachment is patch: true
Attachment #526354 - Attachment mime type: application/octet-stream → text/plain
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.
(Assignee)

Comment 76

6 years ago
Fixed that locally, but forgot to actually attach the patch here again...
No worries. Running the js tests now. I will push it if these pass.
(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?
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 79

6 years ago
Got backed-out, we need to fix something in NewXMLHttpRequest and NewChromeWorker.
Whiteboard: fixed-in-tracemonkey
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.
Attachment #526354 - Flags: review?(jorendorff)
(Assignee)

Comment 81

6 years ago
Created attachment 528167 [details] [diff] [review]
v5 Dissallow new on objects without [[Construct]] and with [[Call]]
Attachment #526354 - Attachment is obsolete: true
Attachment #528167 - Flags: review?(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.
Attachment #528167 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 83

6 years ago
https://hg.mozilla.org/tracemonkey/rev/91c167c210fe

Phew, first own commit.
Whiteboard: fixed-in-tracemonkey

Comment 84

6 years ago
Congrats Tom!
(Assignee)

Updated

6 years ago
Blocks: 652780
http://hg.mozilla.org/mozilla-central/rev/91c167c210fe
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 86

6 years ago
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.
Is this on Mozilla 6 or Mozilla 5?

Updated

6 years ago
Target Milestone: --- → mozilla6
(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.
(Assignee)

Comment 89

6 years ago
In practice most of the code using |new|, probably never really worked, so this unlikely to be an issue for most developers.
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
Keywords: dev-doc-needed → dev-doc-complete

Comment 91

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.