Changeset 69940 in webkit

Timestamp:

Oct 17, 2010 9:08:37 PM (14 years ago)

Author:

oliver@apple.com

Message:

2010-10-17 Oliver Hunt <​oliver@apple.com>

Reviewed by Sam Weinig.

Strict mode: Assignment that would create a global should be a late ReferenceError, not a syntax failure
​https://bugs.webkit.org/show_bug.cgi?id=47788

Fixing this required a couple of changes:

• resolve_base now has a flag to indicate whether it is being used for a put in strict mode. this allows us to throw an exception when we're doing a completely generic resolve for assignment, and that assignment would create a new global.
• There is a new opcode 'op_ensure_property_exists' that is used to determine whether the property being assigned to already exists on the global object. This currently has no caching, but such caching could be added relatively trivially. It is only used in the case where we know that a property will be placed on the global object, and we cannot verify that the property already exists.

In the jit we plant a call to cti_op_resolve_base_strict_put in the effected case rather
than making op_resolve_base have an additional runtime branch.

There's also a new helper function to create the exception for the invalid assignment.

• bytecode/CodeBlock.cpp: (JSC::CodeBlock::dump):
• bytecode/Opcode.h:
• bytecompiler/BytecodeGenerator.cpp: (JSC::BytecodeGenerator::emitResolveBase): (JSC::BytecodeGenerator::emitResolveBaseForPut):
• bytecompiler/BytecodeGenerator.h:
• bytecompiler/NodesCodegen.cpp: (JSC::AssignResolveNode::emitBytecode): (JSC::ForInNode::emitBytecode):
• interpreter/Interpreter.cpp: (JSC::Interpreter::resolveBase): (JSC::Interpreter::privateExecute):
• jit/JIT.cpp: (JSC::JIT::privateCompileMainPass):
• jit/JIT.h:
• jit/JITOpcodes.cpp: (JSC::JIT::emit_op_resolve_base): (JSC::JIT::emit_op_ensure_property_exists):
• jit/JITOpcodes32_64.cpp: (JSC::JIT::emit_op_resolve_base): (JSC::JIT::emit_op_ensure_property_exists):
• jit/JITStubs.cpp: (JSC::DEFINE_STUB_FUNCTION):
• jit/JITStubs.h:
• parser/JSParser.cpp: (JSC::JSParser::parseProgram):
• runtime/ExceptionHelpers.cpp: (JSC::createErrorForInvalidGlobalAssignment):
• runtime/ExceptionHelpers.h:
• runtime/Operations.h: (JSC::resolveBase):

2010-10-17 Oliver Hunt <​oliver@apple.com>

Reviewed by Sam Weinig.

Strict mode: Assignment that would create a global should be a late ReferenceError, not a syntax failure
​https://bugs.webkit.org/show_bug.cgi?id=47788

Update test to check for the correct behaviour.

• fast/js/basic-strict-mode-expected.txt:
• fast/js/script-tests/basic-strict-mode.js:

Location:

trunk

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/bytecode/CodeBlock.cpp (modified) (1 diff)
• JavaScriptCore/bytecode/Opcode.h (modified) (1 diff)
• JavaScriptCore/bytecompiler/BytecodeGenerator.cpp (modified) (2 diffs)
• JavaScriptCore/bytecompiler/BytecodeGenerator.h (modified) (1 diff)
• JavaScriptCore/bytecompiler/NodesCodegen.cpp (modified) (2 diffs)
• JavaScriptCore/interpreter/Interpreter.cpp (modified) (2 diffs)
• JavaScriptCore/jit/JIT.cpp (modified) (1 diff)
• JavaScriptCore/jit/JIT.h (modified) (1 diff)
• JavaScriptCore/jit/JITOpcodes.cpp (modified) (1 diff)
• JavaScriptCore/jit/JITOpcodes32_64.cpp (modified) (1 diff)
• JavaScriptCore/jit/JITStubs.cpp (modified) (1 diff)
• JavaScriptCore/jit/JITStubs.h (modified) (1 diff)
• JavaScriptCore/parser/JSParser.cpp (modified) (1 diff)
• JavaScriptCore/runtime/ExceptionHelpers.cpp (modified) (1 diff)
• JavaScriptCore/runtime/ExceptionHelpers.h (modified) (1 diff)
• JavaScriptCore/runtime/Operations.h (modified) (2 diffs)
• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/js/basic-strict-mode-expected.txt (modified) (1 diff)
• LayoutTests/fast/js/script-tests/basic-strict-mode.js (modified) (1 diff)

trunk/JavaScriptCore/ChangeLog

@@ +1 @@
+2010-10-17  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Sam Weinig.
+
+        Strict mode: Assignment that would create a global should be a late ReferenceError, not a syntax failure
+        https://bugs.webkit.org/show_bug.cgi?id=47788
+
+        Fixing this required a couple of changes:
+         * resolve_base now has a flag to indicate whether it is being used for a put in strict mode.
+           this allows us to throw an exception when we're doing a completely generic resolve for
+           assignment, and that assignment would create a new global.
+         * There is a new opcode 'op_ensure_property_exists' that is used to determine whether
+           the property being assigned to already exists on the global object.  This currently
+           has no caching, but such caching could be added relatively trivially.  It is only used
+           in the case where we know that a property will be placed on the global object, and
+           we cannot verify that the property already exists.
+
+        In the jit we plant a call to cti_op_resolve_base_strict_put in the effected case rather
+        than making op_resolve_base have an additional runtime branch.
+
+        There's also a new helper function to create the exception for the invalid assignment.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::dump):
+        * bytecode/Opcode.h:
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::emitResolveBase):
+        (JSC::BytecodeGenerator::emitResolveBaseForPut):
+        * bytecompiler/BytecodeGenerator.h:
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::AssignResolveNode::emitBytecode):
+        (JSC::ForInNode::emitBytecode):
+        * interpreter/Interpreter.cpp:
+        (JSC::Interpreter::resolveBase):
+        (JSC::Interpreter::privateExecute):
+        * jit/JIT.cpp:
+        (JSC::JIT::privateCompileMainPass):
+        * jit/JIT.h:
+        * jit/JITOpcodes.cpp:
+        (JSC::JIT::emit_op_resolve_base):
+        (JSC::JIT::emit_op_ensure_property_exists):
+        * jit/JITOpcodes32_64.cpp:
+        (JSC::JIT::emit_op_resolve_base):
+        (JSC::JIT::emit_op_ensure_property_exists):
+        * jit/JITStubs.cpp:
+        (JSC::DEFINE_STUB_FUNCTION):
+        * jit/JITStubs.h:
+        * parser/JSParser.cpp:
+        (JSC::JSParser::parseProgram):
+        * runtime/ExceptionHelpers.cpp:
+        (JSC::createErrorForInvalidGlobalAssignment):
+        * runtime/ExceptionHelpers.h:
+        * runtime/Operations.h:
+        (JSC::resolveBase):
+
 2010-10-17  Simon Fraser  <simon.fraser@apple.com>
 

trunk/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -762 +762 @@
             int r0 = (++it)->u.operand;
             int id0 = (++it)->u.operand;
-            printf("[%4d] resolve_base\t %s, %s\n", location, registerName(exec, r0).data(), idName(id0, m_identifiers[id0]).data());
+            int isStrict = (++it)->u.operand;
+            printf("[%4d] resolve_base%s\t %s, %s\n", location, isStrict ? "_strict" : "", registerName(exec, r0).data(), idName(id0, m_identifiers[id0]).data());
+            break;
+        }
+        case op_ensure_property_exists: {
+            int r0 = (++it)->u.operand;
+            int id0 = (++it)->u.operand;
+            printf("[%4d] ensure_property_exists\t %s, %s\n", location, registerName(exec, r0).data(), idName(id0, m_identifiers[id0]).data());
             break;
         }

trunk/JavaScriptCore/bytecode/Opcode.h

@@ -101 +101 @@
         macro(op_get_global_var, 3) \
         macro(op_put_global_var, 3) \
-        macro(op_resolve_base, 3) \
+        macro(op_resolve_base, 4) \
+        macro(op_ensure_property_exists, 3) \
         macro(op_resolve_with_base, 4) \
         macro(op_get_by_id, 8) \

trunk/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

@@ -1272 +1272 @@
         instructions().append(dst->index());
         instructions().append(addConstant(property));
+        instructions().append(false);
         return dst;
     }
@@ -1277 +1278 @@
     // Global object is the base
     return emitLoad(dst, JSValue(globalObject));
+}
+
+RegisterID* BytecodeGenerator::emitResolveBaseForPut(RegisterID* dst, const Identifier& property)
+{
+    if (!m_codeBlock->isStrictMode())
+        return emitResolveBase(dst, property);
+    size_t depth = 0;
+    int index = 0;
+    JSObject* globalObject = 0;
+    bool requiresDynamicChecks = false;
+    findScopedProperty(property, index, depth, false, requiresDynamicChecks, globalObject);
+    if (!globalObject || requiresDynamicChecks) {
+        // We can't optimise at all :-(
+        emitOpcode(op_resolve_base);
+        instructions().append(dst->index());
+        instructions().append(addConstant(property));
+        instructions().append(true);
+        return dst;
+    }
+    
+    // Global object is the base
+    RefPtr<RegisterID> result = emitLoad(dst, JSValue(globalObject));
+    emitOpcode(op_ensure_property_exists);
+    instructions().append(dst->index());
+    instructions().append(addConstant(property));
+    return result.get();
 }
 

trunk/JavaScriptCore/bytecompiler/BytecodeGenerator.h

@@ -330 +330 @@
 
         RegisterID* emitResolveBase(RegisterID* dst, const Identifier& property);
+        RegisterID* emitResolveBaseForPut(RegisterID* dst, const Identifier& property);
         RegisterID* emitResolveWithBase(RegisterID* baseDst, RegisterID* propDst, const Identifier& property);
 

trunk/JavaScriptCore/bytecompiler/NodesCodegen.cpp

@@ -1232 +1232 @@
     }
 
-    RefPtr<RegisterID> base = generator.emitResolveBase(generator.newTemporary(), m_ident);
+    RefPtr<RegisterID> base = generator.emitResolveBaseForPut(generator.newTemporary(), m_ident);
     if (dst == generator.ignoredResult())
         dst = 0;
@@ -1606 +1606 @@
             propertyName = generator.newTemporary();
             RefPtr<RegisterID> protect = propertyName;
-            RegisterID* base = generator.emitResolveBase(generator.newTemporary(), ident);
+            RegisterID* base = generator.emitResolveBaseForPut(generator.newTemporary(), ident);
 
             generator.emitExpressionInfo(divot(), startOffset(), endOffset());

trunk/JavaScriptCore/interpreter/Interpreter.cpp

@@ -281 +281 @@
     int dst = vPC[1].u.operand;
     int property = vPC[2].u.operand;
-    callFrame->r(dst) = JSValue(JSC::resolveBase(callFrame, callFrame->codeBlock()->identifier(property), callFrame->scopeChain()));
-    ASSERT(callFrame->r(dst).jsValue());
+    bool isStrictPut = vPC[3].u.operand;
+    const Identifier ident = callFrame->codeBlock()->identifier(property);
+    JSValue result = JSC::resolveBase(callFrame, ident, callFrame->scopeChain(), isStrictPut);
+    if (!result) {
+        callFrame->r(dst) = result;
+        ASSERT(callFrame->r(dst).jsValue());
+    } else
+        callFrame->globalData().exception = createErrorForInvalidGlobalAssignment(callFrame, ident.ustring());
 }
 
@@ -2431 +2437 @@
 
         vPC += OPCODE_LENGTH(op_resolve_base);
+        NEXT_INSTRUCTION();
+    }
+    DEFINE_OPCODE(op_ensure_property_exists) {
+        /* ensure_property_exists base(r) property(id)
+
+           Throws an exception if property does not exist on base
+         */
+        int base = vPC[1].u.operand;
+        int property = vPC[2].u.operand;
+        Identifier& ident = codeBlock->identifier(property);
+        
+        JSValue baseVal = callFrame->r(base).jsValue();
+        JSObject* baseObject = asObject(baseVal);
+        PropertySlot slot(baseVal);
+        if (!baseObject->getPropertySlot(callFrame, ident, slot)) {
+            exceptionValue = createErrorForInvalidGlobalAssignment(callFrame, ident.ustring());
+            goto vm_throw;
+        }
         NEXT_INSTRUCTION();
     }

trunk/JavaScriptCore/jit/JIT.cpp

@@ -303 +303 @@
         DEFINE_OP(op_resolve)
         DEFINE_OP(op_resolve_base)
+        DEFINE_OP(op_ensure_property_exists)
         DEFINE_OP(op_resolve_global)
         DEFINE_OP(op_resolve_global_dynamic)

trunk/JavaScriptCore/jit/JIT.h

@@ -812 +812 @@
         void emit_op_resolve(Instruction*);
         void emit_op_resolve_base(Instruction*);
+        void emit_op_ensure_property_exists(Instruction*);
         void emit_op_resolve_global(Instruction*, bool dynamic = false);
         void emit_op_resolve_global_dynamic(Instruction*);

trunk/JavaScriptCore/jit/JITOpcodes.cpp

@@ -645 +645 @@
 void JIT::emit_op_resolve_base(Instruction* currentInstruction)
 {
-    JITStubCall stubCall(this, cti_op_resolve_base);
+    JITStubCall stubCall(this, currentInstruction[3].u.operand ? cti_op_resolve_base_strict_put : cti_op_resolve_base);
+    stubCall.addArgument(ImmPtr(&m_codeBlock->identifier(currentInstruction[2].u.operand)));
+    stubCall.call(currentInstruction[1].u.operand);
+}
+
+void JIT::emit_op_ensure_property_exists(Instruction* currentInstruction)
+{
+    JITStubCall stubCall(this, cti_op_ensure_property_exists);
+    stubCall.addArgument(Imm32(currentInstruction[1].u.operand));
     stubCall.addArgument(ImmPtr(&m_codeBlock->identifier(currentInstruction[2].u.operand)));
     stubCall.call(currentInstruction[1].u.operand);

trunk/JavaScriptCore/jit/JITOpcodes32_64.cpp

@@ -738 +738 @@
 void JIT::emit_op_resolve_base(Instruction* currentInstruction)
 {
-    JITStubCall stubCall(this, cti_op_resolve_base);
+    JITStubCall stubCall(this, currentInstruction[3].u.operand ? cti_op_resolve_base_strict_put : cti_op_resolve_base);
+    stubCall.addArgument(ImmPtr(&m_codeBlock->identifier(currentInstruction[2].u.operand)));
+    stubCall.call(currentInstruction[1].u.operand);
+}
+
+void JIT::emit_op_ensure_property_exists(Instruction* currentInstruction)
+{
+    JITStubCall stubCall(this, cti_op_ensure_property_exists);
+    stubCall.addArgument(Imm32(currentInstruction[1].u.operand));
     stubCall.addArgument(ImmPtr(&m_codeBlock->identifier(currentInstruction[2].u.operand)));
     stubCall.call(currentInstruction[1].u.operand);

trunk/JavaScriptCore/jit/JITStubs.cpp

@@ -2698 +2698 @@
     STUB_INIT_STACK_FRAME(stackFrame);
 
-    return JSValue::encode(JSC::resolveBase(stackFrame.callFrame, stackFrame.args[0].identifier(), stackFrame.callFrame->scopeChain()));
-}
-
+    return JSValue::encode(JSC::resolveBase(stackFrame.callFrame, stackFrame.args[0].identifier(), stackFrame.callFrame->scopeChain(), false));
+}
+
+DEFINE_STUB_FUNCTION(EncodedJSValue, op_resolve_base_strict_put)
+{
+    STUB_INIT_STACK_FRAME(stackFrame);
+    JSValue base = JSC::resolveBase(stackFrame.callFrame, stackFrame.args[0].identifier(), stackFrame.callFrame->scopeChain(), true);
+    if (!base) {
+        stackFrame.globalData->exception = createErrorForInvalidGlobalAssignment(stackFrame.callFrame, stackFrame.args[0].identifier().ustring());
+        VM_THROW_EXCEPTION();
+    }
+    return JSValue::encode(base);
+}
+
+DEFINE_STUB_FUNCTION(EncodedJSValue, op_ensure_property_exists)
+{
+    STUB_INIT_STACK_FRAME(stackFrame);
+    JSValue base = stackFrame.callFrame->r(stackFrame.args[0].int32()).jsValue();
+    JSObject* object = asObject(base);
+    PropertySlot slot(object);
+    ASSERT(stackFrame.callFrame->codeBlock()->isStrictMode());
+    if (!object->getPropertySlot(stackFrame.callFrame, stackFrame.args[1].identifier(), slot)) {
+        stackFrame.globalData->exception = createErrorForInvalidGlobalAssignment(stackFrame.callFrame, stackFrame.args[1].identifier().ustring());
+        VM_THROW_EXCEPTION();
+    }
+
+    return JSValue::encode(base);
+}
+    
 DEFINE_STUB_FUNCTION(EncodedJSValue, op_resolve_skip)
 {

trunk/JavaScriptCore/jit/JITStubs.h

@@ -339 +339 @@
     EncodedJSValue JIT_STUB cti_op_resolve(STUB_ARGS_DECLARATION);
     EncodedJSValue JIT_STUB cti_op_resolve_base(STUB_ARGS_DECLARATION);
+    EncodedJSValue JIT_STUB cti_op_resolve_base_strict_put(STUB_ARGS_DECLARATION);
+    EncodedJSValue JIT_STUB cti_op_ensure_property_exists(STUB_ARGS_DECLARATION);
     EncodedJSValue JIT_STUB cti_op_resolve_global(STUB_ARGS_DECLARATION);
     EncodedJSValue JIT_STUB cti_op_resolve_global_dynamic(STUB_ARGS_DECLARATION);

trunk/JavaScriptCore/parser/JSParser.cpp

@@ -530 +530 @@
         return true;
     if (!m_syntaxAlreadyValidated) {
-        IdentifierSet writtenVariables;
-        scope->getUncapturedWrittenVariables(writtenVariables);
-        IdentifierSet::const_iterator end = writtenVariables.end();
-        for (IdentifierSet::const_iterator ptr = writtenVariables.begin(); ptr != end; ++ptr) {
-            PropertySlot slot(lexicalGlobalObject);
-            if (!lexicalGlobalObject->getPropertySlot(lexicalGlobalObject->globalExec(), Identifier(m_globalData, *ptr), slot))
-                return true;
-        }
         IdentifierSet deletedVariables;
         if (!scope->getDeletedVariables(deletedVariables))
             return true;
-        end = deletedVariables.end();
+        IdentifierSet::const_iterator end = deletedVariables.end();
         SymbolTable& globalEnvRecord = lexicalGlobalObject->symbolTable();
         for (IdentifierSet::const_iterator ptr = deletedVariables.begin(); ptr != end; ++ptr) {

trunk/JavaScriptCore/runtime/ExceptionHelpers.cpp

@@ -193 +193 @@
 }
 
+JSObject* createErrorForInvalidGlobalAssignment(ExecState* exec, const UString& propertyName)
+{
+    return createReferenceError(exec, makeUString("Strict mode forbids implicit creation of global property '", propertyName, "'"));
+}    
+
 } // namespace JSC

trunk/JavaScriptCore/runtime/ExceptionHelpers.h

@@ -55 +55 @@
     JSObject* createNotAnObjectError(ExecState*, JSNotAnObjectErrorStub*, unsigned bytecodeOffset, CodeBlock*);
     JSValue throwOutOfMemoryError(ExecState*);
+    JSObject* createErrorForInvalidGlobalAssignment(ExecState*, const UString&);
 
 } // namespace JSC

trunk/JavaScriptCore/runtime/Operations.h

@@ -461 +461 @@
     }
 
-    ALWAYS_INLINE JSValue resolveBase(CallFrame* callFrame, Identifier& property, ScopeChainNode* scopeChain)
+    ALWAYS_INLINE JSValue resolveBase(CallFrame* callFrame, Identifier& property, ScopeChainNode* scopeChain, bool isStrictPut)
     {
         ScopeChainIterator iter = scopeChain->begin();
@@ -473 +473 @@
         while (true) {
             base = *iter;
-            if (next == end || base->getPropertySlot(callFrame, property, slot))
+            if (next == end)
+                return isStrictPut ? JSValue() : base;
+            if (base->getPropertySlot(callFrame, property, slot))
                 return base;
 

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2010-10-17  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Sam Weinig.
+
+        Strict mode: Assignment that would create a global should be a late ReferenceError, not a syntax failure
+        https://bugs.webkit.org/show_bug.cgi?id=47788
+
+        Update test to check for the correct behaviour.
+
+        * fast/js/basic-strict-mode-expected.txt:
+        * fast/js/script-tests/basic-strict-mode.js:
+
 2010-10-17  Antonio Gomes  <agomes@rim.com>
 

trunk/LayoutTests/fast/js/basic-strict-mode-expected.txt

@@ -52 +52 @@
 PASS 'use strict'; (function (){ delete someDeclaredGlobal;}) threw exception SyntaxError: Parse error.
 PASS (function (){ 'use strict'; delete someDeclaredGlobal;}) threw exception SyntaxError: Parse error.
-PASS 'use strict'; someGlobal = 'Shouldn\'t be able to assign this.'; someGlobal; threw exception SyntaxError: Parse error.
+PASS 'use strict'; if (0) { someGlobal = 'Shouldn\'t be able to assign this.'; }; true; is true
+PASS 'use strict'; someGlobal = 'Shouldn\'t be able to assign this.';  threw exception ReferenceError: Strict mode forbids implicit creation of global property 'someGlobal'.
 PASS 'use strict'; eval('var introducedVariable = "FAIL: variable introduced into containing scope";'); introducedVariable threw exception ReferenceError: Can't find variable: introducedVariable.
 PASS 'use strict'; objectWithReadonlyProperty.prop = 'fail' threw exception TypeError: Attempted to assign to readonly property..

trunk/LayoutTests/fast/js/script-tests/basic-strict-mode.js

@@ -61 +61 @@
 shouldThrow("'use strict'; (function (){ delete someDeclaredGlobal;})");
 shouldThrow("(function (){ 'use strict'; delete someDeclaredGlobal;})");
-shouldThrow("'use strict'; someGlobal = 'Shouldn\\'t be able to assign this.'; someGlobal;");
+shouldBeTrue("'use strict'; if (0) { someGlobal = 'Shouldn\\'t be able to assign this.'; }; true;");
+shouldThrow("'use strict'; someGlobal = 'Shouldn\\'t be able to assign this.'; ");
 shouldThrow("'use strict'; eval('var introducedVariable = \"FAIL: variable introduced into containing scope\";'); introducedVariable");
 var objectWithReadonlyProperty = {};