<rdar://problem/6150322> In Gmail, a crash occurs at KJS::Machine::privateExecute...

<https://bugs.webkit.org/show_bug.cgi?id=20386>

Reviewed by Cameron Zwarich.

This crash was caused by "depth()" incorrectly determining the scope depth
of a 0 depth function without a full scope chain.  Because such a function
would not have an activation the depth function would return the scope depth
of the parent frame, thus triggering an incorrect unwind.  Any subsequent
look up that walked the scope chain would result in incorrect behaviour,
leading to a crash or incorrect variable resolution.  This can only actually
happen in try...finally statements as that's the only path that can result in
the need to unwind the scope chain, but not force the function to need a
full scope chain.

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

diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
index 431a961..4f6e8a9 100644 (file)
--- a/JavaScriptCore/ChangeLog
+++ b/JavaScriptCore/ChangeLog
@@ -1,3 +1,26 @@
+2008-08-17  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Cameron Zwarich.
+
+        <rdar://problem/6150322> In Gmail, a crash occurs at KJS::Machine::privateExecute() when applying list styling to text after a quote had been removed
+        <https://bugs.webkit.org/show_bug.cgi?id=20386>
+
+        This crash was caused by "depth()" incorrectly determining the scope depth 
+        of a 0 depth function without a full scope chain.  Because such a function
+        would not have an activation the depth function would return the scope depth
+        of the parent frame, thus triggering an incorrect unwind.  Any subsequent 
+        look up that walked the scope chain would result in incorrect behaviour,
+        leading to a crash or incorrect variable resolution.  This can only actually
+        happen in try...finally statements as that's the only path that can result in
+        the need to unwind the scope chain, but not force the function to need a
+        full scope chain.
+
+        The fix is simply to check for this case before attempting to walk the scope chain.
+
+        * VM/Machine.cpp:
+        (KJS::depth):
+        (KJS::Machine::throwException):
+
 2008-08-17  Cameron Zwarich  <cwzwarich@uwaterloo.ca>
 
         Reviewed by Maciej.

diff --git a/JavaScriptCore/VM/Machine.cpp b/JavaScriptCore/VM/Machine.cpp
index 915fca8..5ae33c6 100644 (file)
--- a/JavaScriptCore/VM/Machine.cpp
+++ b/JavaScriptCore/VM/Machine.cpp
@@ -82,8 +82,10 @@ static void* op_call_indirect;
 #endif
 
 // Returns the depth of the scope chain within a given call frame.
-static int depth(ScopeChain& sc)
+static int depth(CodeBlock* codeBlock, ScopeChain& sc)
 {
+    if (!codeBlock->needsFullScopeChain)
+        return 0;
     int scopeDepth = 0;
     ScopeChainIterator iter = sc.begin();
     ScopeChainIterator end = sc.end();
@@ -740,7 +742,7 @@ NEVER_INLINE Instruction* Machine::throwException(ExecState* exec, JSValue*& exc
     // Now unwind the scope chain within the exception handler's call frame.
 
     ScopeChain sc(scopeChain);
-    int scopeDelta = depth(sc) - scopeDepth;
+    int scopeDelta = depth(codeBlock, sc) - scopeDepth;
     ASSERT(scopeDelta >= 0);
     while (scopeDelta--)
         sc.pop();

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 2685add..f05a460 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,16 @@
+2008-08-17  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Cameron Zwarich.
+
+        <rdar://problem/6150322> In Gmail, a crash occurs at KJS::Machine::privateExecute() when applying list styling to text after a quote had been removed
+        <https://bugs.webkit.org/show_bug.cgi?id=20386>
+
+        Testcases for determining scope chain depth in a function with out a full scope chain.
+
+        * fast/js/exception-try-finally-scope-error-expected.txt: Added.
+        * fast/js/exception-try-finally-scope-error.html: Added.
+        * fast/js/resources/exception-try-finally-scope-error.js: Added.
+
 2008-08-15  Dan Bernstein  <mitz@apple.com>
 
         Rubber-stamped by Tim Hatcher.

diff --git a/LayoutTests/fast/js/exception-try-finally-scope-error-expected.txt b/LayoutTests/fast/js/exception-try-finally-scope-error-expected.txt
new file mode 100644 (file)
index 0000000..0b9bfa0
--- /dev/null
+++ b/LayoutTests/fast/js/exception-try-finally-scope-error-expected.txt
@@ -0,0 +1,10 @@
+This test makes sure stack unwinding works correctly when confronted with a 0-depth scope chain without an activation
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS result is "inner scope"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

diff --git a/LayoutTests/fast/js/exception-try-finally-scope-error.html b/LayoutTests/fast/js/exception-try-finally-scope-error.html
new file mode 100644 (file)
index 0000000..912332a
--- /dev/null
+++ b/LayoutTests/fast/js/exception-try-finally-scope-error.html
@@ -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/exception-try-finally-scope-error.js"></script>
+<script src="resources/js-test-post.js"></script>
+</body>
+</html>

diff --git a/LayoutTests/fast/js/resources/exception-try-finally-scope-error.js b/LayoutTests/fast/js/resources/exception-try-finally-scope-error.js
new file mode 100644 (file)
index 0000000..0ad889f
--- /dev/null
+++ b/LayoutTests/fast/js/resources/exception-try-finally-scope-error.js
@@ -0,0 +1,10 @@
+description('This test makes sure stack unwinding works correctly when confronted with a 0-depth scope chain without an activation');
+var result;
+function runTest() {
+    var test = "outer scope";
+    with({test:"inner scope"})
+       (function () { try { throw ""; } finally { result = test; shouldBe("result", '"inner scope"'); return;}})()
+}
+runTest();
+
+var successfullyParsed = true;