445319 - built in functions should not have a prototype property

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andrew Oakley]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:1.8.1.12) Gecko/20080306 Firefox/2.0.0.12
Build Identifier: 

The beginning of section 15 in ECMA-262 states that "None of the built-in functions described in this section shall initially have a prototype property unless otherwise specified...".  

Firefox gives all the standard functions prototype objects, however other web browsers do not.  

This problem also applies to DOM functions, although that doesn't seem to be specified anywhere.  

Reproducible: Always

Steps to Reproduce:
alert(eval.prototype);
alert(document.getElementById.prototype);
Actual Results:  
"[object Object]" for both tests

Expected Results:  
"undefined" for both tests

Comment 1

[Igor Bukanov]

Fixing this bug should also fix the bug 202019.

Comment 2

[Brendan Eich [:brendan]]

Need an owner...

/be

Comment 3

[Brendan Eich [:brendan]]

Cc'ing Hixie for the DOM function question (do they have .prototype properties?).

/be

Comment 4

[Brendan Eich [:brendan]]

Patching to fix fuzzer bug 600137.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: proposed fix

Some too-local patchwork split the classPrototypeAtom id case.

JSID_IS_ATOM(id, atom) relieves this code of having to use an atom local, which was single-use anyway once unnecessary ATOM_TO_JSID(atom) calls were replaced with id (since JSID_IS_ATOM(id, atom) guarded all such calls).

All return statements in fun_resolve use bool literals now.

/be

Comment 6

[Brendan Eich [:brendan]]

Passes trace-test (why was the python harness renamed trace_test.py?) and tests

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2 (with tests)

Function.prototype is a non-native function but it has no .prototype in ECMA-262.

/be

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> Passes trace-test (why was the python harness renamed trace_test.py?) and tests

Fixed an importability issue in Python on some esoteric system, Android I think.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 480581 [details] [diff] [review]
proposed fix, v2 (with tests)

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp

>     if (!JSID_IS_ATOM(id))
>-        return JS_TRUE;
>+        return true;

I would have preferred if the true/false changes (and id changes) had been in a separate patch.  They're not negligible, and they pretty massively bloat the substantive changes in this patch and make it harder to read.


>+    if (JSID_IS_ATOM(id, cx->runtime->atomState.classPrototypeAtom)) {
>+        /*
>+         * Native or "built-in" functions do not have a .prototype property per
>+         * ECMA-262 (all editions). Built-in constructor functions, e.g. Object
>+         * and Function to name two conspicuous examples, do have a .prototype
>+         * property, but it is created eagerly by js_InitClass (jsobj.cpp).
>+         *
>+         * The empty interpreted function object at Function.prototype must not
>+         * have a .prototype property.
>+         */
>+        if (fun->isNative() || fun->isFunctionPrototype())
>+            return true;

Bound functions, as I recall, are also native functions, so note that this catches bound functions, as requested later.

Perhaps cite where it's stated that Function.prototype doesn't have a prototype property, since you say it and it's not an inherently obvious part of ECMAScript semantics.


>         /* ES5 15.3.4.5: bound functions don't have a prototype property. */
>         if (obj->isBoundFunction())
>-            return JS_TRUE;
>+            return true;

Since native is tested earlier, and bound implies native, this can never be hit -- change it to an assertion, and move the bound citation as noted earlier.


>     JSFunction *fun = js_NewFunction(cx, proto, NULL, 0, JSFUN_INTERPRETED, obj, NULL);
>     if (!fun)
>         return NULL;
>+    fun->flags |= JSFUN_PROTOTYPE;
>     fun->u.i.script = JSScript::emptyScript();

This is kind of hackish, but it works, we have the bits, and it's not really worth agonizing over.


>diff --git a/js/src/tests/ecma/FunctionObjects/15.js b/js/src/tests/ecma/FunctionObjects/15.js

Please put this in ecma_5/Function (or some other ecma_5/ directory, this one just seems like the best one going from memory) and lose the version-checking cruftiness.

>+for (var i in Math) {
>+    var m = Math[i];
>+    if (typeof m === "function")
>+        assertEq(m.prototype, undefined);
>+}

Aren't these properties mostly (completely?) non-enumerable?  Don't you need Object.getOwnPropertyNames to get the property names (further lending credence to the test-move)?


>diff --git a/js/src/tests/js1_8_5/regress/jstests.list b/js/src/tests/js1_8_5/regress/jstests.list
>diff --git a/js/src/tests/js1_8_5/regress/regress-600137.js b/js/src/tests/js1_8_5/regress/regress-600137.js

Extraneous, intended for a different bug, please remove.

Comment 10

[Jeff Walden [:Waldo]]

Oops, forgot: doesn't your test fail to do anything to check Function.prototype.prototype's non-presence?  Please add some checks regarding this matter.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #9)
> Perhaps cite where it's stated that Function.prototype doesn't have a prototype
> property, since you say it and it's not an inherently obvious part of
> ECMAScript semantics.

It is not obvious from ECMA-262 (any edition). You have to read the lack of an explicit .prototype (and [[Construct]]) for Function.prototype's spec (15.3.4 in ES5) and keep the clause 15 intro words in mind. Will cite as best I can.

> >         /* ES5 15.3.4.5: bound functions don't have a prototype property. */
> >         if (obj->isBoundFunction())
> >-            return JS_TRUE;
> >+            return true;
> 
> Since native is tested earlier, and bound implies native, this can never be hit
> -- change it to an assertion, and move the bound citation as noted earlier.

Righto.

> >     JSFunction *fun = js_NewFunction(cx, proto, NULL, 0, JSFUN_INTERPRETED, obj, NULL);
> >     if (!fun)
> >         return NULL;
> >+    fun->flags |= JSFUN_PROTOTYPE;
> >     fun->u.i.script = JSScript::emptyScript();
> 
> This is kind of hackish, but it works, we have the bits, and it's not really
> worth agonizing over.

Got a better idea? I do not.

> >diff --git a/js/src/tests/ecma/FunctionObjects/15.js b/js/src/tests/ecma/FunctionObjects/15.js
> 
> Please put this in ecma_5/Function (or some other ecma_5/ directory, this one
> just seems like the best one going from memory) and lose the version-checking
> cruftiness.

Ok, I was a bit queasy about putting it in ecma, although the semantics go back to ES1. Will do.

> >+for (var i in Math) {
> >+    var m = Math[i];
> >+    if (typeof m === "function")
> >+        assertEq(m.prototype, undefined);
> >+}
> 
> Aren't these properties mostly (completely?) non-enumerable?  Don't you need
> Object.getOwnPropertyNames to get the property names (further lending credence
> to the test-move)?

Oops, thanks! Fixing.

> >diff --git a/js/src/tests/js1_8_5/regress/jstests.list b/js/src/tests/js1_8_5/regress/jstests.list
> >diff --git a/js/src/tests/js1_8_5/regress/regress-600137.js b/js/src/tests/js1_8_5/regress/regress-600137.js
> 
> Extraneous, intended for a different bug, please remove.

No, it's for this bug, see the dependency.
(In reply to comment #10)
> Oops, forgot: doesn't your test fail to do anything to check
> Function.prototype.prototype's non-presence?  Please add some checks regarding
> this matter.

That's the bit that I mentioned being covered implicitly:

    assertEq(builtin_funcs[i].prototype, undefined);

A function object delegates to Function.prototype. These builtins must not have a direct ("own") .prototype, but this assertEq does a get on .prototype so it will look up the prototype chain. If Function.prototype had a .prototype, this assertEq test loop would botch on the first iteration.

/be

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> Will cite as best I can.

Sounds good.


> Got a better idea? I do not.

None.


> although the semantics go back to ES1. Will do.

My memory is that ES3 semantics were vague, or at least *permitted* .prototype on built-ins.  Only with ES5 was behavior precisely specified to make having such a property an unambiguous bug.


> > >diff --git a/js/src/tests/js1_8_5/regress/jstests.list b/js/src/tests/js1_8_5/regress/jstests.list
> > >diff --git a/js/src/tests/js1_8_5/regress/regress-600137.js b/js/src/tests/js1_8_5/regress/regress-600137.js
> > 
> > Extraneous, intended for a different bug, please remove.
> 
> No, it's for this bug, see the dependency.

Ah.  I'd prefer that be separated into its own changeset, with a commit message identifying that bug -- can push them simultaneously, of course.


> That's the bit that I mentioned being covered implicitly:
> 
>     assertEq(builtin_funcs[i].prototype, undefined);
> 
> A function object delegates to Function.prototype. These builtins must not have
> a direct ("own") .prototype, but this assertEq does a get on .prototype so it
> will look up the prototype chain. If Function.prototype had a .prototype, this
> assertEq test loop would botch on the first iteration.

Oy vey, that's implicit.  EIBTI, and this misses checking |Function.prototype.hasOwnProperty("prototype")| as well, so please still add more-obvious Function.prototype checking.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> > although the semantics go back to ES1. Will do.
> 
> My memory is that ES3 semantics were vague, or at least *permitted* .prototype
> on built-ins.

No, ES1 was clear enough. Took a while for anyone to notice and file this bug!

/be

Comment 14

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/caa8bfd6f9b1
http://hg.mozilla.org/tracemonkey/rev/812a37500dda (test from bug 600137)

/be

Comment 15

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/fd97ec71b03e (test fix to cope with loss of native.prototype)

/be

Comment 16

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/2a3ac1acaa99 (another old test, browser-only and not obviously useful now; I made it object-detect harder so it runs as before on older browsers).

/be

Comment 17

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/6ecaaddfc56e (JSOPTION_ANONFUNFIX, grrr!)

/be

Comment 18

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/caa8bfd6f9b1