Incorrect inequality for checking whether a statement is within bounds of a handler
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Dec 2012 07:44:01 +0000 (07:44 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Dec 2012 07:44:01 +0000 (07:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=104313
<rdar://problem/12808934>

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

The most relevant change is in handlerForBytecodeOffset(), which fixes the inequality
used for checking whether a handler is pertinent to the current instruction. '<' is
correct, but '<=' isn't, since the 'end' is not inclusive.

Also found, and addressed, a benign goof in how the finally inliner works: sometimes
we will have end > start. This falls out naturally from how the inliner works and how
we pop scopes in the bytecompiler, but it's sufficiently surprising that, to avoid any
future confusion, I added a comment and some code to prune those handlers out. Because
of how the handler resolution works, these handlers would have been skipped anyway.

Also made various fixes to debugging code, which was necessary for tracking this down.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::dumpBytecode):
(JSC::CodeBlock::handlerForBytecodeOffset):
* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::generate):
* bytecompiler/Label.h:
(JSC::Label::bind):
* interpreter/Interpreter.cpp:
(JSC::Interpreter::throwException):
* llint/LLIntExceptions.cpp:
(JSC::LLInt::interpreterThrowInCaller):
(JSC::LLInt::returnToThrow):
(JSC::LLInt::callToThrow):
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
(JSC::LLInt::handleHostCall):

LayoutTests:

* fast/js/jsc-test-list:
* fast/js/script-tests/try-catch-try-try-catch-try-finally-return-catch-finally.js: Added.
(foo):
* fast/js/try-catch-try-try-catch-try-finally-return-catch-finally-expected.txt: Added.
* fast/js/try-catch-try-try-catch-try-finally-return-catch-finally.html: Added.

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/js/jsc-test-list
LayoutTests/fast/js/script-tests/try-catch-try-try-catch-try-finally-return-catch-finally.js [new file with mode: 0644]
LayoutTests/fast/js/try-catch-try-try-catch-try-finally-return-catch-finally-expected.txt [new file with mode: 0644]
LayoutTests/fast/js/try-catch-try-try-catch-try-finally-return-catch-finally.html [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Source/JavaScriptCore/bytecompiler/Label.h
Source/JavaScriptCore/interpreter/Interpreter.cpp
Source/JavaScriptCore/llint/LLIntExceptions.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp

index 5d2ab975b101dcd8851f1807ab9e0bb03455d3cb..cdeb793afaf6f4d02f2c3ce41bc24f8ccb2fe550 100644 (file)
@@ -1,3 +1,17 @@
+2012-12-06  Filip Pizlo  <fpizlo@apple.com>
+
+        Incorrect inequality for checking whether a statement is within bounds of a handler
+        https://bugs.webkit.org/show_bug.cgi?id=104313
+        <rdar://problem/12808934>
+
+        Reviewed by Geoffrey Garen.
+
+        * fast/js/jsc-test-list:
+        * fast/js/script-tests/try-catch-try-try-catch-try-finally-return-catch-finally.js: Added.
+        (foo):
+        * fast/js/try-catch-try-try-catch-try-finally-return-catch-finally-expected.txt: Added.
+        * fast/js/try-catch-try-try-catch-try-finally-return-catch-finally.html: Added.
+
 2012-12-06  Noel Gordon  <noel.gordon@gmail.com>
 
         Unreviewed Gardening. More http://trac.webkit.org/changeset/136910. Add Chromium Linux results. 
index 5ae4e10632ef1266111ad8aa283dfb5308fe86b6..7cfc11f355c35e9c87e56a9b581f485d0f7ad305 100644 (file)
@@ -338,6 +338,7 @@ fast/js/toString-for-var-decl
 fast/js/toString-number-dot-expr
 fast/js/toString-prefix-postfix-preserve-parens
 fast/js/toString-recursion
+fast/js/try-catch-try-try-catch-try-finally-return-catch-finally
 fast/js/try-try-return-finally-finally
 fast/js/typeof-codegen-crash
 fast/js/typeof-constant-string
diff --git a/LayoutTests/fast/js/script-tests/try-catch-try-try-catch-try-finally-return-catch-finally.js b/LayoutTests/fast/js/script-tests/try-catch-try-try-catch-try-finally-return-catch-finally.js
new file mode 100644 (file)
index 0000000..003141b
--- /dev/null
@@ -0,0 +1,30 @@
+description(
+"Tests what would happen if you a throwing operation at the beginning of a finally blow that gets inlined inside a complicated catch/finally stack. The correct outcome is for this test to not crash during exception throwing."
+);
+
+function foo() {
+    try{
+        N
+    } catch(x) {
+        try {
+            try {
+                w
+            } catch(x) {
+                try {
+                } finally {
+                    return
+                }
+            }
+        } catch (a) {
+        }
+    } finally {
+        z
+    }
+}
+
+try {
+    foo();
+} catch (e) {
+    testPassed("It worked.");
+}
+
diff --git a/LayoutTests/fast/js/try-catch-try-try-catch-try-finally-return-catch-finally-expected.txt b/LayoutTests/fast/js/try-catch-try-try-catch-try-finally-return-catch-finally-expected.txt
new file mode 100644 (file)
index 0000000..32559a9
--- /dev/null
@@ -0,0 +1,10 @@
+Tests what would happen if you a throwing operation at the beginning of a finally blow that gets inlined inside a complicated catch/finally stack. The correct outcome is for this test to not crash during exception throwing.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS It worked.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/js/try-catch-try-try-catch-try-finally-return-catch-finally.html b/LayoutTests/fast/js/try-catch-try-try-catch-try-finally-return-catch-finally.html
new file mode 100644 (file)
index 0000000..95344ec
--- /dev/null
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="script-tests/try-catch-try-try-catch-try-finally-return-catch-finally.js"></script>
+<script src="resources/js-test-post.js"></script>
+</body>
+</html>
index 3f9bc482a83c920c9b04fc8bf9a5c334baf48ae3..df8a0874006564cceb393e78f3831380440efdf4 100644 (file)
@@ -1,3 +1,40 @@
+2012-12-06  Filip Pizlo  <fpizlo@apple.com>
+
+        Incorrect inequality for checking whether a statement is within bounds of a handler
+        https://bugs.webkit.org/show_bug.cgi?id=104313
+        <rdar://problem/12808934>
+
+        Reviewed by Geoffrey Garen.
+
+        The most relevant change is in handlerForBytecodeOffset(), which fixes the inequality
+        used for checking whether a handler is pertinent to the current instruction. '<' is
+        correct, but '<=' isn't, since the 'end' is not inclusive.
+        
+        Also found, and addressed, a benign goof in how the finally inliner works: sometimes
+        we will have end > start. This falls out naturally from how the inliner works and how
+        we pop scopes in the bytecompiler, but it's sufficiently surprising that, to avoid any
+        future confusion, I added a comment and some code to prune those handlers out. Because
+        of how the handler resolution works, these handlers would have been skipped anyway.
+        
+        Also made various fixes to debugging code, which was necessary for tracking this down.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::dumpBytecode):
+        (JSC::CodeBlock::handlerForBytecodeOffset):
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::generate):
+        * bytecompiler/Label.h:
+        (JSC::Label::bind):
+        * interpreter/Interpreter.cpp:
+        (JSC::Interpreter::throwException):
+        * llint/LLIntExceptions.cpp:
+        (JSC::LLInt::interpreterThrowInCaller):
+        (JSC::LLInt::returnToThrow):
+        (JSC::LLInt::callToThrow):
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+        (JSC::LLInt::handleHostCall):
+
 2012-12-06  Rick Byers  <rbyers@chromium.org>
 
         CSS cursor property should support webkit-image-set
index 5d30e20f57794e668797725b65f684f739225ff1..16403f904a751bcc8ef25a2b44a05943899025b6 100644 (file)
@@ -570,7 +570,7 @@ void CodeBlock::dumpBytecode(PrintStream& out)
         out.printf("\nException Handlers:\n");
         unsigned i = 0;
         do {
-            out.printf("\t %d: { start: [%4d] end: [%4d] target: [%4d] }\n", i + 1, m_rareData->m_exceptionHandlers[i].start, m_rareData->m_exceptionHandlers[i].end, m_rareData->m_exceptionHandlers[i].target);
+            out.printf("\t %d: { start: [%4d] end: [%4d] target: [%4d] depth: [%4d] }\n", i + 1, m_rareData->m_exceptionHandlers[i].start, m_rareData->m_exceptionHandlers[i].end, m_rareData->m_exceptionHandlers[i].target, m_rareData->m_exceptionHandlers[i].scopeDepth);
             ++i;
         } while (i < m_rareData->m_exceptionHandlers.size());
     }
@@ -2476,7 +2476,7 @@ HandlerInfo* CodeBlock::handlerForBytecodeOffset(unsigned bytecodeOffset)
     for (size_t i = 0; i < exceptionHandlers.size(); ++i) {
         // Handlers are ordered innermost first, so the first handler we encounter
         // that contains the source address is the correct handler to use.
-        if (exceptionHandlers[i].start <= bytecodeOffset && exceptionHandlers[i].end >= bytecodeOffset)
+        if (exceptionHandlers[i].start <= bytecodeOffset && exceptionHandlers[i].end > bytecodeOffset)
             return &exceptionHandlers[i];
     }
 
index 35976257b4b25c28ffca2c8c33635cc82f459a27..5072416965b738ff8a2f4c377486c2c628429438 100644 (file)
@@ -157,10 +157,38 @@ ParserError BytecodeGenerator::generate()
     
     for (unsigned i = 0; i < m_tryRanges.size(); ++i) {
         TryRange& range = m_tryRanges[i];
+        int start = range.start->bind();
+        int end = range.end->bind();
+        
+        // This will happen for empty try blocks and for some cases of finally blocks:
+        //
+        // try {
+        //    try {
+        //    } finally {
+        //        return 42;
+        //        // *HERE*
+        //    }
+        // } finally {
+        //    print("things");
+        // }
+        //
+        // The return will pop scopes to execute the outer finally block. But this includes
+        // popping the try context for the inner try. The try context is live in the fall-through
+        // part of the finally block not because we will emit a handler that overlaps the finally,
+        // but because we haven't yet had a chance to plant the catch target. Then when we finish
+        // emitting code for the outer finally block, we repush the try contex, this time with a
+        // new start index. But that means that the start index for the try range corresponding
+        // to the inner-finally-following-the-return (marked as "*HERE*" above) will be greater
+        // than the end index of the try block. This is harmless since end < start handlers will
+        // never get matched in our logic, but we do the runtime a favor and choose to not emit
+        // such handlers at all.
+        if (end <= start)
+            continue;
+        
         ASSERT(range.tryData->targetScopeDepth != UINT_MAX);
         UnlinkedHandlerInfo info = {
-            static_cast<uint32_t>(range.start->bind(0, 0)), static_cast<uint32_t>(range.end->bind(0, 0)),
-            static_cast<uint32_t>(range.tryData->target->bind(0, 0)),
+            static_cast<uint32_t>(start), static_cast<uint32_t>(end),
+            static_cast<uint32_t>(range.tryData->target->bind()),
             range.tryData->targetScopeDepth
         };
         m_codeBlock->addExceptionHandler(info);
index 21fa46309a2f2875a28da6483bb4a56fe1ee0dad..8b444de914be29dd47c51633c5b011b5e0942cc6 100644 (file)
@@ -66,6 +66,12 @@ namespace JSC {
         int refCount() const { return m_refCount; }
 
         bool isForward() const { return m_location == invalidLocation; }
+        
+        int bind()
+        {
+            ASSERT(!isForward());
+            return bind(0, 0);
+        }
 
     private:
         typedef Vector<std::pair<int, int>, 8> JumpVector;
index c2c944fcfcaed9fa2856933ee603e0f4bf29a9f4..f0ef0e39400cc9d3bc7a872850b25c25f106ffff 100644 (file)
@@ -786,9 +786,12 @@ NEVER_INLINE HandlerInfo* Interpreter::throwException(CallFrame*& callFrame, JSV
     JSScope* scope = callFrame->scope();
     int scopeDelta = 0;
     if (!codeBlock->needsFullScopeChain() || codeBlock->codeType() != FunctionCode 
-        || callFrame->uncheckedR(codeBlock->activationRegister()).jsValue())
-        scopeDelta = depth(codeBlock, scope) - handler->scopeDepth;
-    ASSERT(scopeDelta >= 0);
+        || callFrame->uncheckedR(codeBlock->activationRegister()).jsValue()) {
+        int currentDepth = depth(codeBlock, scope);
+        int targetDepth = handler->scopeDepth;
+        scopeDelta = currentDepth - targetDepth;
+        ASSERT(scopeDelta >= 0);
+    }
     while (scopeDelta--)
         scope = scope->next();
     callFrame->setScope(scope);
index 5e6bba365e97e2cbc5efee86fb3c06efa4c36d44..17c15aa51272155d281ff50d7cdc04feff236c9c 100644 (file)
@@ -50,7 +50,7 @@ void interpreterThrowInCaller(ExecState* exec, ReturnAddressPtr pc)
     JSGlobalData* globalData = &exec->globalData();
     NativeCallFrameTracer tracer(globalData, exec);
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Throwing exception %s.\n", globalData->exception.description());
+    dataLog("Throwing exception ", globalData->exception, ".\n");
 #endif
     fixupPCforExceptionIfNeeded(exec);
     genericThrow(
@@ -69,7 +69,7 @@ Instruction* returnToThrow(ExecState* exec, Instruction* pc)
     JSGlobalData* globalData = &exec->globalData();
     NativeCallFrameTracer tracer(globalData, exec);
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Throwing exception %s (returnToThrow).\n", globalData->exception.description());
+    dataLog("Throwing exception ", globalData->exception, " (returnToThrow).\n");
 #endif
     fixupPCforExceptionIfNeeded(exec);
     genericThrow(globalData, exec, globalData->exception, pc - exec->codeBlock()->instructions().begin());
@@ -82,7 +82,7 @@ void* callToThrow(ExecState* exec, Instruction* pc)
     JSGlobalData* globalData = &exec->globalData();
     NativeCallFrameTracer tracer(globalData, exec);
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Throwing exception %s (callToThrow).\n", globalData->exception.description());
+    dataLog("Throwing exception ", globalData->exception, " (callToThrow).\n");
 #endif
     fixupPCforExceptionIfNeeded(exec);
     genericThrow(globalData, exec, globalData->exception, pc - exec->codeBlock()->instructions().begin());
index 4300389290d6d363854d2b4bf905827b3ec87364..cdaf919cc59174f6c068f38376f7ddf99bba992a 100644 (file)
@@ -637,8 +637,7 @@ LLINT_SLOW_PATH_DECL(slow_path_add)
     JSValue v2 = LLINT_OP_C(3).jsValue();
     
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Trying to add %s", v1.description());
-    dataLogF(" to %s.\n", v2.description());
+    dataLog("Trying to add ", v1, " to ", v2, ".\n");
 #endif
     
     if (v1.isString() && !v2.isObject())
@@ -1369,7 +1368,7 @@ static SlowPathReturnType handleHostCall(ExecState* execCallee, Instruction* pc,
         }
         
 #if LLINT_SLOW_PATH_TRACING
-        dataLogF("Call callee is not a function: %s\n", callee.description());
+        dataLog("Call callee is not a function: ", callee, "\n");
 #endif
 
         ASSERT(callType == CallTypeNone);
@@ -1392,7 +1391,7 @@ static SlowPathReturnType handleHostCall(ExecState* execCallee, Instruction* pc,
     }
     
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Constructor callee is not a function: %s\n", callee.description());
+    dataLog("Constructor callee is not a function: ", callee, "\n");
 #endif
 
     ASSERT(constructType == ConstructTypeNone);