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

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: till, Assigned: Norbert Lindenberg)

Tracking

(Blocks: 2 bugs)

Other Branch
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
As mentioned in bug 462300 comment 110, this is required for specification conformance.
(Assignee)

Updated

5 years ago
Blocks: 769872
Whiteboard: [js:t]
(Assignee)

Updated

5 years ago
(Assignee)

Comment 1

5 years ago
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.
Attachment #660619 - Flags: review?(tschneidereit)
(Reporter)

Comment 2

5 years ago
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())
Attachment #660619 - Flags: review?(tschneidereit)
(Assignee)

Comment 3

5 years ago
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.
(Reporter)

Comment 4

5 years ago
(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.
(Assignee)

Comment 5

5 years ago
Created attachment 660914 [details] [diff] [review]
proposed patch, v2

Updated as discussed above.
Attachment #660619 - Attachment is obsolete: true
Attachment #660914 - Flags: review?(tschneidereit)
(Reporter)

Updated

5 years ago
Attachment #660914 - Flags: review?(tschneidereit) → review+
(Reporter)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e68f92a96b6
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/9e68f92a96b6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Updated

4 years ago
Blocks: 837963
(Assignee)

Updated

4 years ago
No longer blocks: 769872
You need to log in before you can comment on or make changes to this bug.