Bug 19359: JavaScriptCore behaves differently from FF2/3 and IE when handling context...
authoroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 3 Aug 2008 09:58:21 +0000 (09:58 +0000)
committeroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 3 Aug 2008 09:58:21 +0000 (09:58 +0000)
<https://bugs.webkit.org/show_bug.cgi?id=19359>

Reviewed by Cameron Zwarich

Make our catch behave like Firefox and IE, we do this by using a StaticScopeObject
instead of a generic JSObject for the scope node.  We still don't make use of the
fact that we have a static scope inside the catch block, so the internal performance
of the catch block is not improved, even though technically it would be possible to
do so.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@35533 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
JavaScriptCore/ChangeLog
JavaScriptCore/VM/CodeBlock.cpp
JavaScriptCore/VM/CodeGenerator.cpp
JavaScriptCore/VM/CodeGenerator.h
JavaScriptCore/VM/Machine.cpp
JavaScriptCore/VM/Opcode.h
JavaScriptCore/kjs/JSStaticScopeObject.cpp
JavaScriptCore/kjs/JSStaticScopeObject.h
JavaScriptCore/kjs/nodes.cpp
LayoutTests/ChangeLog
LayoutTests/fast/js/resources/static-scope-object.js [new file with mode: 0644]
LayoutTests/fast/js/static-scope-object-expected.txt [new file with mode: 0644]
LayoutTests/fast/js/static-scope-object.html [new file with mode: 0644]

index bb2e7a6..0027721 100644 (file)
@@ -1,3 +1,33 @@
+2008-07-31  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Cameron Zwarich.
+
+        Bug 19359: JavaScriptCore behaves differently from FF2/3 and IE when handling context in catch statement
+        <https://bugs.webkit.org/show_bug.cgi?id=19359>
+
+        Make our catch behave like Firefox and IE, we do this by using a StaticScopeObject
+        instead of a generic JSObject for the scope node.  We still don't make use of the
+        fact that we have a static scope inside the catch block, so the internal performance
+        of the catch block is not improved, even though technically it would be possible to
+        do so.
+
+        * VM/CodeBlock.cpp:
+        (KJS::CodeBlock::dump):
+        * VM/CodeGenerator.cpp:
+        (KJS::CodeGenerator::emitPushNewScope):
+        * VM/CodeGenerator.h:
+        * VM/Machine.cpp:
+        (KJS::createExceptionScope):
+        (KJS::Machine::privateExecute):
+        * VM/Machine.h:
+        * VM/Opcode.h:
+        * kjs/JSStaticScopeObject.cpp:
+        (KJS::JSStaticScopeObject::toThisObject):
+        (KJS::JSStaticScopeObject::put):
+        * kjs/JSStaticScopeObject.h:
+        * kjs/nodes.cpp:
+        (KJS::TryNode::emitCode):
+
 2008-08-02  Rob Gowin  <robg@gowin.net>
 
         Reviewed by Eric Seidel.
index 5c7665f..0f46426 100644 (file)
@@ -633,6 +633,13 @@ void CodeBlock::dump(ExecState* exec, const Vector<Instruction>::const_iterator&
             printf("[%4d] pop_scope\n", location);
             break;
         }
+        case op_push_new_scope: {
+            int r0 = (++it)->u.operand;
+            int id0 = (++it)->u.operand;
+            int r1 = (++it)->u.operand;
+            printf("[%4d] push_new_scope \t%s, %s, %s\n", location, registerName(r0).c_str(), idName(id0, identifiers[id0]).c_str(), registerName(r1).c_str());
+            break;
+        }
         case op_jmp_scopes: {
             int scopeDelta = (++it)->u.operand;
             int offset = (++it)->u.operand;
index abb5221..f14e493 100644 (file)
@@ -1150,6 +1150,20 @@ void CodeGenerator::emitSubroutineReturn(RegisterID* retAddrSrc)
     instructions().append(retAddrSrc->index());
 }
 
+void CodeGenerator::emitPushNewScope(RegisterID* dst, Identifier& property, RegisterID* value)
+{
+    m_codeBlock->needsFullScopeChain = true;
+    ControlFlowContext context;
+    context.isFinallyBlock = false;
+    m_scopeContextStack.append(context);
+    m_dynamicScopeDepth++;
+    
+    emitOpcode(op_push_new_scope);
+    instructions().append(dst->index());
+    instructions().append(addConstant(property));
+    instructions().append(value->index());
+}
+
 void CodeGenerator::beginSwitch(RegisterID* scrutineeRegister, SwitchInfo::SwitchType type)
 {
     SwitchInfo info = { instructions().size(), type };
index af1e44e..0f53c81 100644 (file)
@@ -296,6 +296,7 @@ namespace KJS {
         RegisterID* emitCatch(RegisterID*, LabelID* start, LabelID* end);
         void emitThrow(RegisterID* exc) { emitUnaryNoDstOp(op_throw, exc); }
         RegisterID* emitNewError(RegisterID* dst, ErrorType type, JSValue* message);
+        void emitPushNewScope(RegisterID* dst, Identifier& property, RegisterID* value);
 
         RegisterID* emitPushScope(RegisterID* scope);
         void emitPopScope();
index ebb9f93..19a520f 100644 (file)
@@ -40,6 +40,7 @@
 #include "JSFunction.h"
 #include "JSNotAnObject.h"
 #include "JSPropertyNameIterator.h"
+#include "JSStaticScopeObject.h"
 #include "JSString.h"
 #include "ObjectPrototype.h"
 #include "Parser.h"
@@ -998,6 +999,16 @@ static int32_t offsetForStringSwitch(StringJumpTable& jumpTable, JSValue* scruti
     return loc->second;
 }
 
+static NEVER_INLINE ScopeChainNode* createExceptionScope(ExecState* exec, CodeBlock* codeBlock, const Instruction* vPC, Register* r, ScopeChainNode* scopeChain)
+{
+    int dst = (++vPC)->u.operand;
+    Identifier& property = codeBlock->identifiers[(++vPC)->u.operand];
+    JSValue* value = r[(++vPC)->u.operand].jsValue(exec);
+    JSObject* scope = new (exec) JSStaticScopeObject(property, value, DontDelete);
+    r[dst] = scope;
+    return scopeChain->push(scope);
+}
+
 JSValue* Machine::privateExecute(ExecutionFlag flag, ExecState* exec, RegisterFile* registerFile, Register* r, ScopeChainNode* scopeChain, CodeBlock* codeBlock, JSValue** exception)
 {
     // One-time initialization of our address tables. We have to put this code
@@ -2602,6 +2613,24 @@ JSValue* Machine::privateExecute(ExecutionFlag flag, ExecState* exec, RegisterFi
         vPC += target;
         NEXT_OPCODE;
     }
+#if HAVE(COMPUTED_GOTO)
+    // Appease GCC
+    goto *(&&skip_new_scope);
+#endif
+    BEGIN_OPCODE(op_push_new_scope) {
+        /* new_scope dst(r) property(id) value(r)
+         
+           Constructs a new StaticScopeObject with property set to value.  That scope
+           object is then pushed onto the ScopeChain.  The scope object is then stored
+           in dst for GC.
+         */
+        setScopeChain(exec, scopeChain, createExceptionScope(exec, codeBlock, vPC, r, scopeChain));
+        vPC += 4;
+        NEXT_OPCODE;
+    }
+#if HAVE(COMPUTED_GOTO)
+    skip_new_scope:
+#endif
     BEGIN_OPCODE(op_catch) {
         /* catch ex(r)
 
index d6e9443..5ca935a 100644 (file)
@@ -121,6 +121,7 @@ namespace KJS {
         \
         macro(op_push_scope) \
         macro(op_pop_scope) \
+        macro(op_push_new_scope) \
         \
         macro(op_catch) \
         macro(op_throw) \
index 1cdc1ce..eb3ddc8 100644 (file)
 
 namespace KJS {
 
+JSObject* JSStaticScopeObject::toThisObject(ExecState* exec) const
+{
+    return exec->globalThisValue();
+}
+
+void JSStaticScopeObject::put(ExecState*, const Identifier& propertyName, JSValue* value)
+{
+    if (symbolTablePut(propertyName, value))
+        return;
+    
+    ASSERT_NOT_REACHED();
+}
+
 void JSStaticScopeObject::putWithAttributes(ExecState*, const Identifier& propertyName, JSValue* value, unsigned attributes)
 {
     if (symbolTablePutWithAttributes(propertyName, value, attributes))
index 6cde320..3499571 100644 (file)
@@ -53,8 +53,10 @@ namespace KJS{
         }
         virtual ~JSStaticScopeObject();
         bool isDynamicScope() const;
+        virtual JSObject* toThisObject(ExecState*) const;
         virtual bool getOwnPropertySlot(ExecState*, const Identifier&, PropertySlot&);
         virtual bool getOwnPropertySlot(ExecState*, const Identifier&, PropertySlot&, bool& slotIsWriteable);
+        virtual void put(ExecState*, const Identifier&, JSValue*);
         void putWithAttributes(ExecState*, const Identifier&, JSValue*, unsigned attributes);
     };
 
index d7a22af..0db8087 100644 (file)
@@ -1604,10 +1604,7 @@ RegisterID* TryNode::emitCode(CodeGenerator& generator, RegisterID* dst)
         RefPtr<LabelID> handlerEndLabel = generator.newLabel();
         generator.emitJump(handlerEndLabel.get());
         RefPtr<RegisterID> exceptionRegister = generator.emitCatch(generator.newTemporary(), tryStartLabel.get(), tryEndLabel.get());
-        RefPtr<RegisterID> newScope = generator.emitNewObject(generator.newTemporary()); // scope must be protected until popped
-        generator.emitPutById(newScope.get(), m_exceptionIdent, exceptionRegister.get());
-        exceptionRegister = 0; // Release register used for temporaries
-        generator.emitPushScope(newScope.get());
+        generator.emitPushNewScope(exceptionRegister.get(), m_exceptionIdent, exceptionRegister.get());
         generator.emitNode(dst, m_catchBlock.get());
         generator.emitPopScope();
         generator.emitLabel(handlerEndLabel.get());
index 9075bca..e4aa5a2 100644 (file)
@@ -1,3 +1,14 @@
+2008-07-31  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Cameron Zwarich.
+
+        Add tests to cover behaviour of "this" when calling named function
+        expressions and functions thrown as exceptions.
+
+        * fast/js/resources/static-scope-object.js: Added.
+        * fast/js/static-scope-object-expected.txt: Added.
+        * fast/js/static-scope-object.html: Added.
+
 2008-07-31  David Kilzer  <ddkilzer@apple.com>
 
         Updated test results for DumpRenderTree fix in r35497
diff --git a/LayoutTests/fast/js/resources/static-scope-object.js b/LayoutTests/fast/js/resources/static-scope-object.js
new file mode 100644 (file)
index 0000000..0c4fec1
--- /dev/null
@@ -0,0 +1,22 @@
+description('This test ensures that the correct "this" object is used when calling named function expressions or exceptions.');
+
+this.toString = function() { return "the global object" };
+var globalObject = this;
+
+function namedFunctionExpression() {
+    return function f(i) { if (i > 0) return this; return f(1); }(0);
+}
+
+shouldBe("namedFunctionExpression()", 'globalObject');
+
+function throwingFunctionAsException() {
+    try {
+        throw function(){ return this; }
+    } catch(e) {
+        return e();
+    }
+}
+
+shouldBe("throwingFunctionAsException()", 'globalObject');
+
+successfullyParsed = true;
diff --git a/LayoutTests/fast/js/static-scope-object-expected.txt b/LayoutTests/fast/js/static-scope-object-expected.txt
new file mode 100644 (file)
index 0000000..3832fcc
--- /dev/null
@@ -0,0 +1,11 @@
+This test ensures that the correct "this" object is used when calling named function expressions or exceptions.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS namedFunctionExpression() is globalObject
+PASS throwingFunctionAsException() is globalObject
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/js/static-scope-object.html b/LayoutTests/fast/js/static-scope-object.html
new file mode 100644 (file)
index 0000000..5ed71b7
--- /dev/null
@@ -0,0 +1,13 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<link rel="stylesheet" href="resources/js-test-style.css">
+<script src="resources/js-test-pre.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+<script src="resources/static-scope-object.js"></script>
+<script src="resources/js-test-post.js"></script>
+</body>
+</html>