1169736 - Temporarily disable arrow functions and eval inside derived class constructors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

This simplifies getting super() off the ground tremendously.

Comment 1

[Eric Faust [:efaust]]

Attachment: Patch

We don't need to outlaw in indirect eval, because the spec says we will take the globalThis, which will always be initialized.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8615155 [details] [diff] [review]
Patch

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

Please also test that Debugger.Frame.eval is inhibited.

::: js/src/frontend/Parser.cpp
@@ +8321,5 @@
>  template <typename ParseHandler>
>  typename ParseHandler::Node
>  Parser<ParseHandler>::newPropertyListNode(PropListType type)
>  {
> +    if (type >= ClassBody)

I'm ok with either (type == ClassBody || type == DerivedClassBody) or IsClassBody(type) in all these places. `>= ClassBody` is a bit much.

@@ +8575,5 @@
>  
>                  if (!handler.addShorthand(propList, propname, nameExpr))
>                      return null();
>              } else if (tt == TOK_LP) {
> +                tokenStream.ungetToken();;

extra semicolon

::: js/src/js.msg
@@ +102,5 @@
>  MSG_DEF(JSMSG_INVALID_ARG_TYPE,        3, JSEXN_TYPEERR, "Invalid type: {0} can't be a{1} {2}")
>  MSG_DEF(JSMSG_TERMINATED,              1, JSEXN_ERR, "Script terminated by timeout at:\n{0}")
>  MSG_DEF(JSMSG_PROTO_NOT_OBJORNULL,     1, JSEXN_TYPEERR, "{0}.prototype is not an object or null")
>  MSG_DEF(JSMSG_CANT_CALL_CLASS_CONSTRUCTOR, 0, JSEXN_TYPEERR, "class constructors must be invoked with |new|")
> +MSG_DEF(JSMSG_DISABLED_DERIVED_CLASS,  1, JSEXN_TYPEERR, "{0} temporarily disallowed in derived class constructors")

InternalError is better for this kind of implementation restriction.

It seems the argument is always "direct eval", so might as well hard-code it and change the number of arguments here from 1 to 0.

Comment 3

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #2)
>
> Please also test that Debugger.Frame.eval is inhibited.
>

Great point.
 
> ::: js/src/frontend/Parser.cpp
> @@ +8321,5 @@
> >  template <typename ParseHandler>
> >  typename ParseHandler::Node
> >  Parser<ParseHandler>::newPropertyListNode(PropListType type)
> >  {
> > +    if (type >= ClassBody)
> 
> I'm ok with either (type == ClassBody || type == DerivedClassBody) or
> IsClassBody(type) in all these places. `>= ClassBody` is a bit much.
> 

Can't really blame you for that. I opted for IsClassBody().
> @@ +8575,5 @@
> >  
> >                  if (!handler.addShorthand(propList, propname, nameExpr))
> >                      return null();
> >              } else if (tt == TOK_LP) {
> > +                tokenStream.ungetToken();;
> 
> extra semicolon
> 
> ::: js/src/js.msg
> @@ +102,5 @@
> >  MSG_DEF(JSMSG_INVALID_ARG_TYPE,        3, JSEXN_TYPEERR, "Invalid type: {0} can't be a{1} {2}")
> >  MSG_DEF(JSMSG_TERMINATED,              1, JSEXN_ERR, "Script terminated by timeout at:\n{0}")
> >  MSG_DEF(JSMSG_PROTO_NOT_OBJORNULL,     1, JSEXN_TYPEERR, "{0}.prototype is not an object or null")
> >  MSG_DEF(JSMSG_CANT_CALL_CLASS_CONSTRUCTOR, 0, JSEXN_TYPEERR, "class constructors must be invoked with |new|")
> > +MSG_DEF(JSMSG_DISABLED_DERIVED_CLASS,  1, JSEXN_TYPEERR, "{0} temporarily disallowed in derived class constructors")
> 
> InternalError is better for this kind of implementation restriction.
> 
> It seems the argument is always "direct eval", so might as well hard-code it
> and change the number of arguments here from 1 to 0.

InternalError is fine by me. I will keep the arg, though, as it is used. "arrow functions" are disallowed from the parser.

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5ab792827a

Comment 5

[Eric Faust [:efaust]]

Attachment: Followup fix

Clean up the js1_8_5/reflect-parse/classes.js errors that cropped up before the tree closing.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Landed the followup in https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7188e6713c

Comment 7

[Pulsebot]

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

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe9781e4953b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fe9781e4953b