Object.observe: Move notification of JSArray length changes to JSArray::SetElementsLength

The previous implementation in Accessors::ArraySetLength failed when array length was set through StoreIC_ArrayLength. But that stub and the accessor both delegate to JSArray::SetElementsLength, so moving the code there allows notifications to be sent in both cases.

Review URL: https://codereview.chromium.org/11275292
Patch from Adam Klein <adamk@chromium.org>.

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

src/accessors.cc

@@ -95,47 +95,6 @@ Object* Accessors::FlattenNumber(Object* value) {
 }
 
 
-static MaybeObject* ArraySetLengthObserved(Isolate* isolate,
-                                           Handle<JSArray> array,
-                                           Handle<Object> new_length_handle) {
-  List<Handle<String> > indices;
-  List<Handle<Object> > old_values;
-  Handle<Object> old_length_handle(array->length(), isolate);
-  uint32_t old_length = 0;
-  CHECK(old_length_handle->ToArrayIndex(&old_length));
-  uint32_t new_length = 0;
-  CHECK(new_length_handle->ToArrayIndex(&new_length));
-  // TODO(adamk): This loop can be very slow for arrays in dictionary mode.
-  // Find another way to iterate over arrays with dictionary elements.
-  for (uint32_t i = old_length - 1; i + 1 > new_length; --i) {
-    PropertyAttributes attributes = array->GetLocalElementAttribute(i);
-    if (attributes == ABSENT) continue;
-    // A non-configurable property will cause the truncation operation to
-    // stop at this index.
-    if (attributes == DONT_DELETE) break;
-    // TODO(adamk): Don't fetch the old value if it's an accessor.
-    old_values.Add(Object::GetElement(array, i));
-    indices.Add(isolate->factory()->Uint32ToString(i));
-  }
-
-  MaybeObject* result = array->SetElementsLength(*new_length_handle);
-  Handle<Object> hresult;
-  if (!result->ToHandle(&hresult)) return result;
-
-  CHECK(array->length()->ToArrayIndex(&new_length));
-  if (old_length != new_length) {
-    for (int i = 0; i < indices.length(); ++i) {
-      JSObject::EnqueueChangeRecord(
-          array, "deleted", indices[i], old_values[i]);
-    }
-    JSObject::EnqueueChangeRecord(
-        array, "updated", isolate->factory()->length_symbol(),
-        old_length_handle);
-  }
-  return *hresult;
-}
-
-
 MaybeObject* Accessors::ArraySetLength(JSObject* object, Object* value, void*) {
   Isolate* isolate = object->GetIsolate();
 
@@ -163,11 +122,7 @@ MaybeObject* Accessors::ArraySetLength(JSObject* object, Object* value, void*) {
   if (has_exception) return Failure::Exception();
 
   if (uint32_v->Number() == number_v->Number()) {
-    if (FLAG_harmony_observation && array_handle->map()->is_observed()) {
-      return ArraySetLengthObserved(isolate, array_handle, uint32_v);
-    } else {
-      return array_handle->SetElementsLength(*uint32_v);
-    }
+    return array_handle->SetElementsLength(*uint32_v);
   }
   return isolate->Throw(
       *isolate->factory()->NewRangeError("invalid_array_length",

src/objects.cc

@@ -9382,7 +9382,51 @@ void JSArray::Expand(int required_size) {
 MaybeObject* JSArray::SetElementsLength(Object* len) {
   // We should never end in here with a pixel or external array.
   ASSERT(AllowsSetElementsLength());
-  return GetElementsAccessor()->SetLength(this, len);
+  if (!(FLAG_harmony_observation && map()->is_observed()))
+    return GetElementsAccessor()->SetLength(this, len);
+
+  Isolate* isolate = GetIsolate();
+  HandleScope scope(isolate);
+  Handle<JSArray> self(this);
+  List<Handle<String> > indices;
+  List<Handle<Object> > old_values;
+  Handle<Object> old_length_handle(self->length());
+  Handle<Object> new_length_handle(len);
+  uint32_t old_length = 0;
+  CHECK(old_length_handle->ToArrayIndex(&old_length));
+  uint32_t new_length = 0;
+  if (!new_length_handle->ToArrayIndex(&new_length))
+    return Failure::InternalError();
+
+  // TODO(adamk): This loop can be very slow for arrays in dictionary mode.
+  // Find another way to iterate over arrays with dictionary elements.
+  for (uint32_t i = old_length - 1; i + 1 > new_length; --i) {
+    PropertyAttributes attributes = self->GetLocalElementAttribute(i);
+    if (attributes == ABSENT) continue;
+    // A non-configurable property will cause the truncation operation to
+    // stop at this index.
+    if (attributes == DONT_DELETE) break;
+    // TODO(adamk): Don't fetch the old value if it's an accessor.
+    old_values.Add(Object::GetElement(self, i));
+    indices.Add(isolate->factory()->Uint32ToString(i));
+  }
+
+  MaybeObject* result =
+      self->GetElementsAccessor()->SetLength(*self, *new_length_handle);
+  Handle<Object> hresult;
+  if (!result->ToHandle(&hresult)) return result;
+
+  CHECK(self->length()->ToArrayIndex(&new_length));
+  if (old_length != new_length) {
+    for (int i = 0; i < indices.length(); ++i) {
+      JSObject::EnqueueChangeRecord(
+          self, "deleted", indices[i], old_values[i]);
+    }
+    JSObject::EnqueueChangeRecord(
+        self, "updated", isolate->factory()->length_symbol(),
+        old_length_handle);
+  }
+  return *hresult;
 }
 
 

src/objects.h

@@ -8345,6 +8345,7 @@ class JSArray: public JSObject {
 
   // Initializes the array to a certain length.
   inline bool AllowsSetElementsLength();
+  // Can cause GC.
   MUST_USE_RESULT MaybeObject* SetElementsLength(Object* length);
 
   // Set the content of the array to the content of storage.

test/mjsunit/harmony/object-observe.js

@@ -590,3 +590,17 @@ observer.assertCallbackRecords([
   { object: array, name: '2', type: 'updated', oldValue: 3 },
   { object: array, name: 'length', type: 'updated', oldValue: 3 },
 ]);
+
+// Exercise StoreIC_ArrayLength
+reset();
+var dummy = {};
+Object.observe(dummy, observer.callback);
+Object.unobserve(dummy, observer.callback);
+var array = [0];
+Object.observe(array, observer.callback);
+array.splice(0, 1);
+Object.deliverChangeRecords(observer.callback);
+observer.assertCallbackRecords([
+  { object: array, name: '0', type: 'deleted', oldValue: 0 },
+  { object: array, name: 'length', type: 'updated', oldValue: 1},
+]);