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");
+ })();
+}