Title: [188926] trunk/Source/_javascript_Core
Revision
188926
Author
saambara...@gmail.com
Date
2015-08-25 11:40:14 -0700 (Tue, 25 Aug 2015)

Log Message

Callee can be incorrectly overridden when it's captured
https://bugs.webkit.org/show_bug.cgi?id=148400

Reviewed by Filip Pizlo.

We now resort to always creating the function name scope
when the function name is in scope. Because the bytecode
generator now has a notion of local lexical scoping,
this incurs no runtime penalty for function _expression_ names
that aren't heap allocated. If they are heap allocated,
this means we may now have one more scope on the runtime
scope stack than before. This modification simplifies the
callee initialization code and uses the lexical scoping constructs
to implement this. This implementation also ensures
that everything Just Works for function's with default
parameter values. Before this patch, IIFE functions
with default parameter values and a captured function
name would crash JSC.

* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::pushLexicalScopeInternal):
(JSC::BytecodeGenerator::popLexicalScopeInternal):
(JSC::BytecodeGenerator::variable):
(JSC::BytecodeGenerator::resolveType):
(JSC::BytecodeGenerator::emitThrowTypeError):
(JSC::BytecodeGenerator::emitPushFunctionNameScope):
(JSC::BytecodeGenerator::emitReadOnlyExceptionIfNeeded):
* bytecompiler/BytecodeGenerator.h:
(JSC::Variable::isReadOnly):
(JSC::Variable::isSpecial):
(JSC::Variable::isConst):
(JSC::Variable::setIsReadOnly):
* bytecompiler/NodesCodegen.cpp:
(JSC::PostfixNode::emitResolve):
(JSC::PrefixNode::emitResolve):
(JSC::ReadModifyResolveNode::emitBytecode):
(JSC::AssignResolveNode::emitBytecode):
(JSC::BindingNode::bindValue):
* tests/stress/IIFE-es6-default-parameters.js: Added.
(assert):
(.):
* tests/stress/IIFE-function-name-captured.js: Added.
(assert):
(.):

Modified Paths

Added Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (188925 => 188926)


--- trunk/Source/_javascript_Core/ChangeLog	2015-08-25 18:38:14 UTC (rev 188925)
+++ trunk/Source/_javascript_Core/ChangeLog	2015-08-25 18:40:14 UTC (rev 188926)
@@ -1,3 +1,51 @@
+2015-08-25  Saam barati  <sbar...@apple.com>
+
+        Callee can be incorrectly overridden when it's captured
+        https://bugs.webkit.org/show_bug.cgi?id=148400
+
+        Reviewed by Filip Pizlo.
+
+        We now resort to always creating the function name scope
+        when the function name is in scope. Because the bytecode
+        generator now has a notion of local lexical scoping,
+        this incurs no runtime penalty for function _expression_ names
+        that aren't heap allocated. If they are heap allocated,
+        this means we may now have one more scope on the runtime
+        scope stack than before. This modification simplifies the
+        callee initialization code and uses the lexical scoping constructs
+        to implement this. This implementation also ensures
+        that everything Just Works for function's with default
+        parameter values. Before this patch, IIFE functions
+        with default parameter values and a captured function
+        name would crash JSC.
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::BytecodeGenerator):
+        (JSC::BytecodeGenerator::pushLexicalScopeInternal):
+        (JSC::BytecodeGenerator::popLexicalScopeInternal):
+        (JSC::BytecodeGenerator::variable):
+        (JSC::BytecodeGenerator::resolveType):
+        (JSC::BytecodeGenerator::emitThrowTypeError):
+        (JSC::BytecodeGenerator::emitPushFunctionNameScope):
+        (JSC::BytecodeGenerator::emitReadOnlyExceptionIfNeeded):
+        * bytecompiler/BytecodeGenerator.h:
+        (JSC::Variable::isReadOnly):
+        (JSC::Variable::isSpecial):
+        (JSC::Variable::isConst):
+        (JSC::Variable::setIsReadOnly):
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::PostfixNode::emitResolve):
+        (JSC::PrefixNode::emitResolve):
+        (JSC::ReadModifyResolveNode::emitBytecode):
+        (JSC::AssignResolveNode::emitBytecode):
+        (JSC::BindingNode::bindValue):
+        * tests/stress/IIFE-es6-default-parameters.js: Added.
+        (assert):
+        (.):
+        * tests/stress/IIFE-function-name-captured.js: Added.
+        (assert):
+        (.):
+
 2015-08-24  Brian Burg  <bb...@apple.com>
 
         Web Inspector: add protocol test for existing error handling performed by the backend

Modified: trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp (188925 => 188926)


--- trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp	2015-08-25 18:38:14 UTC (rev 188925)
+++ trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp	2015-08-25 18:40:14 UTC (rev 188926)
@@ -232,9 +232,11 @@
     
     m_calleeRegister.setIndex(JSStack::Callee);
     
-    if (functionNameIsInScope(functionNode->ident(), functionNode->functionMode())
-        && functionNameScopeIsDynamic(codeBlock->usesEval(), codeBlock->isStrictMode())) {
-        emitPushFunctionNameScope(functionNode->ident(), &m_calleeRegister);
+    if (functionNameIsInScope(functionNode->ident(), functionNode->functionMode())) {
+        bool isDynamicScope = functionNameScopeIsDynamic(codeBlock->usesEval(), codeBlock->isStrictMode());
+        bool isFunctionNameCaptured = captures(functionNode->ident().impl());
+        bool markAsCaptured = isDynamicScope || isFunctionNameCaptured;
+        emitPushFunctionNameScope(functionNode->ident(), &m_calleeRegister, markAsCaptured);
     }
     
     if (shouldCaptureSomeOfTheThings) {
@@ -396,15 +398,12 @@
     // - "arguments": unless it's used as a function or parameter, this should refer to the
     //   arguments object.
     //
-    // - callee: unless it's used as a var, function, or parameter, this should refer to the
-    //   callee (i.e. our function).
-    //
     // - functions: these always override everything else.
     //
     // The most logical way to do all of this is to initialize none of the variables until now,
     // and then initialize them in BytecodeGenerator::generate() in such an order that the rules
-    // for how these things override each other end up holding. We would initialize the callee
-    // first, then "arguments", then all arguments, then the functions.
+    // for how these things override each other end up holding. We would initialize "arguments" first, 
+    // then all arguments, then the functions.
     //
     // But some arguments are already initialized by default, since if they aren't captured and we
     // don't have "arguments" then we just point the symbol table at the stack slot of those
@@ -412,35 +411,6 @@
     // binding (i.e. don't involve destructuring) above when figuring out how to lay them out,
     // because that's just the simplest thing. This means that when we initialize them, we have to
     // watch out for the things that override arguments (namely, functions).
-    //
-    // We also initialize callee here as well, just because it's so weird. We know whether we want
-    // to do this because we can just check if it's in the symbol table.
-    if (functionNameIsInScope(functionNode->ident(), functionNode->functionMode())
-        && !functionNameScopeIsDynamic(codeBlock->usesEval(), codeBlock->isStrictMode())
-        && functionSymbolTable->get(functionNode->ident().impl()).isNull()) {
-        if (captures(functionNode->ident().impl())) {
-            ScopeOffset offset;
-            {
-                ConcurrentJITLocker locker(functionSymbolTable->m_lock);
-                offset = functionSymbolTable->takeNextScopeOffset(locker);
-                functionSymbolTable->add(
-                    locker, functionNode->ident().impl(),
-                    SymbolTableEntry(VarOffset(offset), ReadOnly));
-            }
-            
-            emitOpcode(op_put_to_scope);
-            instructions().append(m_lexicalEnvironmentRegister->index());
-            instructions().append(addConstant(functionNode->ident()));
-            instructions().append(m_calleeRegister.index());
-            instructions().append(ResolveModeAndType(ThrowIfNotFound, LocalClosureVar).operand());
-            instructions().append(symbolTableConstantIndex);
-            instructions().append(offset.offset());
-        } else {
-            functionSymbolTable->add(
-                functionNode->ident().impl(),
-                SymbolTableEntry(VarOffset(m_calleeRegister.virtualRegister()), ReadOnly));
-        }
-    }
     
     // This is our final act of weirdness. "arguments" is overridden by everything except the
     // callee. We add it to the symbol table if it's not already there and it's not an argument.
@@ -1441,8 +1411,12 @@
                 hasCapturedVariables = true;
             } else {
                 ASSERT(varKind == VarKind::Stack);
-                RegisterID* local = newBlockScopeVariable();
-                local->ref();
+                RegisterID* local;
+                if (scopeRegisterType == ScopeRegisterType::Block) {
+                    local = newBlockScopeVariable();
+                    local->ref();
+                } else
+                    local = addVar();
                 varOffset = VarOffset(local->virtualRegister());
             }
 
@@ -1512,6 +1486,8 @@
 
 void BytecodeGenerator::popLexicalScopeInternal(VariableEnvironment& environment, TDZRequirement tdzRequirement)
 {
+    // NOTE: This function only makes sense for scopes that aren't ScopeRegisterType::Var (only function name scope right now is ScopeRegisterType::Var).
+    // This doesn't make sense for ScopeRegisterType::Var because we deref RegisterIDs here.
     if (!environment.size())
         return;
 
@@ -1654,12 +1630,20 @@
         SymbolTableEntry symbolTableEntry = symbolTable->get(property.impl());
         if (symbolTableEntry.isNull())
             continue;
-        if (symbolTable->scopeType() == SymbolTable::ScopeType::FunctionNameScope && m_usesNonStrictEval) {
-            // We don't know if an eval has introduced a "var" named the same thing as the function name scope variable name.
-            // We resort to dynamic lookup to answer this question.
-            return Variable(property);
+        bool resultIsCallee = false;
+        if (symbolTable->scopeType() == SymbolTable::ScopeType::FunctionNameScope) {
+            if (m_usesNonStrictEval) {
+                // We don't know if an eval has introduced a "var" named the same thing as the function name scope variable name.
+                // We resort to dynamic lookup to answer this question.
+                Variable result = Variable(property);
+                return result;
+            }
+            resultIsCallee = true;
         }
-        return variableForLocalEntry(property, symbolTableEntry, stackEntry.m_symbolTableConstantIndex, symbolTable->scopeType() == SymbolTable::ScopeType::LexicalScope);
+        Variable result = variableForLocalEntry(property, symbolTableEntry, stackEntry.m_symbolTableConstantIndex, symbolTable->scopeType() == SymbolTable::ScopeType::LexicalScope);
+        if (resultIsCallee)
+            result.setIsReadOnly();
+        return result;
     }
 
     return Variable(property);
@@ -1740,8 +1724,9 @@
         if (m_symbolTableStack[i].m_isWithScope)
             return Dynamic;
         if (m_usesNonStrictEval && m_symbolTableStack[i].m_symbolTable->scopeType() == SymbolTable::ScopeType::FunctionNameScope) {
-            // What we really want here is something like LocalClosureVarWithVarInjectionsCheck but it's probably
-            // not worth inventing just for the function name scope.
+            // We never want to assign to a FunctionNameScope. Returning Dynamic here achieves this goal.
+            // If we aren't in non-strict eval mode, then NodesCodeGen needs to take care not to emit
+            // a put_to_scope with the destination being the function name scope variable.
             return Dynamic;
         }
     }
@@ -3101,7 +3086,7 @@
     instructions().append(false);
 }
 
-void BytecodeGenerator::emitPushFunctionNameScope(const Identifier& property, RegisterID* callee)
+void BytecodeGenerator::emitPushFunctionNameScope(const Identifier& property, RegisterID* callee, bool isCaptured)
 {
     // There is some nuance here:
     // If we're in strict mode code, the function name scope variable acts exactly like a "const" variable.
@@ -3114,7 +3099,8 @@
     // to lexical environments.
     VariableEnvironment nameScopeEnvironment;
     auto addResult = nameScopeEnvironment.add(property);
-    addResult.iterator->value.setIsCaptured();
+    if (isCaptured)
+        addResult.iterator->value.setIsCaptured();
     addResult.iterator->value.setIsConst(); // The function name scope name acts like a const variable.
     unsigned numVars = m_codeBlock->m_numVars;
     pushLexicalScopeInternal(nameScopeEnvironment, true, nullptr, TDZRequirement::NotUnderTDZ, ScopeType::FunctionNameScope, ScopeRegisterType::Var);
@@ -3287,6 +3273,8 @@
 
 bool BytecodeGenerator::emitReadOnlyExceptionIfNeeded(const Variable& variable)
 {
+    // If we're in strict mode, we always throw.
+    // If we're not in strict mode, we throw for "const" variables but not the function callee.
     if (isStrictMode() || variable.isConst()) {
         emitOpcode(op_throw_static_error);
         instructions().append(addConstantValue(addStringConstant(Identifier::fromString(m_vm, StrictModeReadonlyPropertyWriteError)))->index());

Modified: trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.h (188925 => 188926)


--- trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.h	2015-08-25 18:38:14 UTC (rev 188925)
+++ trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.h	2015-08-25 18:40:14 UTC (rev 188926)
@@ -234,6 +234,7 @@
         bool isReadOnly() const { return m_attributes & ReadOnly; }
         bool isSpecial() const { return m_kind != NormalVariable; }
         bool isConst() const { return isReadOnly() && m_isLexicallyScoped; }
+        void setIsReadOnly() { m_attributes |= ReadOnly; }
 
     private:
         Identifier m_ident;
@@ -589,7 +590,6 @@
         void emitThrowReferenceError(const String& message);
         void emitThrowTypeError(const String& message);
 
-        void emitPushFunctionNameScope(const Identifier& property, RegisterID* value);
         void emitPushCatchScope(const Identifier& property, RegisterID* exceptionValue, VariableEnvironment&);
         void emitPopCatchScope(VariableEnvironment&);
 
@@ -637,6 +637,8 @@
         void popLexicalScopeInternal(VariableEnvironment&, TDZRequirement);
         void emitPopScope(RegisterID* dst, RegisterID* scope);
         RegisterID* emitGetParentScope(RegisterID* dst, RegisterID* scope);
+        void emitPushFunctionNameScope(const Identifier& property, RegisterID* value, bool isCaptured);
+
     public:
         void pushLexicalScope(VariableEnvironmentNode*, bool canOptimizeTDZChecks, RegisterID** constantSymbolTableResult = nullptr);
         void popLexicalScope(VariableEnvironmentNode*);

Modified: trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp (188925 => 188926)


--- trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp	2015-08-25 18:38:14 UTC (rev 188925)
+++ trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp	2015-08-25 18:40:14 UTC (rev 188926)
@@ -1052,8 +1052,10 @@
             return value.get();
     }
     RefPtr<RegisterID> oldValue = emitPostIncOrDec(generator, generator.finalDestination(dst), value.get(), m_operator);
-    generator.emitPutToScope(scope.get(), var, value.get(), ThrowIfNotFound);
-    generator.emitProfileType(value.get(), var, divotStart(), divotEnd());
+    if (!var.isReadOnly()) {
+        generator.emitPutToScope(scope.get(), var, value.get(), ThrowIfNotFound);
+        generator.emitProfileType(value.get(), var, divotStart(), divotEnd());
+    }
 
     return oldValue.get();
 }
@@ -1250,8 +1252,10 @@
     }
 
     emitIncOrDec(generator, value.get(), m_operator);
-    generator.emitPutToScope(scope.get(), var, value.get(), ThrowIfNotFound);
-    generator.emitProfileType(value.get(), var, divotStart(), divotEnd());
+    if (!var.isReadOnly()) {
+        generator.emitPutToScope(scope.get(), var, value.get(), ThrowIfNotFound);
+        generator.emitProfileType(value.get(), var, divotStart(), divotEnd());
+    }
     return generator.moveToDestinationIfNeeded(dst, value.get());
 }
 
@@ -1771,8 +1775,11 @@
             return value.get();
     }
     RefPtr<RegisterID> result = emitReadModifyAssignment(generator, generator.finalDestination(dst, value.get()), value.get(), m_right, m_operator, OperandTypes(ResultType::unknownType(), m_right->resultDescriptor()), this);
-    RegisterID* returnResult = generator.emitPutToScope(scope.get(), var, result.get(), ThrowIfNotFound);
-    generator.emitProfileType(result.get(), var, divotStart(), divotEnd());
+    RegisterID* returnResult = result.get();
+    if (!var.isReadOnly()) {
+        returnResult = generator.emitPutToScope(scope.get(), var, result.get(), ThrowIfNotFound);
+        generator.emitProfileType(result.get(), var, divotStart(), divotEnd());
+    }
     return returnResult;
 }
 
@@ -1781,12 +1788,13 @@
 RegisterID* AssignResolveNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst)
 {
     Variable var = generator.variable(m_ident);
+    bool isReadOnly = var.isReadOnly() && m_assignmentContext != AssignmentContext::ConstDeclarationStatement;
     if (RegisterID* local = var.local()) {
         RegisterID* result = nullptr;
         if (m_assignmentContext == AssignmentContext::AssignmentExpression)
             generator.emitTDZCheckIfNecessary(var, local, nullptr);
 
-        if (var.isReadOnly() && m_assignmentContext != AssignmentContext::ConstDeclarationStatement) {
+        if (isReadOnly) {
             result = generator.emitNode(dst, m_right); // Execute side effects first.
             generator.emitReadOnlyExceptionIfNeeded(var);
             generator.emitProfileType(result, var, divotStart(), divotEnd());
@@ -1817,15 +1825,18 @@
     if (dst == generator.ignoredResult())
         dst = 0;
     RefPtr<RegisterID> result = generator.emitNode(dst, m_right);
-    if (var.isReadOnly() && m_assignmentContext != AssignmentContext::ConstDeclarationStatement) {
+    if (isReadOnly) {
         RegisterID* result = generator.emitNode(dst, m_right); // Execute side effects first.
         bool threwException = generator.emitReadOnlyExceptionIfNeeded(var);
         if (threwException)
             return result;
     }
     generator.emitExpressionInfo(divot(), divotStart(), divotEnd());
-    RegisterID* returnResult = generator.emitPutToScope(scope.get(), var, result.get(), generator.isStrictMode() ? ThrowIfNotFound : DoNotThrowIfNotFound);
-    generator.emitProfileType(result.get(), var, divotStart(), divotEnd());
+    RegisterID* returnResult = result.get();
+    if (!isReadOnly) {
+        returnResult = generator.emitPutToScope(scope.get(), var, result.get(), generator.isStrictMode() ? ThrowIfNotFound : DoNotThrowIfNotFound);
+        generator.emitProfileType(result.get(), var, divotStart(), divotEnd());
+    }
 
     if (m_assignmentContext == AssignmentContext::DeclarationStatement || m_assignmentContext == AssignmentContext::ConstDeclarationStatement)
         generator.liftTDZCheckIfPossible(var);
@@ -3282,13 +3293,13 @@
 void BindingNode::bindValue(BytecodeGenerator& generator, RegisterID* value) const
 {
     Variable var = generator.variable(m_boundProperty);
+    bool isReadOnly = var.isReadOnly() && m_bindingContext != AssignmentContext::ConstDeclarationStatement;
     if (RegisterID* local = var.local()) {
         if (m_bindingContext == AssignmentContext::AssignmentExpression)
             generator.emitTDZCheckIfNecessary(var, local, nullptr);
-        if (var.isReadOnly() && m_bindingContext != AssignmentContext::ConstDeclarationStatement) {
-            bool threwException = generator.emitReadOnlyExceptionIfNeeded(var);
-            if (threwException)
-                return;
+        if (isReadOnly) {
+            generator.emitReadOnlyExceptionIfNeeded(var);
+            return;
         }
         generator.emitMove(local, value);
         generator.emitProfileType(local, var, divotStart(), divotEnd());
@@ -3302,10 +3313,9 @@
     generator.emitExpressionInfo(divotEnd(), divotStart(), divotEnd());
     if (m_bindingContext == AssignmentContext::AssignmentExpression)
         generator.emitTDZCheckIfNecessary(var, nullptr, scope);
-    if (var.isReadOnly() && m_bindingContext != AssignmentContext::ConstDeclarationStatement) {
-        bool threwException = generator.emitReadOnlyExceptionIfNeeded(var);
-        if (threwException)
-            return;
+    if (isReadOnly) {
+        generator.emitReadOnlyExceptionIfNeeded(var);
+        return;
     }
     generator.emitPutToScope(scope, var, value, generator.isStrictMode() ? ThrowIfNotFound : DoNotThrowIfNotFound);
     generator.emitProfileType(value, var, divotStart(), divotEnd());

Added: trunk/Source/_javascript_Core/tests/stress/IIFE-es6-default-parameters.js (0 => 188926)


--- trunk/Source/_javascript_Core/tests/stress/IIFE-es6-default-parameters.js	                        (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/IIFE-es6-default-parameters.js	2015-08-25 18:40:14 UTC (rev 188926)
@@ -0,0 +1,35 @@
+
+function assert(b) {
+    if (!b)
+        throw new Error("Bad assertion");
+}
+
+for (var i = 0; i < 1000; i++) {
+
+    ;(function foo(x = 20) {
+        assert(typeof foo === "function");
+    })();
+
+    ;(function foo(x = 20) {
+        function bar() { return foo; }
+        assert(typeof foo === "function");
+    })();
+
+    ;(function foo(x = foo) {
+        var foo = 20;
+        assert(foo === 20);
+        assert(typeof x === "function");
+    })();
+
+    ;(function foo(capFoo = function() { return foo; }) {
+        var foo = 20;
+        assert(foo === 20);
+        assert(typeof capFoo() === "function");
+    })();
+
+    ;(function foo(x = eval("foo")) {
+        var foo = 20;
+        assert(foo === 20);
+        assert(typeof x === "function");
+    })();
+}

Added: trunk/Source/_javascript_Core/tests/stress/IIFE-function-name-captured.js (0 => 188926)


--- trunk/Source/_javascript_Core/tests/stress/IIFE-function-name-captured.js	                        (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/IIFE-function-name-captured.js	2015-08-25 18:40:14 UTC (rev 188926)
@@ -0,0 +1,38 @@
+function assert(b) {
+    if (!b)
+        throw new Error("Bad assertion");
+}
+
+for (var i = 0; i < 1000; i++) {
+    ;(function foo() {
+        foo = 20;
+        assert(foo !== 20);
+        assert(typeof foo === "function");
+    })();
+
+    ;(function foo() {
+        var bar = function() { return foo; }
+        foo = 20;
+        assert(foo !== 20);
+        assert(bar() !== 20);
+        assert(typeof foo === "function");
+        assert(typeof bar() === "function");
+    })();
+
+    ;(function foo() {
+        eval("foo = 20;");
+        assert(foo !== 20);
+        assert(typeof foo === "function");
+    })();
+
+    ;(function foo() {
+        eval("var foo = 20;");
+        assert(foo === 20);
+    })();
+
+    ;(function foo() {
+        "use strict";
+        assert(foo !== 20);
+        assert(typeof foo === "function");
+    })();
+}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to