Closed Bug 445319 Opened 17 years ago Closed 15 years ago

built in functions should not have a prototype property

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: andrew_oakley2002, Assigned: brendan)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Fixing this bug should also fix the bug 202019.
Blocks: 202019
Need an owner... /be
Flags: wanted1.9.2?
Assignee: general → jim
Flags: wanted1.9.2? → wanted1.9.2+
Cc'ing Hixie for the DOM function question (do they have .prototype properties?). /be
Patching to fix fuzzer bug 600137. /be
Assignee: jimb → brendan
Blocks: 600137
Severity: normal → critical
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla2.0b7
Attached patch proposed fix (obsolete) — Splinter Review
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
Attachment #480570 - Flags: review?(jwalden+bmo)
Passes trace-test (why was the python harness renamed trace_test.py?) and tests /be
Function.prototype is a non-native function but it has no .prototype in ECMA-262. /be
Attachment #480570 - Attachment is obsolete: true
Attachment #480581 - Flags: review?(jwalden+bmo)
Attachment #480570 - Flags: review?(jwalden+bmo)
(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 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.
Attachment #480581 - Flags: review?(jwalden+bmo) → review+
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.
(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
(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.
(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
http://hg.mozilla.org/tracemonkey/rev/fd97ec71b03e (test fix to cope with loss of native.prototype) /be
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 630865
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: