[1.8>1.9] [MERGE #4618 @boingoing] OS#14568840: Remove 'this' binding for indirect eval

Merge pull request #4618 from boingoing:RemoveThisBindingIndirectEval

Having a 'this' binding in the indirect eval leads to problems if there is a lambda capturing 'this' in the indirect eval. The lambda would try to load 'this' from a scope slot in the global scope of the indirect eval which asserts.

Seems we can simplify the above by just removing the 'this' binding from the indirect eval. Then we'll simply load 'this' like an ordinary lambda at global scope would.

Fixes:
https://microsoft.visualstudio.com/web/wi.aspx?id=14568840

Author: boingoing

lib/Parser/Parse.cpp

@@ -1711,19 +1711,17 @@ ParseNodePtr Parser::CreateSpecialVarDeclIfNeeded(ParseNodePtr pnodeFnc, IdentPt
     return nullptr;
 }
 
-void Parser::CreateSpecialSymbolDeclarations(ParseNodePtr pnodeFnc, bool isGlobal)
+void Parser::CreateSpecialSymbolDeclarations(ParseNodePtr pnodeFnc)
 {
     // Lambda function cannot have any special bindings.
     if (pnodeFnc->sxFnc.IsLambda())
     {
         return;
     }
-    Assert(!(isGlobal && (this->m_grfscr & fscrEval)));
-    Assert(!isGlobal || (this->m_grfscr & fscrEvalCode));
 
     bool isTopLevelEventHandler = (this->m_grfscr & fscrImplicitThis || this->m_grfscr & fscrImplicitParents) && !pnodeFnc->sxFnc.IsNested();
 
-    // Create a 'this' symbol for indirect eval, non-lambda functions with references to 'this', and all class constructors and top level event hanlders.
+    // Create a 'this' symbol for non-lambda functions with references to 'this', and all class constructors and top level event hanlders.
     ParseNodePtr varDeclNode = CreateSpecialVarDeclIfNeeded(pnodeFnc, wellKnownPropertyPids._this, pnodeFnc->sxFnc.IsClassConstructor() || isTopLevelEventHandler);
     if (varDeclNode)
     {
@@ -1735,12 +1733,6 @@ void Parser::CreateSpecialSymbolDeclarations(ParseNodePtr pnodeFnc, bool isGloba
         }
     }
 
-    // Global code cannot have 'new.target' or 'super' bindings.
-    if (isGlobal)
-    {
-        return;
-    }
-
     // Create a 'new.target' symbol for any ordinary function with a reference and all class constructors.
     varDeclNode = CreateSpecialVarDeclIfNeeded(pnodeFnc, wellKnownPropertyPids._newTarget, pnodeFnc->sxFnc.IsClassConstructor());
     if (varDeclNode)
@@ -5760,7 +5752,7 @@ bool Parser::ParseFncDeclHelper(ParseNodePtr pnodeFnc, LPCOLESTR pNameHint, usho
             UpdateArgumentsNode(pnodeFnc, argNode);
         }
 
-        CreateSpecialSymbolDeclarations(pnodeFnc, false);
+        CreateSpecialSymbolDeclarations(pnodeFnc);
 
         // Restore the lists of scopes that contain function expressions.
 
@@ -7082,7 +7074,7 @@ ParseNodePtr Parser::GenerateEmptyConstructor(bool extends)
 
     FinishParseBlock(pnodeInnerBlock);
 
-    CreateSpecialSymbolDeclarations(pnodeFnc, false);
+    CreateSpecialSymbolDeclarations(pnodeFnc);
 
     FinishParseBlock(pnodeBlock);
 
@@ -11352,7 +11344,7 @@ void Parser::FinishDeferredFunction(ParseNodePtr pnodeScopeList)
                 UpdateArgumentsNode(pnodeFnc, argNode);
             }
 
-            CreateSpecialSymbolDeclarations(pnodeFnc, false);
+            CreateSpecialSymbolDeclarations(pnodeFnc);
 
             this->FinishParseBlock(pnodeBlock);
             if (pnodeFncExprBlock)
@@ -11762,12 +11754,6 @@ ParseNodePtr Parser::Parse(LPCUTF8 pszSrc, size_t offset, size_t length, charcou
     if (tkEOF != m_token.tk)
         Error(ERRsyntax);
 
-    // We only need to create special symbol bindings for 'this' for indirect eval
-    if ((this->m_grfscr & fscrEvalCode) && !(this->m_grfscr & fscrEval))
-    {
-        CreateSpecialSymbolDeclarations(pnodeProg, true);
-    }
-
     // Append an EndCode node.
     AddToNodeList(&pnodeProg->sxFnc.pnodeBody, &lastNodeRef,
         CreateNodeWithScanner<knopEndCode>());

lib/Parser/Parse.h

@@ -755,7 +755,7 @@ class Parser
     void FinishParseFncExprScope(ParseNodePtr pnodeFnc, ParseNodePtr pnodeFncExprScope);
 
     bool IsSpecialName(IdentPtr pid);
-    void CreateSpecialSymbolDeclarations(ParseNodePtr pnodeFnc, bool isGlobal);
+    void CreateSpecialSymbolDeclarations(ParseNodePtr pnodeFnc);
     ParseNodePtr ReferenceSpecialName(IdentPtr pid, charcount_t ichMin = 0, charcount_t ichLim = 0, bool createNode = false);
     ParseNodePtr CreateSpecialVarDeclIfNeeded(ParseNodePtr pnodeFnc, IdentPtr pid, bool forceCreate = false);
 

test/Basics/SpecialSymbolCapture.js

@@ -961,6 +961,19 @@ var tests = [
             assert.throws(() => WScript.LoadScript(`(class classExpr {}())`), TypeError, "Class expression called at global scope", "Class constructor cannot be called without the new keyword");
             assert.throws(() => WScript.LoadScript(`(() => (class classExpr {}()))()`), TypeError, "Class expression called in global lambda", "Class constructor cannot be called without the new keyword");
         }
+    },
+    {
+        name: "Indirect eval should not create a 'this' binding",
+        body: function() {
+            WScript.LoadScript(`
+                this.eval("(() => assert.areEqual('global', this.o, 'Lambda in indirect eval called off of this capturing this'))()");
+                this['eval']("(() => assert.areEqual('global', this.o, 'Lambda in indirect eval called from a property index capturing this'))()");
+                var _eval = 'eval';
+                this[_eval]("(() => assert.areEqual('global', this.o, 'Lambda in indirect eval called from a property index capturing this'))()");
+                _eval = eval;
+                _eval("(() => assert.areEqual('global', this.o, 'Lambda in indirect eval capturing this'))()");
+            `);
+        }
     }
 ]