Bug 153864 – Invoking super()/super inside of the eval should not lead to SyntaxError

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[GSkachkov]

Following case should not lead to Syntax Error:
class A {
}
class B extends A {
  constructor() {
    eval("eval('super()')");
  }
}

new B();

[GSkachkov]

Following case should not lead to Syntax Error:

const defaultValue = 'default-value';

class A {
  setDefaultValue() {
      this.value = defaultValue;
  }
}

class B extends A {
  constructor() {
    eval("super()");
  }
  setValue () {
      eval("super.setDefaultValue()");
  }
}

let b = new B();
b.setValue();
b.id === defaultValue;

[GSkachkov]

Created attachment 273142 [details]
Patch

Patch, more test will be later

[Saam Barati]

Comment on attachment 273142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273142&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:307
> +            return !(m_codeBlock->isArrowFunction() || m_codeType == EvalCode) || m_isNewTargetLoadedInArrowFunction
>                  ? m_newTargetRegister : emitLoadNewTargetFromArrowFunctionLexicalEnvironment();

This function is getting hard to read. I vote for an if statement here.
Also, I found a bug in the logic of caching here:
https://bugs.webkit.org/show_bug.cgi?id=155153

> Source/JavaScriptCore/parser/Parser.cpp:2068
> +            ConstructorKind functionConstructorKind = functionBodyType == StandardFunctionBodyBlock && !m_isEvalContext

Don't we already know if we're parsing an eval?
Why do we need m_isEvalContext?
Maybe I'm mistaken.

> Source/JavaScriptCore/parser/Parser.cpp:3820
> +        semanticFailIfFalse(currentScope()->isFunction() || (m_isEvalContext && closestParentOrdinaryFunctionNonLexicalScope()->expectedSuperBinding() == SuperBinding::Needed), "super is only valid inside functions");

This error message should be updated.

> Source/JavaScriptCore/runtime/CodeCache.cpp:104
> +    bool isEvalConext = true;

This is definitely not true. This should probably be a parameter to this function.

> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:232
> +class K extends A {

let's have some eval tests inside a constructor.
tests for TDZ, etc.

[GSkachkov]

Created attachment 274093 [details]
Patch

[GSkachkov]

Comment on attachment 273142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273142&action=review

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:307
>>                  ? m_newTargetRegister : emitLoadNewTargetFromArrowFunctionLexicalEnvironment();
> 
> This function is getting hard to read. I vote for an if statement here.
> Also, I found a bug in the logic of caching here:
> https://bugs.webkit.org/show_bug.cgi?id=155153

OK. It will be overwritten in fix of the 155153

>> Source/JavaScriptCore/parser/Parser.cpp:2068
>> +            ConstructorKind functionConstructorKind = functionBodyType == StandardFunctionBodyBlock && !m_isEvalContext
> 
> Don't we already know if we're parsing an eval?
> Why do we need m_isEvalContext?
> Maybe I'm mistaken.

In new patch,  I've used isEvalNode template function that is deepest place where I can check EvalNode.

>> Source/JavaScriptCore/parser/Parser.cpp:3820
>> +        semanticFailIfFalse(currentScope()->isFunction() || (m_isEvalContext && closestParentOrdinaryFunctionNonLexicalScope()->expectedSuperBinding() == SuperBinding::Needed), "super is only valid inside functions");
> 
> This error message should be updated.

Yeah, I've created issue https://bugs.webkit.org/show_bug.cgi?id=155491. There is several tests that rely on this 'text', so I decided to separate this change is another patch.

>> Source/JavaScriptCore/runtime/CodeCache.cpp:104
>> +    bool isEvalConext = true;
> 
> This is definitely not true. This should probably be a parameter to this function.

Removed

>> Source/JavaScriptCore/tests/stress/arrowfunction-lexical-bind-superproperty.js:232
>> +class K extends A {
> 
> let's have some eval tests inside a constructor.
> tests for TDZ, etc.

Done

[Saam Barati]

Comment on attachment 274093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274093&action=review

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:643
> +    if (needsToUpdateArrowFunctionContext() && !codeBlock->isArrowFunctionContext() && !isDerivedConstructorContext()) {

Why the "!isDerivedConstructorContext()" check?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4145
> +    return m_scopeNode->doAnyInnerArrowFunctionsUseNewTarget() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();

I believe m_codeBlock->usesEval() should always be true if m_scopeNode->doAnyInnerAroowFunctionsUseEval().
You should double check though.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4150
> +    return m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperProperty() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();

ditto

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4155
> +    return m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();

ditto

> Source/JavaScriptCore/parser/Parser.cpp:3848
> +        // TODO: Change error message for more suitable. https://bugs.webkit.org/show_bug.cgi?id=155491 

Style: FIXME not TODO

[Saam Barati]

Comment on attachment 274093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274093&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Invoking super()/super inside of the eval should not lead to SyntaxError

Do we have a bug open for new.target inside eval?

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:643
>> +    if (needsToUpdateArrowFunctionContext() && !codeBlock->isArrowFunctionContext() && !isDerivedConstructorContext()) {
> 
> Why the "!isDerivedConstructorContext()" check?

Ah, I guess it's that way so we don't recreate a scope. I think I understand.

[GSkachkov]

Comment on attachment 274093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274093&action=review

>> Source/JavaScriptCore/ChangeLog:3
>> +        Invoking super()/super inside of the eval should not lead to SyntaxError
> 
> Do we have a bug open for new.target inside eval?

Yes, I did this recently
 https://bugs.webkit.org/show_bug.cgi?id=155545

>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:643
>>> +    if (needsToUpdateArrowFunctionContext() && !codeBlock->isArrowFunctionContext() && !isDerivedConstructorContext()) {
>> 
>> Why the "!isDerivedConstructorContext()" check?
> 
> Ah, I guess it's that way so we don't recreate a scope. I think I understand.

Yes, that is correct. It is for this case: 
class C {};
class D extends C {
  constructor() {
    eval("(()=>super())()");//Error
  }
}
new D();
Otherwise we will have two context scope for 'this'. I've spend last three evenings to find out why mention simple test case did not work

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4145
>> +    return m_scopeNode->doAnyInnerArrowFunctionsUseNewTarget() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();
> 
> I believe m_codeBlock->usesEval() should always be true if m_scopeNode->doAnyInnerAroowFunctionsUseEval().
> You should double check though.

It is true, but unfortunately this patch cover cases when we do not have arrow function, for instance from previous comments, so I need to check m_codeBlock->usesEval(). Possible we need to change name of the function isNewTargetUsedInInnerArrowFunction -> isNewTargetUsedInInnerArrowFunctionOrEval

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4150
>> +    return m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperProperty() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();
> 
> ditto

The same

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4155
>> +    return m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();
> 
> ditto

The same

>> Source/JavaScriptCore/parser/Parser.cpp:3848
>> +        // TODO: Change error message for more suitable. https://bugs.webkit.org/show_bug.cgi?id=155491 
> 
> Style: FIXME not TODO

Will be updated in next patch.

[Saam Barati]

(In reply to comment #8)
> Comment on attachment 274093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274093&action=review
> 
> >> Source/JavaScriptCore/ChangeLog:3
> >> +        Invoking super()/super inside of the eval should not lead to SyntaxError
> > 
> > Do we have a bug open for new.target inside eval?
> 
> Yes, I did this recently
>  https://bugs.webkit.org/show_bug.cgi?id=155545
> 
> >>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:643
> >>> +    if (needsToUpdateArrowFunctionContext() && !codeBlock->isArrowFunctionContext() && !isDerivedConstructorContext()) {
> >> 
> >> Why the "!isDerivedConstructorContext()" check?
> > 
> > Ah, I guess it's that way so we don't recreate a scope. I think I understand.
> 
> Yes, that is correct. It is for this case: 
> class C {};
> class D extends C {
>   constructor() {
>     eval("(()=>super())()");//Error
>   }
> }
> new D();
> Otherwise we will have two context scope for 'this'. I've spend last three
> evenings to find out why mention simple test case did not work
> 
> >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4145
> >> +    return m_scopeNode->doAnyInnerArrowFunctionsUseNewTarget() || m_scopeNode->doAnyInnerArrowFunctionsUseSuperCall() || m_scopeNode->doAnyInnerArrowFunctionsUseEval() || m_codeBlock->usesEval();
> > 
> > I believe m_codeBlock->usesEval() should always be true if m_scopeNode->doAnyInnerAroowFunctionsUseEval().
> > You should double check though.
> 
> It is true, but unfortunately this patch cover cases when we do not have
> arrow function, for instance from previous comments, so I need to check
> m_codeBlock->usesEval(). Possible we need to change name of the function
> isNewTargetUsedInInnerArrowFunction ->
> isNewTargetUsedInInnerArrowFunctionOrEval

What I mean is shouldn't it be sufficient to just check m_codeBlock->usesEval()?
It should never be the case that m_scopeNode->innerArrowEval() is true but m_codeBlock->usesEval() is false.
Therefore, m_codeBlock->usesEval() should cover everything.

[GSkachkov]

Created attachment 274212 [details]
Patch

[GSkachkov]

Comment on attachment 274093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274093&action=review

>>>> Source/JavaScriptCore/ChangeLog:3
>>>> +        Invoking super()/super inside of the eval should not lead to SyntaxError
>>> 
>>> Do we have a bug open for new.target inside eval?
>> 
>> Yes, I did this recently
>>  https://bugs.webkit.org/show_bug.cgi?id=155545
> 
> What I mean is shouldn't it be sufficient to just check m_codeBlock->usesEval()?
> It should never be the case that m_scopeNode->innerArrowEval() is true but m_codeBlock->usesEval() is false.
> Therefore, m_codeBlock->usesEval() should cover everything.

Hmm, before it was not, innerArrow... is set only for arrow functions and usesEval only if in current function/block we are using eval. So as for now:
  m_codeBlock->usesEval() is true but m_scopeNode-innerArrowEval() is false for this case -> var f = function () { eval('debug("hello world")'); }; f();
  m_codeBlock->usesEval() is false but m_scopeNode->innerArrowEval() true for case -> var f = function () { var arr = () => () =>  eval('debug("hello world")'); }; f();

[GSkachkov]

Committed r198324: <http://trac.webkit.org/changeset/198324>

[GSkachkov]

All reviewed patches have been landed.  Closing bug.