Closed Bug 1169736 Opened 5 years ago Closed 5 years ago

Temporarily disable arrow functions and eval inside derived class constructors

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: efaust, Assigned: efaust)

References

Details

Attachments

(2 files)

This simplifies getting super() off the ground tremendously.
Attached patch PatchSplinter Review
We don't need to outlaw in indirect eval, because the spec says we will take the globalThis, which will always be initialized.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8615155 - Flags: review?(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.
Attachment #8615155 - Flags: review?(jorendorff) → review+
(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.
Attached patch Followup fixSplinter Review
Clean up the js1_8_5/reflect-parse/classes.js errors that cropped up before the tree closing.
https://hg.mozilla.org/mozilla-central/rev/fe9781e4953b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.