Fix for Bug 16889: REGRESSION (r29425): Canvas-based graphing calculator fails to run
authoroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Feb 2008 07:58:06 +0000 (07:58 +0000)
committeroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Feb 2008 07:58:06 +0000 (07:58 +0000)
      Bug 17015: REGRESSION (r29414-29428): www.fox.com "shows" menu fails to render
      Bug 17164: REGRESSION: JavaScript pop-up menu appears at wrong location when hovering image at http://news.chinatimes.com/

Reviewed by Oliver Hunt

<http://bugs.webkit.org/show_bug.cgi?id=16889>
<rdar://problem/5696255>

<http://bugs.webkit.org/show_bug.cgi?id=17015>

<http://bugs.webkit.org/show_bug.cgi?id=17164>
<rdar://problem/5720947>

The ActivationImp tear-off (r29425) introduced a problem with ReadModify
nodes that first resolve a slot, call valueForReadModifyNode(), and then
store a value in the previously resolved slot. Since valueForReadModifyNode()
may cause a tear-off, the slot needs to be resolved again, but this was
not happening with the existing code.

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

JavaScriptCore/ChangeLog
JavaScriptCore/kjs/nodes.cpp
LayoutTests/ChangeLog
LayoutTests/fast/js/read-modify-eval-expected.txt [new file with mode: 0644]
LayoutTests/fast/js/read-modify-eval.html [new file with mode: 0644]
LayoutTests/fast/js/resources/read-modify-eval.js [new file with mode: 0644]

index e8dad56..01d003e 100644 (file)
@@ -1,3 +1,29 @@
+2008-02-04  Cameron Zwarich  <cwzwarich@uwaterloo.ca>
+
+        Reviewed by Oliver Hunt.
+
+        Fixes Bug 16889: REGRESSION (r29425): Canvas-based graphing calculator fails to run
+              Bug 17015: REGRESSION (r29414-29428): www.fox.com "shows" menu fails to render
+              Bug 17164: REGRESSION: JavaScript pop-up menu appears at wrong location when hovering image at http://news.chinatimes.com/
+
+        <http://bugs.webkit.org/show_bug.cgi?id=16889>
+        <rdar://problem/5696255>
+
+        <http://bugs.webkit.org/show_bug.cgi?id=17015>
+
+        <http://bugs.webkit.org/show_bug.cgi?id=17164>
+        <rdar://problem/5720947>
+
+        The ActivationImp tear-off (r29425) introduced a problem with ReadModify
+        nodes that first resolve a slot, call valueForReadModifyNode(), and then
+        store a value in the previously resolved slot. Since valueForReadModifyNode()
+        may cause a tear-off, the slot needs to be resolved again, but this was
+        not happening with the existing code.
+
+        * kjs/nodes.cpp:
+        (KJS::ReadModifyLocalVarNode::evaluate):
+        (KJS::ReadModifyResolveNode::evaluate):
+
 2008-02-04  Cameron McCormack <cam@mcc.id.au>
 
         Reviewed by Geoff Garen.
index 9d7cc92..59f7dec 100644 (file)
@@ -1289,8 +1289,11 @@ JSValue* PostIncResolveNode::evaluate(ExecState* exec)
     PropertySlot slot;
     do {
         if ((*iter)->getPropertySlot(exec, m_ident, slot)) {
-            // See the comment in PostIncResolveNode::evaluate().
-
+            // If m_ident is 'arguments', the base->getPropertySlot() may cause 
+            // base (which must be an ActivationImp in such this case) to be torn
+            // off from the activation stack, in which case we need to get it again
+            // from the ScopeChainIterator.
+            
             JSObject* base = *iter;
             JSValue* v = slot.getValue(exec, base, m_ident)->toJSNumber(exec);
             base->put(exec, m_ident, jsNumber(v->toNumber(exec) + 1));
@@ -3256,13 +3259,17 @@ void AssignResolveNode::optimizeVariableAccess(const SymbolTable& symbolTable, c
 JSValue* ReadModifyLocalVarNode::evaluate(ExecState* exec)
 {
     ASSERT(exec->variableObject() == exec->scopeChain().top());
-    JSValue** slot = &exec->localStorage()[m_index].value;
 
     ASSERT(m_operator != OpEqual);
-    JSValue* v = valueForReadModifyAssignment(exec, *slot, m_right.get(), m_operator);
+    JSValue* v = valueForReadModifyAssignment(exec, exec->localStorage()[m_index].value, m_right.get(), m_operator);
 
     KJS_CHECKEXCEPTIONVALUE
-    *slot = v;
+    
+    // We can't store a pointer into localStorage() and use it throughout the function
+    // body, because valueForReadModifyAssignment() might cause an ActivationImp tear-off,
+    // changing the value of localStorage().
+    
+    exec->localStorage()[m_index].value = v;
     return v;
 }
 
@@ -3334,8 +3341,11 @@ found:
     v = valueForReadModifyAssignment(exec, v1, m_right.get(), m_operator);
 
     KJS_CHECKEXCEPTIONVALUE
-
-    base->put(exec, m_ident, v);
+    
+    // Since valueForReadModifyAssignment() might cause an ActivationImp tear-off,
+    // we need to get the base from the ScopeChainIterator again.
+    
+    (*iter)->put(exec, m_ident, v);
     return v;
 }
 
index 483e81c..ebd2504 100644 (file)
@@ -1,3 +1,25 @@
+2008-02-04  Cameron Zwarich  <cwzwarich@uwaterloo.ca>
+
+        Reviewed by Oliver Hunt.
+
+        Adds layout tests for the collective fix for the following bugs:
+
+        Bug 16889: REGRESSION (r29425): Canvas-based graphing calculator fails to run
+        Bug 17015: REGRESSION (r29414-29428): www.fox.com "shows" menu fails to render
+        Bug 17164: REGRESSION: JavaScript pop-up menu appears at wrong location when hovering image at http://news.chinatimes.com/
+
+        <http://bugs.webkit.org/show_bug.cgi?id=16889>
+        <rdar://problem/5696255>
+
+        <http://bugs.webkit.org/show_bug.cgi?id=17015>
+
+        <http://bugs.webkit.org/show_bug.cgi?id=17164>
+        <rdar://problem/5720947>
+
+        * fast/js/read-modify-eval-expected.txt: Added.
+        * fast/js/read-modify-eval.html: Added.
+        * fast/js/resources/read-modify-eval.js: Added.
+
 2008-02-04  Oliver Hunt  <oliver@apple.com>
 
         Reviewed by Steve F.
diff --git a/LayoutTests/fast/js/read-modify-eval-expected.txt b/LayoutTests/fast/js/read-modify-eval-expected.txt
new file mode 100644 (file)
index 0000000..412ca2c
--- /dev/null
@@ -0,0 +1,24 @@
+Tests whether eval() works inside statements that read and modify a value.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS multTest(); is true
+PASS divTest(); is true
+PASS addTest(); is true
+PASS subTest(); is true
+PASS lshiftTest(); is true
+PASS rshiftTest(); is true
+PASS urshiftTest(); is true
+PASS andTest(); is true
+PASS xorTest(); is true
+PASS orTest(); is true
+PASS modTest(); is true
+PASS preIncTest(); is true
+PASS preDecTest(); is true
+PASS postIncTest(); is true
+PASS postDecTest(); is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/js/read-modify-eval.html b/LayoutTests/fast/js/read-modify-eval.html
new file mode 100644 (file)
index 0000000..a3e60de
--- /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/read-modify-eval.js"></script>
+<script src="resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/LayoutTests/fast/js/resources/read-modify-eval.js b/LayoutTests/fast/js/resources/read-modify-eval.js
new file mode 100644 (file)
index 0000000..08b1412
--- /dev/null
@@ -0,0 +1,127 @@
+description(
+'Tests whether eval() works inside statements that read and modify a value.'
+);
+
+function multTest()
+{
+    var x = 1;
+    x *= eval('2');
+    return x == 2;
+}
+
+function divTest()
+{
+    var x = 2;
+    x /= eval('2');
+    return x == 1;
+}
+
+function addTest()
+{
+    var x = 0;
+    x += eval('1');
+    return x == 1;
+}
+
+function subTest()
+{
+    var x = 0;
+    x -= eval('1');
+    return x == -1;
+}
+
+function lshiftTest()
+{
+    var x = 1;
+    x <<= eval('1');
+    return x == 2;
+}
+
+function rshiftTest()
+{
+    var x = 1;
+    x >>= eval('1');
+    return x == 0;
+}
+
+function urshiftTest()
+{
+    var x = 1;
+    x >>>= eval('1');
+    return x == 0;
+}
+
+function andTest()
+{
+    var x = 1;
+    x &= eval('1');
+    return x == 1;
+}
+
+function xorTest()
+{
+    var x = 0;
+    x ^= eval('1');
+    return x == 1;
+}
+
+function orTest()
+{
+    var x = 0;
+    x |= eval('1');
+    return x == 1;
+}
+
+function modTest()
+{
+    var x = 4;
+    x %= eval('3');
+    return x == 1;
+}
+
+function preIncTest()
+{
+    var x = { value: 0 };
+    ++eval('x').value;
+    return x.value == 1;
+}
+
+function preDecTest()
+{
+    var x = { value: 0 };
+    --eval('x').value;
+    return x.value == -1;
+}
+
+function postIncTest()
+{
+    var x = { value: 0 };
+    eval('x').value++;
+    return x.value == 1;
+}
+
+function postDecTest()
+{
+    var x = { value: 0 };
+    eval('x').value--;
+    return x.value == -1;
+}
+
+shouldBeTrue('multTest();');
+shouldBeTrue('divTest();');
+shouldBeTrue('addTest();');
+shouldBeTrue('subTest();');
+shouldBeTrue('lshiftTest();');
+shouldBeTrue('rshiftTest();');
+shouldBeTrue('urshiftTest();');
+shouldBeTrue('andTest();');
+shouldBeTrue('xorTest();');
+shouldBeTrue('orTest();');
+shouldBeTrue('modTest();');
+
+shouldBeTrue('preIncTest();');
+shouldBeTrue('preDecTest();');
+shouldBeTrue('postIncTest();');
+shouldBeTrue('postDecTest();');
+
+successfullyParsed = true;