Re-land: Implement ObservedArrayPop, ObservedArrayShift, ObservedArrayUnshift & ObservedArraySplice

Original issue: https://codereview.chromium.org/15331002/

Broke mozilla/js1_5/Array/regress-451483.js, which ends up attempting to call %IsObserved() on a non-object.

IsObserved now checks for JSReceiver and returns false rather than crashing.

R=adamk@chromium.org, rossberg@chromium.org

Review URL: https://codereview.chromium.org/15777007

Patch from Rafael Weinstein <rafaelw@chromium.org>.

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@14888 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/array.js

@@ -395,6 +395,23 @@ function ArrayJoin(separator) {
 }
 
 
+function ObservedArrayPop(n) {
+  n--;
+  var value = this[n];
+
+  EnqueueSpliceRecord(this, n, [value], 1, 0);
+
+  try {
+    BeginPerformSplice(this);
+    delete this[n];
+    this.length = n;
+  } finally {
+    EndPerformSplice(this);
+  }
+
+  return value;
+}
+
 // Removes the last element from the array and returns it. See
 // ECMA-262, section 15.4.4.6.
 function ArrayPop() {
@@ -408,6 +425,10 @@ function ArrayPop() {
     this.length = n;
     return;
   }
+
+  if (%IsObserved(this))
+    return ObservedArrayPop.call(this, n);
+
   n--;
   var value = this[n];
   delete this[n];
@@ -424,7 +445,6 @@ function ObservedArrayPush() {
 
   try {
     BeginPerformSplice(this);
-
     for (var i = 0; i < m; i++) {
       this[i+n] = %_Arguments(i);
     }
@@ -558,6 +578,22 @@ function ArrayReverse() {
 }
 
 
+function ObservedArrayShift(len) {
+  var first = this[0];
+
+  EnqueueSpliceRecord(this, 0, [first], 1, 0);
+
+  try {
+    BeginPerformSplice(this);
+    SimpleMove(this, 0, 1, len, 0);
+    this.length = len - 1;
+  } finally {
+    EndPerformSplice(this);
+  }
+
+  return first;
+}
+
 function ArrayShift() {
   if (IS_NULL_OR_UNDEFINED(this) && !IS_UNDETECTABLE(this)) {
     throw MakeTypeError("called_on_null_or_undefined",
@@ -571,9 +607,12 @@ function ArrayShift() {
     return;
   }
 
+  if (%IsObserved(this))
+    return ObservedArrayShift.call(this, len);
+
   var first = this[0];
 
-  if (IS_ARRAY(this) && !%IsObserved(this)) {
+  if (IS_ARRAY(this)) {
     SmartMove(this, 0, 1, len, 0);
   } else {
     SimpleMove(this, 0, 1, len, 0);
@@ -584,6 +623,25 @@ function ArrayShift() {
   return first;
 }
 
+function ObservedArrayUnshift() {
+  var len = TO_UINT32(this.length);
+  var num_arguments = %_ArgumentsLength();
+
+  EnqueueSpliceRecord(this, 0, [], 0, num_arguments);
+
+  try {
+    BeginPerformSplice(this);
+    SimpleMove(this, 0, 0, len, num_arguments);
+    for (var i = 0; i < num_arguments; i++) {
+      this[i] = %_Arguments(i);
+    }
+    this.length = len + num_arguments;
+  } finally {
+    EndPerformSplice(this);
+  }
+
+  return len + num_arguments;
+}
 
 function ArrayUnshift(arg1) {  // length == 1
   if (IS_NULL_OR_UNDEFINED(this) && !IS_UNDETECTABLE(this)) {
@@ -591,10 +649,13 @@ function ArrayUnshift(arg1) {  // length == 1
                         ["Array.prototype.unshift"]);
   }
 
+  if (%IsObserved(this))
+    return ObservedArrayUnshift.apply(this, arguments);
+
   var len = TO_UINT32(this.length);
   var num_arguments = %_ArgumentsLength();
 
-  if (IS_ARRAY(this) && !%IsObserved(this)) {
+  if (IS_ARRAY(this)) {
     SmartMove(this, 0, 0, len, num_arguments);
   } else {
     SimpleMove(this, 0, 0, len, num_arguments);
@@ -655,52 +716,100 @@ function ArraySlice(start, end) {
 }
 
 
-function ArraySplice(start, delete_count) {
-  if (IS_NULL_OR_UNDEFINED(this) && !IS_UNDETECTABLE(this)) {
-    throw MakeTypeError("called_on_null_or_undefined",
-                        ["Array.prototype.splice"]);
-  }
-
-  var num_arguments = %_ArgumentsLength();
-
-  var len = TO_UINT32(this.length);
-  var start_i = TO_INTEGER(start);
-
+function ComputeSpliceStartIndex(start_i, len) {
   if (start_i < 0) {
     start_i += len;
-    if (start_i < 0) start_i = 0;
-  } else {
-    if (start_i > len) start_i = len;
+    return start_i < 0 ? 0 : start_i;
   }
 
+  return start_i > len ? len : start_i;
+}
+
+
+function ComputeSpliceDeleteCount(delete_count, num_arguments, len, start_i) {
   // SpiderMonkey, TraceMonkey and JSC treat the case where no delete count is
   // given as a request to delete all the elements from the start.
   // And it differs from the case of undefined delete count.
   // This does not follow ECMA-262, but we do the same for
   // compatibility.
   var del_count = 0;
-  if (num_arguments == 1) {
-    del_count = len - start_i;
-  } else {
-    del_count = TO_INTEGER(delete_count);
-    if (del_count < 0) del_count = 0;
-    if (del_count > len - start_i) del_count = len - start_i;
-  }
+  if (num_arguments == 1)
+    return len - start_i;
+
+  del_count = TO_INTEGER(delete_count);
+  if (del_count < 0)
+    return 0;
+
+  if (del_count > len - start_i)
+    return len - start_i;
+
+  return del_count;
+}
 
+
+function ObservedArraySplice(start, delete_count) {
+  var num_arguments = %_ArgumentsLength();
+  var len = TO_UINT32(this.length);
+  var start_i = ComputeSpliceStartIndex(TO_INTEGER(start), len);
+  var del_count = ComputeSpliceDeleteCount(delete_count, num_arguments, len,
+                                           start_i);
   var deleted_elements = [];
   deleted_elements.length = del_count;
+  var num_elements_to_add = num_arguments > 2 ? num_arguments - 2 : 0;
+
+  try {
+    BeginPerformSplice(this);
 
-  // Number of elements to add.
-  var num_additional_args = 0;
-  if (num_arguments > 2) {
-    num_additional_args = num_arguments - 2;
+    SimpleSlice(this, start_i, del_count, len, deleted_elements);
+    SimpleMove(this, start_i, del_count, len, num_elements_to_add);
+
+    // Insert the arguments into the resulting array in
+    // place of the deleted elements.
+    var i = start_i;
+    var arguments_index = 2;
+    var arguments_length = %_ArgumentsLength();
+    while (arguments_index < arguments_length) {
+      this[i++] = %_Arguments(arguments_index++);
+    }
+    this.length = len - del_count + num_elements_to_add;
+
+  } finally {
+    EndPerformSplice(this);
+    if (deleted_elements.length || num_elements_to_add) {
+       EnqueueSpliceRecord(this,
+                           start_i,
+                           deleted_elements.slice(),
+                           deleted_elements.length,
+                           num_elements_to_add);
+    }
   }
 
-  var use_simple_splice = true;
+  // Return the deleted elements.
+  return deleted_elements;
+}
+
+
+function ArraySplice(start, delete_count) {
+  if (IS_NULL_OR_UNDEFINED(this) && !IS_UNDETECTABLE(this)) {
+    throw MakeTypeError("called_on_null_or_undefined",
+                        ["Array.prototype.splice"]);
+  }
 
+  if (%IsObserved(this))
+    return ObservedArraySplice.apply(this, arguments);
+
+  var num_arguments = %_ArgumentsLength();
+  var len = TO_UINT32(this.length);
+  var start_i = ComputeSpliceStartIndex(TO_INTEGER(start), len);
+  var del_count = ComputeSpliceDeleteCount(delete_count, num_arguments, len,
+                                           start_i);
+  var deleted_elements = [];
+  deleted_elements.length = del_count;
+  var num_elements_to_add = num_arguments > 2 ? num_arguments - 2 : 0;
+
+  var use_simple_splice = true;
   if (IS_ARRAY(this) &&
-      !%IsObserved(this) &&
-      num_additional_args !== del_count) {
+      num_elements_to_add !== del_count) {
     // If we are only deleting/moving a few things near the end of the
     // array then the simple version is going to be faster, because it
     // doesn't touch most of the array.
@@ -712,10 +821,10 @@ function ArraySplice(start, delete_count) {
 
   if (use_simple_splice) {
     SimpleSlice(this, start_i, del_count, len, deleted_elements);
-    SimpleMove(this, start_i, del_count, len, num_additional_args);
+    SimpleMove(this, start_i, del_count, len, num_elements_to_add);
   } else {
     SmartSlice(this, start_i, del_count, len, deleted_elements);
-    SmartMove(this, start_i, del_count, len, num_additional_args);
+    SmartMove(this, start_i, del_count, len, num_elements_to_add);
   }
 
   // Insert the arguments into the resulting array in
@@ -726,7 +835,7 @@ function ArraySplice(start, delete_count) {
   while (arguments_index < arguments_length) {
     this[i++] = %_Arguments(arguments_index++);
   }
-  this.length = len - del_count + num_additional_args;
+  this.length = len - del_count + num_elements_to_add;
 
   // Return the deleted elements.
   return deleted_elements;

src/runtime.cc

@@ -13358,7 +13358,9 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_HaveSameMap) {
 RUNTIME_FUNCTION(MaybeObject*, Runtime_IsObserved) {
   NoHandleAllocation ha(isolate);
   ASSERT(args.length() == 1);
-  CONVERT_ARG_CHECKED(JSReceiver, obj, 0);
+
+  if (!args[0]->IsJSReceiver()) return isolate->heap()->false_value();
+  JSReceiver* obj = JSReceiver::cast(args[0]);
   if (obj->IsJSGlobalProxy()) {
     Object* proto = obj->GetPrototype();
     if (proto->IsNull()) return isolate->heap()->false_value();

test/mjsunit/harmony/object-observe.js

@@ -1175,11 +1175,13 @@ observer.assertCallbackRecords([
 ]);
 
 // Pop
-reset()
-var array = {0: 1, 1: 2, length: 2};
+reset();
+var array = [1, 2];
 Object.observe(array, observer.callback);
-Array.prototype.pop.call(array);
-Array.prototype.pop.call(array);
+Array.observe(array, observer2.callback);
+array.pop();
+array.pop();
+array.pop();
 Object.deliverChangeRecords(observer.callback);
 observer.assertCallbackRecords([
   { object: array, name: '1', type: 'deleted', oldValue: 2 },
@@ -1187,13 +1189,20 @@ observer.assertCallbackRecords([
   { object: array, name: '0', type: 'deleted', oldValue: 1 },
   { object: array, name: 'length', type: 'updated', oldValue: 1 },
 ]);
+Object.deliverChangeRecords(observer2.callback);
+observer2.assertCallbackRecords([
+  { object: array, type: 'splice', index: 1, removed: [2], addedCount: 0 },
+  { object: array, type: 'splice', index: 0, removed: [1], addedCount: 0 }
+]);
 
 // Shift
-reset()
-var array = {0: 1, 1: 2, length: 2};
+reset();
+var array = [1, 2];
 Object.observe(array, observer.callback);
-Array.prototype.shift.call(array);
-Array.prototype.shift.call(array);
+Array.observe(array, observer2.callback);
+array.shift();
+array.shift();
+array.shift();
 Object.deliverChangeRecords(observer.callback);
 observer.assertCallbackRecords([
   { object: array, name: '0', type: 'updated', oldValue: 1 },
@@ -1202,32 +1211,71 @@ observer.assertCallbackRecords([
   { object: array, name: '0', type: 'deleted', oldValue: 2 },
   { object: array, name: 'length', type: 'updated', oldValue: 1 },
 ]);
+Object.deliverChangeRecords(observer2.callback);
+observer2.assertCallbackRecords([
+  { object: array, type: 'splice', index: 0, removed: [1], addedCount: 0 },
+  { object: array, type: 'splice', index: 0, removed: [2], addedCount: 0 }
+]);
 
 // Unshift
-reset()
-var array = {0: 1, 1: 2, length: 2};
+reset();
+var array = [1, 2];
 Object.observe(array, observer.callback);
-Array.prototype.unshift.call(array, 3, 4);
+Array.observe(array, observer2.callback);
+array.unshift(3, 4);
+array.unshift(5);
 Object.deliverChangeRecords(observer.callback);
 observer.assertCallbackRecords([
   { object: array, name: '3', type: 'new' },
+  { object: array, name: 'length', type: 'updated', oldValue: 2 },
   { object: array, name: '2', type: 'new' },
   { object: array, name: '0', type: 'updated', oldValue: 1 },
   { object: array, name: '1', type: 'updated', oldValue: 2 },
-  { object: array, name: 'length', type: 'updated', oldValue: 2 },
+  { object: array, name: '4', type: 'new' },
+  { object: array, name: 'length', type: 'updated', oldValue: 4 },
+  { object: array, name: '3', type: 'updated', oldValue: 2 },
+  { object: array, name: '2', type: 'updated', oldValue: 1 },
+  { object: array, name: '1', type: 'updated', oldValue: 4 },
+  { object: array, name: '0', type: 'updated', oldValue: 3 },
+]);
+Object.deliverChangeRecords(observer2.callback);
+observer2.assertCallbackRecords([
+  { object: array, type: 'splice', index: 0, removed: [], addedCount: 2 },
+  { object: array, type: 'splice', index: 0, removed: [], addedCount: 1 }
 ]);
 
 // Splice
-reset()
-var array = {0: 1, 1: 2, 2: 3, length: 3};
+reset();
+var array = [1, 2, 3];
 Object.observe(array, observer.callback);
-Array.prototype.splice.call(array, 1, 1, 4, 5);
+Array.observe(array, observer2.callback);
+array.splice(1, 0, 4, 5); // 1 4 5 2 3
+array.splice(0, 2); // 5 2 3
+array.splice(1, 2, 6, 7); // 5 6 7
+array.splice(2, 0);
 Object.deliverChangeRecords(observer.callback);
 observer.assertCallbackRecords([
+  { object: array, name: '4', type: 'new' },
+  { object: array, name: 'length', type: 'updated', oldValue: 3 },
   { object: array, name: '3', type: 'new' },
   { object: array, name: '1', type: 'updated', oldValue: 2 },
   { object: array, name: '2', type: 'updated', oldValue: 3 },
-  { object: array, name: 'length', type: 'updated', oldValue: 3 },
+
+  { object: array, name: '0', type: 'updated', oldValue: 1 },
+  { object: array, name: '1', type: 'updated', oldValue: 4 },
+  { object: array, name: '2', type: 'updated', oldValue: 5 },
+  { object: array, name: '4', type: 'deleted', oldValue: 3 },
+  { object: array, name: '3', type: 'deleted', oldValue: 2 },
+  { object: array, name: 'length', type: 'updated', oldValue: 5 },
+
+  { object: array, name: '1', type: 'updated', oldValue: 2 },
+  { object: array, name: '2', type: 'updated', oldValue: 3 },
+]);
+Object.deliverChangeRecords(observer2.callback);
+observer2.assertCallbackRecords([
+  { object: array, type: 'splice', index: 1, removed: [], addedCount: 2 },
+  { object: array, type: 'splice', index: 0, removed: [1, 4], addedCount: 0 },
+  { object: array, type: 'splice', index: 1, removed: [2, 3], addedCount: 2 },
 ]);
 
 // Exercise StoreIC_ArrayLength