- Revision
- 91164
- Author
- [email protected]
- Date
- 2011-07-16 22:04:36 -0700 (Sat, 16 Jul 2011)
Log 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/_javascript_Core:
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):
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (91163 => 91164)
--- trunk/LayoutTests/ChangeLog 2011-07-17 04:56:20 UTC (rev 91163)
+++ trunk/LayoutTests/ChangeLog 2011-07-17 05:04:36 UTC (rev 91164)
@@ -1,3 +1,16 @@
+2011-07-16 Gavin Barraclough <[email protected]>
+
+ 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 <[email protected]>
REGRESSION: Pressing return in a particular document sends the cursor to the end of the document.
Modified: trunk/LayoutTests/fast/js/read-modify-eval-expected.txt (91163 => 91164)
--- trunk/LayoutTests/fast/js/read-modify-eval-expected.txt 2011-07-17 04:56:20 UTC (rev 91163)
+++ trunk/LayoutTests/fast/js/read-modify-eval-expected.txt 2011-07-17 05:04:36 UTC (rev 91164)
@@ -18,6 +18,8 @@
PASS preDecTest(); is true
PASS postIncTest(); is true
PASS postDecTest(); is true
+PASS primitiveThisTest.call(1); is true
+PASS strictThisTest.call(1); is true
PASS successfullyParsed is true
TEST COMPLETE
Modified: trunk/LayoutTests/fast/js/script-tests/read-modify-eval.js (91163 => 91164)
--- trunk/LayoutTests/fast/js/script-tests/read-modify-eval.js 2011-07-17 04:56:20 UTC (rev 91163)
+++ trunk/LayoutTests/fast/js/script-tests/read-modify-eval.js 2011-07-17 05:04:36 UTC (rev 91164)
@@ -107,6 +107,24 @@
return x.value == -1;
}
+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();');
shouldBeTrue('addTest();');
@@ -124,4 +142,7 @@
shouldBeTrue('postIncTest();');
shouldBeTrue('postDecTest();');
+shouldBeTrue('primitiveThisTest.call(1);');
+shouldBeTrue('strictThisTest.call(1);');
+
successfullyParsed = true;
Modified: trunk/Source/_javascript_Core/ChangeLog (91163 => 91164)
--- trunk/Source/_javascript_Core/ChangeLog 2011-07-17 04:56:20 UTC (rev 91163)
+++ trunk/Source/_javascript_Core/ChangeLog 2011-07-17 05:04:36 UTC (rev 91164)
@@ -1,3 +1,38 @@
+2011-07-16 Gavin Barraclough <[email protected]>
+
+ 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 <[email protected]>
DFG JIT compare/branch code emits is-integer tests even when a value is
Modified: trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp (91163 => 91164)
--- trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp 2011-07-17 04:56:20 UTC (rev 91163)
+++ trunk/Source/_javascript_Core/bytecompiler/BytecodeGenerator.cpp 2011-07-17 05:04:36 UTC (rev 91164)
@@ -413,11 +413,9 @@
emitOpcode(op_create_this);
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());
}
}
Modified: trunk/Source/_javascript_Core/interpreter/Interpreter.cpp (91163 => 91164)
--- trunk/Source/_javascript_Core/interpreter/Interpreter.cpp 2011-07-17 04:56:20 UTC (rev 91163)
+++ trunk/Source/_javascript_Core/interpreter/Interpreter.cpp 2011-07-17 05:04:36 UTC (rev 91164)
@@ -1240,7 +1240,7 @@
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();
if (*profiler)
@@ -4621,7 +4621,7 @@
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));
vPC += OPCODE_LENGTH(op_convert_this);
Modified: trunk/Source/_javascript_Core/jit/JITStubs.cpp (91163 => 91164)
--- trunk/Source/_javascript_Core/jit/JITStubs.cpp 2011-07-17 04:56:20 UTC (rev 91163)
+++ trunk/Source/_javascript_Core/jit/JITStubs.cpp 2011-07-17 05:04:36 UTC (rev 91164)
@@ -1280,7 +1280,7 @@
JSValue v1 = stackFrame.args[0].jsValue();
CallFrame* callFrame = stackFrame.callFrame;
- ASSERT(!v1.isCell() || v1.isString());
+ ASSERT(v1.isPrimitive());
JSObject* result = v1.toThisObject(callFrame);
CHECK_FOR_EXCEPTION_AT_END();
Modified: trunk/Source/_javascript_Core/runtime/JSCell.h (91163 => 91164)
--- trunk/Source/_javascript_Core/runtime/JSCell.h 2011-07-17 04:56:20 UTC (rev 91163)
+++ trunk/Source/_javascript_Core/runtime/JSCell.h 2011-07-17 05:04:36 UTC (rev 91164)
@@ -208,6 +208,11 @@
return isCell() && asCell()->isString();
}
+ inline bool JSValue::isPrimitive() const
+ {
+ return !isCell() || asCell()->isString();
+ }
+
inline bool JSValue::isGetterSetter() const
{
return isCell() && asCell()->isGetterSetter();
Modified: trunk/Source/_javascript_Core/runtime/JSValue.h (91163 => 91164)
--- trunk/Source/_javascript_Core/runtime/JSValue.h 2011-07-17 04:56:20 UTC (rev 91163)
+++ trunk/Source/_javascript_Core/runtime/JSValue.h 2011-07-17 05:04:36 UTC (rev 91164)
@@ -152,6 +152,7 @@
bool isBoolean() const;
bool isNumber() const;
bool isString() const;
+ bool isPrimitive() const;
bool isGetterSetter() const;
bool isObject() const;
bool inherits(const ClassInfo*) const;
