784300 - Make self-hosted non-constructor functions not have a prototype

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

As mentioned in bug 462300 comment 110, this is required for specification conformance.

Comment 1

[Norbert Lindenberg]

Attachment: 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]]

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]

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]]

(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]

Attachment: proposed patch, v2

Updated as discussed above.

Comment 6

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e68f92a96b6

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e68f92a96b6