Last Comment Bug 608421 - disallow new on non-function [[Call]]-able objects without [[Construct]]
: disallow new on non-function [[Call]]-able objects without [[Construct]]
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
Depends on: 202019
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-29 15:35 PDT by Luke Wagner [:luke]
Modified: 2011-10-13 11:54 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Wip1 (6.65 KB, patch)
2010-12-25 15:12 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Review
WIP1 better diff (6.43 KB, patch)
2010-12-26 13:05 PST, Tom Schuster [:evilpie]
no flags Details | Diff | Review

Description Luke Wagner [:luke] 2010-10-29 15:35:19 PDT
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 Schuster [:evilpie] 2010-12-25 15:12:48 PST
Created attachment 499738 [details] [diff] [review]
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] (remove +bmo to email) 2010-12-25 15:35:53 PST
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 Schuster [:evilpie] 2010-12-26 13:05:57 PST
Created attachment 499776 [details] [diff] [review]
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 Schuster [:evilpie] 2010-12-30 10:20:45 PST
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] (remove +bmo to email) 2010-12-30 13:25:26 PST
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] (remove +bmo to email) 2010-12-30 13:49:52 PST
(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] 2010-12-30 19:14:19 PST
(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 Schuster [:evilpie] 2011-01-03 18:02:39 PST
The better patch is in
Comment 3.
Comment 9 Brendan Eich [:brendan] 2011-01-03 20:46:06 PST
How is this different from bug 202019?

/be
Comment 10 Luke Wagner [:luke] 2011-01-03 21:22:12 PST
See bug 202019 comment 46.
Comment 11 Luke Wagner [:luke] 2011-01-03 21:42:52 PST
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 Schuster [:evilpie] 2011-01-04 06:51:59 PST
>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 Schuster [:evilpie] 2011-04-26 02:28:56 PDT
Did this in bug 202019.
Comment 14 Luke Wagner [:luke] 2011-04-26 07:16:10 PDT
Nice job!

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