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

src/objects.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 4114 matching lines @@
     if (proto->IsNull()) return isolate->heap()->false_value();
     ASSERT(proto->IsJSGlobalObject());
     return JSGlobalObject::cast(proto)->DeleteElement(index, mode);
   }
 
   // From this point on everything needs to be handlified.
   HandleScope scope(isolate);
   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);
     }
   }
 
   MaybeObject* result;
   // Skip interceptor if forcing deletion.
   if (self->HasIndexedInterceptor() && mode != FORCE_DELETION) {
     result = self->DeleteElementWithInterceptor(index);
   } else {
     result = self->GetElementsAccessor()->Delete(*self, index, mode);
   }
@@ skipping 726 matching lines @@
   // From this point on everything needs to be handlified.
   HandleScope scope(GetIsolate());
   Handle<JSObject> self(this);
   Handle<String> name(name_raw);
   Handle<Object> getter(getter_raw);
   Handle<Object> setter(setter_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 {
       LookupResult lookup(isolate);
       LocalLookup(*name, &lookup);
       preexists = lookup.IsProperty();
       if (preexists) old_value = handle(lookup.GetLazyValue());
     }
   }
 
@@ skipping 4642 matching lines @@
                                                 EnsureElementsMode mode) {
   // Elements in |Arguments| are ordered backwards (because they're on the
   // stack), but the method that's called here iterates over them in forward
   // direction.
   return EnsureCanContainElements(
       args->arguments() - first_arg - (arg_count - 1),
       arg_count, mode);
 }
 
 
-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();
     if (!heap->isolate()->MayIndexedAccess(this, index, v8::ACCESS_HAS)) {
       heap->isolate()->ReportFailedAccessCheck(this, v8::ACCESS_HAS);
       return UNDEFINED_ELEMENT;
     }
   }
 
   if (IsJSGlobalProxy()) {
     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
   if (HasIndexedInterceptor()) {
     return GetElementAttributeWithInterceptor(this, index, false) != ABSENT
         ? INTERCEPTED_ELEMENT : UNDEFINED_ELEMENT;
   }
 
   // Handle [] on String objects.
   if (this->IsStringObjectWithCharacterAt(index)) {
@@ skipping 732 matching lines @@
   }
 
   // From here on, everything has to be handlified.
   Handle<JSObject> self(this);
   Handle<Object> value(value_raw);
   PropertyAttributes old_attributes = self->GetLocalElementAttribute(index);
   Handle<Object> old_value = isolate->factory()->the_hole_value();
   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());
   }
 
   // Check for lookup interceptor
   MaybeObject* result = self->HasIndexedInterceptor()
     ? self->SetElementWithInterceptor(
         index, *value, attributes, strict_mode, check_prototype, set_mode)
     : self->SetElementWithoutInterceptor(
@@ skipping 3522 matching lines @@
   set_year(Smi::FromInt(year), SKIP_WRITE_BARRIER);
   set_month(Smi::FromInt(month), SKIP_WRITE_BARRIER);
   set_day(Smi::FromInt(day), SKIP_WRITE_BARRIER);
   set_weekday(Smi::FromInt(weekday), SKIP_WRITE_BARRIER);
   set_hour(Smi::FromInt(hour), SKIP_WRITE_BARRIER);
   set_min(Smi::FromInt(min), SKIP_WRITE_BARRIER);
   set_sec(Smi::FromInt(sec), SKIP_WRITE_BARRIER);
 }
 
 } }  // namespace v8::internal