Changeset 180564 in webkit

Timestamp:

Feb 24, 2015, 9:08:06 AM (11 years ago)

Author:

fpizlo@apple.com

Message:

Our bizarre behavior on Arguments::defineOwnProperty should be deliberate rather than a spaghetti incident
​https://bugs.webkit.org/show_bug.cgi?id=141951

Reviewed by Benjamin Poulain.

This patch has no behavioral change, but it simplifies a bunch of wrong code. The code is
still wrong in exactly the same way, but at least it's obvious what's going on. The wrongness
is covered by this bug: ​https://bugs.webkit.org/show_bug.cgi?id=141952.

• runtime/Arguments.cpp:

(JSC::Arguments::copyBackingStore): We should only see the arguments token; assert otherwise. This works because if the GC sees the butterfly token it calls the JSObject::copyBackingStore method directly.
(JSC::Arguments::defineOwnProperty): Make our bizarre behavior deliberate rather than an accident of a decade of patches.

• tests/stress/arguments-bizarre-behavior.js: Added.

(foo):

• tests/stress/arguments-bizarre-behaviour-disable-enumerability.js: Added. My choice of spellings of the word "behavio[u]r" is almost as consistent as our implementation of arguments.

(foo):

• tests/stress/arguments-custom-properties-gc.js: Added. I added this test because at first I was unsure if we GCd arguments correctly.

(makeBaseArguments):
(makeArray):
(cons):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/Arguments.cpp (modified) (2 diffs)
• tests/stress/arguments-bizarre-behavior.js (added)
• tests/stress/arguments-bizarre-behaviour-disable-enumerability.js (added)
• tests/stress/arguments-custom-properties-gc.js (added)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-02-23  Filip Pizlo  <fpizlo@apple.com>
+
+        Our bizarre behavior on Arguments::defineOwnProperty should be deliberate rather than a spaghetti incident
+        https://bugs.webkit.org/show_bug.cgi?id=141951
+
+        Reviewed by Benjamin Poulain.
+        
+        This patch has no behavioral change, but it simplifies a bunch of wrong code. The code is
+        still wrong in exactly the same way, but at least it's obvious what's going on. The wrongness
+        is covered by this bug: https://bugs.webkit.org/show_bug.cgi?id=141952.
+
+        * runtime/Arguments.cpp:
+        (JSC::Arguments::copyBackingStore): We should only see the arguments token; assert otherwise. This works because if the GC sees the butterfly token it calls the JSObject::copyBackingStore method directly.
+        (JSC::Arguments::defineOwnProperty): Make our bizarre behavior deliberate rather than an accident of a decade of patches.
+        * tests/stress/arguments-bizarre-behavior.js: Added.
+        (foo):
+        * tests/stress/arguments-bizarre-behaviour-disable-enumerability.js: Added. My choice of spellings of the word "behavio[u]r" is almost as consistent as our implementation of arguments.
+        (foo):
+        * tests/stress/arguments-custom-properties-gc.js: Added. I added this test because at first I was unsure if we GCd arguments correctly.
+        (makeBaseArguments):
+        (makeArray):
+        (cons):
+
 2015-02-23  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/JavaScriptCore/runtime/Arguments.cpp

@@ -82 +82 @@
 
     default:
-        return;
+        RELEASE_ASSERT_NOT_REACHED();
     }
 }
@@ -296 +296 @@
     if (i < thisObject->m_numArguments) {
         RELEASE_ASSERT(i < PropertyName::NotAnIndex);
-        // If the property is not yet present on the object, and is not yet marked as deleted, then add it now.
-        PropertySlot slot(thisObject);
-        if (!thisObject->isDeletedArgument(i) && !JSObject::getOwnPropertySlot(thisObject, exec, propertyName, slot)) {
+        
+        if (thisObject->isArgument(i)) {
+            if (!descriptor.isAccessorDescriptor()) {
+                // If the property is not deleted and we are using a non-accessor descriptor, then
+                // make sure that the aliased argument sees the value.
+                if (descriptor.value())
+                    thisObject->trySetArgument(exec->vm(), i, descriptor.value());
+            
+                // If the property is not deleted and we are using a non-accessor, writable
+                // descriptor, then we are done. The argument continues to be aliased. Note that we
+                // ignore the request to change enumerability. We appear to have always done so, in
+                // cases where the argument was still aliased.
+                // FIXME: https://bugs.webkit.org/show_bug.cgi?id=141952
+                if (descriptor.writable())
+                    return true;
+            }
+            
+            // If the property is a non-deleted argument, then move it into the base object and then
+            // delete it.
             JSValue value = thisObject->tryGetArgument(i);
             ASSERT(value);
             object->putDirectMayBeIndex(exec, propertyName, value);
-        }
-        if (!Base::defineOwnProperty(object, exec, propertyName, descriptor, shouldThrow))
-            return false;
-
-        // From ES 5.1, 10.6 Arguments Object
-        // 5. If the value of isMapped is not undefined, then
-        if (thisObject->isArgument(i)) {
-            // a. If IsAccessorDescriptor(Desc) is true, then
-            if (descriptor.isAccessorDescriptor()) {
-                // i. Call the [[Delete]] internal method of map passing P, and false as the arguments.
-                thisObject->tryDeleteArgument(exec->vm(), i);
-            } else { // b. Else
-                // i. If Desc.[[Value]] is present, then
-                // 1. Call the [[Put]] internal method of map passing P, Desc.[[Value]], and Throw as the arguments.
-                if (descriptor.value())
-                    thisObject->trySetArgument(exec->vm(), i, descriptor.value());
-                // ii. If Desc.[[Writable]] is present and its value is false, then
-                // 1. Call the [[Delete]] internal method of map passing P and false as arguments.
-                if (descriptor.writablePresent() && !descriptor.writable())
-                    thisObject->tryDeleteArgument(exec->vm(), i);
-            }
-        }
-        return true;
+            thisObject->tryDeleteArgument(exec->vm(), i);
+        }
+        
+        // Now just let the normal object machinery do its thing.
+        return Base::defineOwnProperty(object, exec, propertyName, descriptor, shouldThrow);
     }