Changeset 29997 in webkit

Timestamp:

Feb 4, 2008 11:58:06 PM (16 years ago)

Author:

oliver@apple.com

Message:

Fix for 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/

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.

Location:

trunk

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/kjs/nodes.cpp (modified) (3 diffs)
• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/js/read-modify-eval-expected.txt (added)
• LayoutTests/fast/js/read-modify-eval.html (added)
• LayoutTests/fast/js/resources/read-modify-eval.js (added)

trunk/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/JavaScriptCore/kjs/nodes.cpp

@@ -1290 +1290 @@
     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);
@@ -3257 +3260 @@
 {
     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);
-
-    KJS_CHECKEXCEPTIONVALUE
-    *slot = v;
+    JSValue* v = valueForReadModifyAssignment(exec, exec->localStorage()[m_index].value, m_right.get(), m_operator);
+
+    KJS_CHECKEXCEPTIONVALUE
+    
+    // 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;
 }
@@ -3335 +3342 @@
 
     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;
 }

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>