Object.observe: Handle oldValue for elements with accessors properly.

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index a03054653aa16cf014d8ffd83783a7ccd4d7c223..05754b23d7502a86b00d62ca3783491a5df69d56 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -4132,14 +4132,15 @@ MaybeObject* JSObject::DeleteElement(uint32_t index, DeleteMode mode) {
   Handle<JSObject> self(this);
 
   Handle<String> name;
-  Handle<Object> old_value(isolate->heap()->the_hole_value());
+  Handle<Object> old_value;
   bool preexists = false;
   if (FLAG_harmony_observation && map()->is_observed()) {
     name = isolate->factory()->Uint32ToString(index);
     preexists = self->HasLocalElement(index);
     if (preexists) {
-      // TODO(observe): only read & set old_value if it's not an accessor
-      old_value = Object::GetElement(self, index);
+      old_value = GetLocalElementAccessorPair(index) != NULL
+          ? Handle<Object>::cast(isolate->factory()->the_hole_value())
+          : Object::GetElement(self, index);
     }
   }
 
@@ -4886,13 +4887,12 @@ MaybeObject* JSObject::DefineAccessor(String* name_raw,
   uint32_t index = 0;
   bool is_element = name->AsArrayIndex(&index);
 
-  Handle<Object> old_value(isolate->heap()->the_hole_value());
+  Handle<Object> old_value = isolate->factory()->the_hole_value();
   bool preexists = false;
   if (FLAG_harmony_observation && map()->is_observed()) {
     if (is_element) {
       preexists = HasLocalElement(index);
-      if (preexists) {
-        // TODO(observe): distinguish the case where it's an accessor
+      if (preexists && GetLocalElementAccessorPair(index) == NULL) {
         old_value = Object::GetElement(self, index);
       }
     } else {
@@ -9555,7 +9555,43 @@ MaybeObject* JSObject::EnsureCanContainElements(Arguments* args,
 }
 
 
-JSObject::LocalElementType JSObject::GetLocalElementType(uint32_t index) {
+PropertyType JSObject::GetLocalPropertyType(String* name) {
+  uint32_t index = 0;
+  if (name->AsArrayIndex(&index)) {
+    return GetLocalElementType(index);
+  }
+  LookupResult lookup(GetIsolate());
+  LocalLookup(name, &lookup);
+  return lookup.type();
+}
+
+
+PropertyType JSObject::GetLocalElementType(uint32_t index) {
+  return GetElementsAccessor()->GetType(this, this, index);
+}
+
+
+AccessorPair* JSObject::GetLocalPropertyAccessorPair(String* name) {
+  uint32_t index = 0;
+  if (name->AsArrayIndex(&index)) {
+    return GetLocalElementAccessorPair(index);
+  }
+  LookupResult lookup(GetIsolate());
+  LocalLookup(name, &lookup);
+  if (lookup.IsPropertyCallbacks() &&
+      lookup.GetCallbackObject()->IsAccessorPair()) {
+    return AccessorPair::cast(lookup.GetCallbackObject());
+  }
+  return NULL;
+}
+
+
+AccessorPair* JSObject::GetLocalElementAccessorPair(uint32_t index) {
+  return GetElementsAccessor()->GetAccessorPair(this, this, index);
+}
+
+
+JSObject::LocalElementKind JSObject::GetLocalElementKind(uint32_t index) {
   // Check access rights if needed.
   if (IsAccessCheckNeeded()) {
     Heap* heap = GetHeap();
@@ -9569,7 +9605,7 @@ JSObject::LocalElementType JSObject::GetLocalElementType(uint32_t index) {
     Object* proto = GetPrototype();
     if (proto->IsNull()) return UNDEFINED_ELEMENT;
     ASSERT(proto->IsJSGlobalObject());
-    return JSObject::cast(proto)->GetLocalElementType(index);
+    return JSObject::cast(proto)->GetLocalElementKind(index);
   }
 
   // Check for lookup interceptor
@@ -10322,8 +10358,8 @@ MaybeObject* JSObject::SetElement(uint32_t index,
   Handle<Object> old_length;
 
   if (old_attributes != ABSENT) {
-    // TODO(observe): only read & set old_value if we have a data property
-    old_value = Object::GetElement(self, index);
+    if (GetLocalElementAccessorPair(index) == NULL)
+      old_value = Object::GetElement(self, index);
   } else if (self->IsJSArray()) {
     // Store old array length in case adding an element grows the array.
     old_length = handle(Handle<JSArray>::cast(self)->length());