Last Comment Bug 784300 - Make self-hosted non-constructor functions not have a prototype
: Make self-hosted non-constructor functions not have a prototype
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla18
Assigned To: Norbert Lindenberg
:
Mentors:
http://ecma-international.org/ecma-26...
Depends on:
Blocks: 784288 es-intl
  Show dependency treegraph
 
Reported: 2012-08-21 04:12 PDT by Till Schneidereit [:till] (pto July 23 - July 31)
Modified: 2013-02-04 17:16 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (1.47 KB, patch)
2012-09-12 16:40 PDT, Norbert Lindenberg
no flags Details | Diff | Splinter Review
proposed patch, v2 (2.69 KB, patch)
2012-09-13 11:33 PDT, Norbert Lindenberg
till: review+
Details | Diff | Splinter Review

Description Till Schneidereit [:till] (pto July 23 - July 31) 2012-08-21 04:12:08 PDT
As mentioned in bug 462300 comment 110, this is required for specification conformance.
Comment 1 Norbert Lindenberg 2012-09-12 16:40:33 PDT
Created attachment 660619 [details] [diff] [review]
proposed patch

Tested with ECMAScript Internationalization API conformance test suite run against the WIP implementation of that API in bug 769872. Patch fixes all 9 failures caused by prototypes on non-constructor functions.
Comment 2 Till Schneidereit [:till] (pto July 23 - July 31) 2012-09-13 05:02:52 PDT
Comment on attachment 660619 [details] [diff] [review]
proposed patch

Review of attachment 660619 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good: simple and effective. I have two small request, so I reset the r? flag. Please add a new patch with the feedback addressed. I can then check that in for you.

::: js/src/jsfun.cpp
@@ +269,5 @@
>      if (JSID_IS_ATOM(id, cx->runtime->atomState.classPrototypeAtom)) {
>          /*
>           * Native or "built-in" functions do not have a .prototype property per
>           * ECMA-262, or (Object.prototype, Function.prototype, etc.) have that
> +         * property created eagerly. Self-hosted functions are also "built-in".

We're moving away from using the term "native", where possible. So this comment can be simplified to something along the lines of "Built-in functions do not have [...]".

@@ +278,5 @@
>           * ES5 15.3.4.5: bound functions don't have a prototype property. The
>           * isNative() test covers this case because bound functions are native
>           * functions by definition/construction.
>           */
> +        if (fun->isNative() || fun->isSelfHostedBuiltin() || fun->isFunctionPrototype())

This should check for isSelfHostedConstructor() being false, I think. Also, since you're using these predicates, maybe you can add a isBuiltin() predicate to JSFunction along the lines of "isNative() || isSelfHostedBuiltin()"?

This would then become
if ((fun->isBuiltin() && !fun->isSelfHostedConstructor()) || fun->isFunctionPrototype())
Comment 3 Norbert Lindenberg 2012-09-13 08:53:14 PDT
Checking isSelfHostedConstructor() seems unnecessary. fun_resolve is only called to access the prototype property of a function that doesn't have one yet, and for a constructor you'd create and populate the prototype eagerly, just as is mentioned for Object.prototype and friends in the comment.

Happy to make the other changes.
Comment 4 Till Schneidereit [:till] (pto July 23 - July 31) 2012-09-13 09:36:44 PDT
(In reply to Norbert Lindenberg from comment #3)
> Checking isSelfHostedConstructor() seems unnecessary. fun_resolve is only
> called to access the prototype property of a function that doesn't have one
> yet, and for a constructor you'd create and populate the prototype eagerly,
> just as is mentioned for Object.prototype and friends in the comment.

Ok, that makes sense.

> Happy to make the other changes.

Cool, thanks.
Comment 5 Norbert Lindenberg 2012-09-13 11:33:45 PDT
Created attachment 660914 [details] [diff] [review]
proposed patch, v2

Updated as discussed above.
Comment 6 Till Schneidereit [:till] (pto July 23 - July 31) 2012-09-14 01:05:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e68f92a96b6
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-09-14 18:19:47 PDT
https://hg.mozilla.org/mozilla-central/rev/9e68f92a96b6

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