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

Extended ElementAccessor interface to allow querying PropertyType and
AccessorPair. Also added respective functionality to JSObject.

R=mstarzinger@chromium.org
BUG=

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

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

src/elements.cc

@@ -586,6 +586,49 @@ class ElementsAccessorBase : public ElementsAccessor {
     return backing_store->is_the_hole(key) ? ABSENT : NONE;
   }
 
+  MUST_USE_RESULT virtual PropertyType GetType(
+      Object* receiver,
+      JSObject* holder,
+      uint32_t key,
+      FixedArrayBase* backing_store) {
+    if (backing_store == NULL) {
+      backing_store = holder->elements();
+    }
+    return ElementsAccessorSubclass::GetTypeImpl(
+        receiver, holder, key, BackingStore::cast(backing_store));
+  }
+
+  MUST_USE_RESULT static PropertyType GetTypeImpl(
+        Object* receiver,
+        JSObject* obj,
+        uint32_t key,
+        BackingStore* backing_store) {
+    if (key >= ElementsAccessorSubclass::GetCapacityImpl(backing_store)) {
+      return NONEXISTENT;
+    }
+    return backing_store->is_the_hole(key) ? NONEXISTENT : FIELD;
+  }
+
+  MUST_USE_RESULT virtual AccessorPair* GetAccessorPair(
+      Object* receiver,
+      JSObject* holder,
+      uint32_t key,
+      FixedArrayBase* backing_store) {
+    if (backing_store == NULL) {
+      backing_store = holder->elements();
+    }
+    return ElementsAccessorSubclass::GetAccessorPairImpl(
+        receiver, holder, key, BackingStore::cast(backing_store));
+  }
+
+  MUST_USE_RESULT static AccessorPair* GetAccessorPairImpl(
+        Object* receiver,
+        JSObject* obj,
+        uint32_t key,
+        BackingStore* backing_store) {
+    return NULL;
+  }
+
   MUST_USE_RESULT virtual MaybeObject* SetLength(JSArray* array,
                                                  Object* length) {
     return ElementsAccessorSubclass::SetLengthImpl(
@@ -1172,7 +1215,17 @@ class ExternalElementsAccessor
       BackingStore* backing_store) {
     return
         key < ExternalElementsAccessorSubclass::GetCapacityImpl(backing_store)
-        ? NONE : ABSENT;
+          ? NONE : ABSENT;
+  }
+
+  MUST_USE_RESULT static PropertyType GetTypeImpl(
+      Object* receiver,
+      JSObject* obj,
+      uint32_t key,
+      BackingStore* backing_store) {
+    return
+        key < ExternalElementsAccessorSubclass::GetCapacityImpl(backing_store)
+          ? FIELD : NONEXISTENT;
   }
 
   MUST_USE_RESULT static MaybeObject* SetLengthImpl(
@@ -1475,6 +1528,32 @@ class DictionaryElementsAccessor
     return ABSENT;
   }
 
+  MUST_USE_RESULT static PropertyType GetTypeImpl(
+      Object* receiver,
+      JSObject* obj,
+      uint32_t key,
+      SeededNumberDictionary* backing_store) {
+    int entry = backing_store->FindEntry(key);
+    if (entry != SeededNumberDictionary::kNotFound) {
+      return backing_store->DetailsAt(entry).type();
+    }
+    return NONEXISTENT;
+  }
+
+  MUST_USE_RESULT static AccessorPair* GetAccessorPairImpl(
+      Object* receiver,
+      JSObject* obj,
+      uint32_t key,
+      BackingStore* backing_store) {
+    int entry = backing_store->FindEntry(key);
+    if (entry != SeededNumberDictionary::kNotFound &&
+        backing_store->DetailsAt(entry).type() == CALLBACKS &&
+        backing_store->ValueAt(entry)->IsAccessorPair()) {
+      return AccessorPair::cast(backing_store->ValueAt(entry));
+    }
+    return NULL;
+  }
+
   static bool HasElementImpl(Object* receiver,
                              JSObject* holder,
                              uint32_t key,
@@ -1550,6 +1629,38 @@ class NonStrictArgumentsElementsAccessor : public ElementsAccessorBase<
     }
   }
 
+  MUST_USE_RESULT static PropertyType GetTypeImpl(
+      Object* receiver,
+      JSObject* obj,
+      uint32_t key,
+      FixedArray* parameter_map) {
+    Object* probe = GetParameterMapArg(obj, parameter_map, key);
+    if (!probe->IsTheHole()) {
+      return FIELD;
+    } else {
+      // If not aliased, check the arguments.
+      FixedArray* arguments = FixedArray::cast(parameter_map->get(1));
+      return ElementsAccessor::ForArray(arguments)->GetType(
+          receiver, obj, key, arguments);
+    }
+  }
+
+  MUST_USE_RESULT static AccessorPair* GetAccessorPairImpl(
+      Object* receiver,
+      JSObject* obj,
+      uint32_t key,
+      FixedArray* parameter_map) {
+    Object* probe = GetParameterMapArg(obj, parameter_map, key);
+    if (!probe->IsTheHole()) {
+      return NULL;
+    } else {
+      // If not aliased, check the arguments.
+      FixedArray* arguments = FixedArray::cast(parameter_map->get(1));
+      return ElementsAccessor::ForArray(arguments)->GetAccessorPair(
+          receiver, obj, key, arguments);
+    }
+  }
+
   MUST_USE_RESULT static MaybeObject* SetLengthImpl(
       JSObject* obj,
       Object* length,

src/elements.h

@@ -82,6 +82,28 @@ class ElementsAccessor {
       uint32_t key,
       FixedArrayBase* backing_store = NULL) = 0;
 
+  // Returns an element's type, or NONEXISTENT if there is no such
+  // element. This method doesn't iterate up the prototype chain.  The caller
+  // can optionally pass in the backing store to use for the check, which must
+  // be compatible with the ElementsKind of the ElementsAccessor. If
+  // backing_store is NULL, the holder->elements() is used as the backing store.
+  MUST_USE_RESULT virtual PropertyType GetType(
+      Object* receiver,
+      JSObject* holder,
+      uint32_t key,
+      FixedArrayBase* backing_store = NULL) = 0;
+
+  // Returns an element's accessors, or NULL if the element does not exist or
+  // is plain. This method doesn't iterate up the prototype chain.  The caller
+  // can optionally pass in the backing store to use for the check, which must
+  // be compatible with the ElementsKind of the ElementsAccessor. If
+  // backing_store is NULL, the holder->elements() is used as the backing store.
+  MUST_USE_RESULT virtual AccessorPair* GetAccessorPair(
+      Object* receiver,
+      JSObject* holder,
+      uint32_t key,
+      FixedArrayBase* backing_store = NULL) = 0;
+
   // Modifies the length data property as specified for JSArrays and resizes the
   // underlying backing store accordingly. The method honors the semantics of
   // changing array sizes as defined in EcmaScript 5.1 15.4.5.2, i.e. array that

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 {
@@ -9603,7 +9603,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();
@@ -9617,7 +9653,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
@@ -10370,8 +10406,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());

src/objects.h

@@ -1842,8 +1842,13 @@ class JSObject: public JSReceiver {
     return old_capacity + (old_capacity >> 1) + 16;
   }
 
+  PropertyType GetLocalPropertyType(String* name);
+  PropertyType GetLocalElementType(uint32_t index);
+  AccessorPair* GetLocalPropertyAccessorPair(String* name);
+  AccessorPair* GetLocalElementAccessorPair(uint32_t index);
+
   // Tells whether the index'th element is present and how it is stored.
-  enum LocalElementType {
+  enum LocalElementKind {
     // There is no element with given index.
     UNDEFINED_ELEMENT,
 
@@ -1860,7 +1865,7 @@ class JSObject: public JSReceiver {
     DICTIONARY_ELEMENT
   };
 
-  LocalElementType GetLocalElementType(uint32_t index);
+  LocalElementKind GetLocalElementKind(uint32_t index);
 
   MUST_USE_RESULT MaybeObject* SetFastElement(uint32_t index,
                                               Object* value,

src/runtime.cc

@@ -1091,7 +1091,7 @@ static MaybeObject* GetOwnProperty(Isolate* isolate,
   // This could be an element.
   uint32_t index;
   if (name->AsArrayIndex(&index)) {
-    switch (obj->GetLocalElementType(index)) {
+    switch (obj->GetLocalElementKind(index)) {
       case JSObject::UNDEFINED_ELEMENT:
         return heap->undefined_value();
 
@@ -4727,7 +4727,7 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_IsPropertyEnumerable) {
 
   uint32_t index;
   if (key->AsArrayIndex(&index)) {
-    JSObject::LocalElementType type = object->GetLocalElementType(index);
+    JSObject::LocalElementKind type = object->GetLocalElementKind(index);
     switch (type) {
       case JSObject::UNDEFINED_ELEMENT:
       case JSObject::STRING_CHARACTER_ELEMENT:

test/mjsunit/harmony/object-observe.js

@@ -56,6 +56,7 @@ function createObserver() {
     assertCallbackRecords: function(recs) {
       this.assertRecordCount(recs.length);
       for (var i = 0; i < recs.length; i++) {
+        print(i, JSON.stringify(this.records[i]), JSON.stringify(recs[i]))
         assertSame(this.records[i].object, recs[i].object);
         assertEquals('string', typeof recs[i].type);
         assertPropertiesEqual(this.records[i], recs[i]);
@@ -320,6 +321,7 @@ obj[1] = 7;  // ignored
 Object.defineProperty(obj, "1", {value: 8});
 Object.defineProperty(obj, "1", {value: 7, writable: true});
 Object.defineProperty(obj, "1", {get: function() {}});
+Object.defineProperty(obj, "1", {get: function() {}});
 delete obj[1];
 delete obj[1];
 Object.defineProperty(obj, "1", {get: function() {}, configurable: true});
@@ -339,11 +341,10 @@ observer.assertCallbackRecords([
   { object: obj, name: "1", type: "updated", oldValue: 6 },
   { object: obj, name: "1", type: "reconfigured", oldValue: 8 },
   { object: obj, name: "1", type: "reconfigured", oldValue: 7 },
-  // TODO(observe): oldValue should not be present below.
-  { object: obj, name: "1", type: "deleted", oldValue: undefined },
+  { object: obj, name: "1", type: "reconfigured" },
+  { object: obj, name: "1", type: "deleted" },
   { object: obj, name: "1", type: "new" },
-  // TODO(observe): oldValue should be absent below, and type = "reconfigured".
-  { object: obj, name: "1", type: "updated", oldValue: undefined },
+  { object: obj, name: "1", type: "reconfigured" },
   { object: obj, name: "1", type: "updated", oldValue: 9 },
   { object: obj, name: "1", type: "deleted", oldValue: 10 },
   { object: obj, name: "1", type: "new" },