Changeset 91164 in webkit

Timestamp:

Jul 16, 2011 10:04:36 PM (13 years ago)

Author:

barraclough@apple.com

Message:

​https://bugs.webkit.org/show_bug.cgi?id=64657
Converted this value not preserved when accessed via direct eval.

Reviewed by Oliver Hunt.

Source/JavaScriptCore:

Upon entry into a non-strict function, primitive this values should be boxed as Object types
(or substituted with the global object) - which is done by op_convert_this. However we only
do so where this is used lexically within the function (we omit the conversion op if not).
The problem comes if a direct eval (running within the function's scope) accesses the this
value.

We are safe in the case of a single eval, since the this object will be converted within
callEval, however the converted value is not preserved, and a new wrapper object is allocated
each time eval is invoked. This is inefficient and incorrect, since any changes to the wrapper
object will be lost between eval statements.

• bytecompiler/BytecodeGenerator.cpp:

(JSC::BytecodeGenerator::BytecodeGenerator):

• If a function uses eval, we always need to convert this.

• interpreter/Interpreter.cpp:

(JSC::Interpreter::execute):

• Don't convert primitive values here - this is too late!

(JSC::Interpreter::privateExecute):

• Changed op_convert_this to call new isPrimitive method.

• jit/JITStubs.cpp:

(JSC::DEFINE_STUB_FUNCTION):

• Changed op_convert_this to call new isPrimitive method.

• runtime/JSCell.h:

(JSC::JSCell::JSValue::isPrimitive):

• Added JSValue::isPrimitive.

• runtime/JSValue.h:
  • Added JSValue::isPrimitive.

LayoutTests:

Added test case.

• fast/js/read-modify-eval-expected.txt:
• fast/js/script-tests/read-modify-eval.js:

(primitiveThisTest):

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/js/read-modify-eval-expected.txt (modified) (1 diff)
• LayoutTests/fast/js/script-tests/read-modify-eval.js (modified) (2 diffs)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp (modified) (1 diff)
• Source/JavaScriptCore/interpreter/Interpreter.cpp (modified) (2 diffs)
• Source/JavaScriptCore/jit/JITStubs.cpp (modified) (1 diff)
• Source/JavaScriptCore/runtime/JSCell.h (modified) (1 diff)
• Source/JavaScriptCore/runtime/JSValue.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2011-07-16  Gavin Barraclough  <barraclough@apple.com>
+
+        https://bugs.webkit.org/show_bug.cgi?id=64657
+        Converted this value not preserved when accessed via direct eval.
+
+        Reviewed by Oliver Hunt.
+
+        Added test case.
+
+        * fast/js/read-modify-eval-expected.txt:
+        * fast/js/script-tests/read-modify-eval.js:
+        (primitiveThisTest):
+
 2011-07-16  Enrica Casucci  <enrica@apple.com>
 

trunk/LayoutTests/fast/js/read-modify-eval-expected.txt

@@ -19 +19 @@
 PASS postIncTest(); is true
 PASS postDecTest(); is true
+PASS primitiveThisTest.call(1); is true
+PASS strictThisTest.call(1); is true
 PASS successfullyParsed is true
 

trunk/LayoutTests/fast/js/script-tests/read-modify-eval.js

@@ -108 +108 @@
 }
 
+function primitiveThisTest()
+{
+    // Test that conversion of this is persistant over multiple calls to eval,
+    // even where 'this' is not directly used within the function.
+    eval('this.value = "Seekrit message";');
+    return eval('this.value') === "Seekrit message";
+}
+
+function strictThisTest()
+{
+    // In a strict mode function primitive this values are not converted, so
+    // the property access in the first eval is writing a value to a temporary
+    // object, and should not be observed by the second eval.
+    "use strict";
+    eval('this.value = "Seekrit message";');
+    return eval('this.value') === undefined;
+}
+
 shouldBeTrue('multTest();');
 shouldBeTrue('divTest();');
@@ -125 +143 @@
 shouldBeTrue('postDecTest();');
 
+shouldBeTrue('primitiveThisTest.call(1);');
+shouldBeTrue('strictThisTest.call(1);');
+
 successfullyParsed = true;

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2011-07-16  Gavin Barraclough  <barraclough@apple.com>
+
+        https://bugs.webkit.org/show_bug.cgi?id=64657
+        Converted this value not preserved when accessed via direct eval.
+
+        Reviewed by Oliver Hunt.
+
+        Upon entry into a non-strict function, primitive this values should be boxed as Object types
+        (or substituted with the global object) - which is done by op_convert_this. However we only
+        do so where this is used lexically within the function (we omit the conversion op if not).
+        The problem comes if a direct eval (running within the function's scope) accesses the this
+        value.
+
+        We are safe in the case of a single eval, since the this object will be converted within
+        callEval, however the converted value is not preserved, and a new wrapper object is allocated
+        each time eval is invoked. This is inefficient and incorrect, since any changes to the wrapper
+        object will be lost between eval statements.
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::BytecodeGenerator):
+            - If a function uses eval, we always need to convert this.
+        * interpreter/Interpreter.cpp:
+        (JSC::Interpreter::execute):
+            - Don't convert primitive values here - this is too late!
+        (JSC::Interpreter::privateExecute):
+            - Changed op_convert_this to call new isPrimitive method.
+        * jit/JITStubs.cpp:
+        (JSC::DEFINE_STUB_FUNCTION):
+            - Changed op_convert_this to call new isPrimitive method.
+        * runtime/JSCell.h:
+        (JSC::JSCell::JSValue::isPrimitive):
+            - Added JSValue::isPrimitive.
+        * runtime/JSValue.h:
+            - Added JSValue::isPrimitive.
+
 2011-07-16  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

@@ -414 +414 @@
         instructions().append(m_thisRegister.index());
         instructions().append(funcProto->index());
-    } else if (functionBody->usesThis() || m_shouldEmitDebugHooks) {
-        if (!codeBlock->isStrictMode()) {
-            emitOpcode(op_convert_this);
-            instructions().append(m_thisRegister.index());
-        }
+    } else if (!codeBlock->isStrictMode() && (functionBody->usesThis() || codeBlock->usesEval() || m_shouldEmitDebugHooks)) {
+        emitOpcode(op_convert_this);
+        instructions().append(m_thisRegister.index());
     }
 }

trunk/Source/JavaScriptCore/interpreter/Interpreter.cpp

@@ -1241 +1241 @@
     ASSERT(codeBlock->m_numParameters == 1); // 1 parameter for 'this'.
     newCallFrame->init(codeBlock, 0, scopeChain, callFrame->addHostCallFrameFlag(), codeBlock->m_numParameters, 0);
-    newCallFrame->uncheckedR(newCallFrame->hostThisRegister()) = thisValue.toThisObject(newCallFrame);
+    newCallFrame->uncheckedR(newCallFrame->hostThisRegister()) = thisValue;
 
     Profiler** profiler = Profiler::enabledProfilerReference();
@@ -4622 +4622 @@
         int thisRegister = vPC[1].u.operand;
         JSValue thisVal = callFrame->r(thisRegister).jsValue();
-        if (!thisVal.isCell() || thisVal.isString())
+        if (thisVal.isPrimitive())
             callFrame->uncheckedR(thisRegister) = JSValue(thisVal.toThisObject(callFrame));
 

trunk/Source/JavaScriptCore/jit/JITStubs.cpp

@@ -1281 +1281 @@
     CallFrame* callFrame = stackFrame.callFrame;
 
-    ASSERT(!v1.isCell() || v1.isString());
+    ASSERT(v1.isPrimitive());
 
     JSObject* result = v1.toThisObject(callFrame);

trunk/Source/JavaScriptCore/runtime/JSCell.h

@@ -209 +209 @@
     }
 
+    inline bool JSValue::isPrimitive() const
+    {
+        return !isCell() || asCell()->isString();
+    }
+
     inline bool JSValue::isGetterSetter() const
     {

trunk/Source/JavaScriptCore/runtime/JSValue.h

@@ -153 +153 @@
         bool isNumber() const;
         bool isString() const;
+        bool isPrimitive() const;
         bool isGetterSetter() const;
         bool isObject() const;