608421 - disallow new on non-function [[Call]]-able objects without [[Construct]]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

For the reasons in bug 202019 comment 46, we should not allow this.
As a bonus, it will make InvokeConstructor shorter/cleaner.

Comment 1

[Tom S [:evilpie]]

Attachment: Wip1

After all these bug related to Function.prototype, and because i am a bit concerned about our Sputnik score because of this, i tried this approach. 

Somehow it doesn't involve much code change, mostly deleting :). I was able to make the methodjit happy with only on line, please somebody check me on this.

I didn't include test or the dom/xpc stuff brendan included in his patch, yet.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 499738 [details] [diff] [review]
Wip1

It'd be helpful if you added this to your ~/.hgrc:

# Use git diffs for binary diffing, emulate -p
[diff]
git = 1
showfunc = true

I can make educated guesses about where some of these modifications lie, but without that info it's somewhat harder to use Bugzilla's diff view.

*Assuming* I'm making those guesses correctly, we're making Function.prototype non-constructable.  Does the spec really say to do this?  It's not clear to me that it says anything one way or the other, really (a bit sad, really).

Comment 3

[Tom S [:evilpie]]

Attachment: WIP1 better diff

Thanks fixed my diff settings.

>*Assuming* I'm making those guesses correctly, we're making Function.prototype
>non-constructable.  Does the spec really say to do this?  It's not clear to me
>that it says anything one way or the other, really (a bit sad, really).
The section of interest is 15.3.4, which says "when invoked, accepts any arguments and returns undefined", but says nothing about what would happen if used in constructor context. And we probably don't want to extend the spec by allowing undefined behavior. Chrome, Opera and IE (don't have Safari) do likewise.

Comment 4

[Tom S [:evilpie]]

Is this patch wanted for Firefox 4.0? I think it's rather low risk, because when something went wrong we would relay on the old behavior. And this kind of absurd |new| usages should be nearly to not existing.

Comment 5

[Jeff Walden [:Waldo]]

It seems reasonable to me to get it.  Whether someone can review it with sufficiently little hassle, however, isn't entirely clear to me, and that affects the calculus somewhat.  Brendan's probably best since he did the JSFUN_CONSTRUCTOR stuff (?), I think, and can probably review it with the least effort.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to comment #3)
> >*Assuming* I'm making those guesses correctly, we're making Function.prototype
> >non-constructable.  Does the spec really say to do this?  It's not clear to me
> >that it says anything one way or the other, really (a bit sad, really).
> The section of interest is 15.3.4, which says "when invoked, accepts any
> arguments and returns undefined", but says nothing about what would happen if
> used in constructor context. And we probably don't want to extend the spec by
> allowing undefined behavior. Chrome, Opera and IE (don't have Safari) do
> likewise.

Actually, the sentence of note is this paragraph at the start of chapter 15, just pointed out to me by heycam in a semi-unrelated context:

"None of the built-in functions described in this clause that are not constructors shall implement the [[Construct]] internal method unless otherwise specified in the description of a particular function. None of the built-in functions described in this clause shall have a prototype property unless otherwise specified in the description of a particular function."

But in any case, we are in agreement: new Function.prototype should throw a TypeError.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> It seems reasonable to me to get it.  Whether someone can review it with
> sufficiently little hassle, however, isn't entirely clear to me, and that
> affects the calculus somewhat.  Brendan's probably best since he did the
> JSFUN_CONSTRUCTOR stuff (?),

That was Luke in bug 581263. I was going to generalize JSFUN_CONSTRUCTOR in bug 202019 (which blocks this bug) but I haven't gotten back to it.

Tom, stick a new patch with better diff options as Waldo suggested in this bug and feedback?brendan it. Thanks,

/be

Comment 8

[Tom S [:evilpie]]

The better patch is in
Comment 3.

Comment 9

[Brendan Eich [:brendan]]

How is this different from bug 202019?

/be

Comment 10

[Luke Wagner [:luke]]

See bug 202019 comment 46.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 499776 [details] [diff] [review]
WIP1 better diff

I made this block on bug 202019 since the current patch overlaps and conflicts with the one in bug 202019.  It would probably be better to build this one on top of that one.

>@@ -2766,6 +2767,7 @@ js_NewFunction(JSContext *cx, JSObject *
>     if ((flags & JSFUN_KINDMASK) >= JSFUN_INTERPRETED) {
>         JS_ASSERT(!native);
>         JS_ASSERT(nargs == 0);
>+        fun->flags |= JSFUN_CONSTRUCTOR;

So every interpreted function is a constructor?  That doesn't seem right.  I think you are setting this because of this change:

>@@ -1252,43 +1252,25 @@ InvokeConstructor(JSContext *cx, const C
>     JSFunction *fun = NULL;
>     if (clasp == &js_FunctionClass) {
>         fun = callee->getFunctionPrivate();
>-        if (fun->isConstructor()) {
>+        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());
>         }

which also doesn't seem right.  After bug 202019, this bug should mostly just be adding the "else" to InvokeConstructor (which you did) and removing code (which you have also done; good job removing junk from the jits).

Also, you can remove the

  JS_ASSERT_IF(flags & JSINVOKE_CONSTRUCT, !clasp->construct);

from inside the 'if' in Invoke() and instead put a modified assert before the 'if'

  JS_ASSERT_IF(flags & JSINVOKE_CONSTRUCT, callee.getFunctionPrivate()->isInterpreted());

Comment 12

[Tom S [:evilpie]]

>So every interpreted function is a constructor?  That doesn't seem right.  I
>think you are setting this because of this change:

No i don't, i think Brendan proposed this way in 202019, second we need this, because currently Function.prototype is scripted, but not a constructor. If brendan finishes the other bug, i would be happy to rebase on that.

Comment 13

[Tom S [:evilpie]]

Did this in bug 202019.

Comment 14

[Luke Wagner [:luke]]

Nice job!