Object.observe: generate change records for named properties.

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 906f7feef6bb30f5bf9f3787e568f1b7f9405a30..472f45c79e79b2bdc8b134ace5072f58cdf631d6 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -1677,6 +1677,7 @@ MaybeObject* JSObject::AddProperty(String* name,
   ASSERT(!IsJSGlobalProxy());
   Map* map_of_this = map();
   Heap* heap = GetHeap();
+  MaybeObject* ret;

[Toon Verwaest, 2012/11/05 13:33:22]

Can we call this maybe_result for consistency?

[rossberg, 2012/11/05 17:11:08]

Done.

   if (extensibility_check == PERFORM_EXTENSIBILITY_CHECK &&
       !map_of_this->is_extensible()) {
     if (strict_mode == kNonStrictMode) {
@@ -1688,28 +1689,61 @@ MaybeObject* JSObject::AddProperty(String* name,
                                  HandleVector(args, 1)));
     }
   }
+
   if (HasFastProperties()) {
     // Ensure the descriptor array does not get too big.
     if (map_of_this->NumberOfOwnDescriptors() <
         DescriptorArray::kMaxNumberOfDescriptors) {
       if (value->IsJSFunction()) {
-        return AddConstantFunctionProperty(name,
-                                           JSFunction::cast(value),
-                                           attributes);
+        ret = AddConstantFunctionProperty(name,
+                                             JSFunction::cast(value),

[rafaelw, 2012/10/31 14:44:31]

whitespace

[rossberg, 2012/11/05 17:11:08]

Done.

+                                             attributes);
       } else {
-        return AddFastProperty(name, value, attributes, store_mode);
+        ret = AddFastProperty(name, value, attributes, store_mode);
       }
     } else {
       // Normalize the object to prevent very large instance descriptors.
       // This eliminates unwanted N^2 allocation and lookup behavior.
       Object* obj;
-      { MaybeObject* maybe_obj =
-            NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
-        if (!maybe_obj->ToObject(&obj)) return maybe_obj;
-      }
+      ret = NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
+      if (!ret->ToObject(&obj)) return ret;

[Toon Verwaest, 2012/11/05 13:33:22]

if (ret->IsFailure()) return ret;

I actually prefer:
MaybeObject* maybe_failure = NormalizeProperties(...;
if (maybe_failure->IsFailure()) return maybe_failure;

since ret will be overwritten in the next line anyway. The values seem
independent.

[rossberg, 2012/11/05 17:11:08]

Done.

+      ret = AddSlowProperty(name, value, attributes);
     }
+  } else {
+    ret = AddSlowProperty(name, value, attributes);
   }
-  return AddSlowProperty(name, value, attributes);
+
+  if (FLAG_harmony_observation && map()->is_observed() && !ret->IsFailure()) {
+    this->NotifyObservers("new", name, heap->the_hole_value());
+  }
+
+  return ret;

[rafaelw, 2012/10/31 14:44:31]

Isn't this unsafe? The notifyObservers could have called GC.  Don't you need to:

if (ret->IsFailure()) return ret;
Handle<Object> return_handle(return_value->ToObjectUnchecked());
// call NotifyObservers
return *return_handle;

[Toon Verwaest, 2012/11/05 13:33:22]

Yes. We should not jump into handlified code from non-handlified code. This is
very prone to errors, especially since the caller of the function does not know
that handlified code may cause GC without returning failure, potentially
invalidating all raw pointers in the client code.

Either we have to ensure that no code is handlified for implementing
NotifyObservers, or we'll have to pull it up to the handlified client of this
functionality.

On 2012/10/31 14:44:31, rafaelw wrote:

[rossberg, 2012/11/05 17:11:08]

Indeed.

[rossberg, 2012/11/05 17:11:08]

Most of the methods in question are already documented as "can cause GC", and I
added respective comments to the remaining ones (plus, checked all their call
sites).

As discussed offline, there is no real alternative short of "handlifying all the
code" -- which I agree we should do at some point.

+}
+
+
+void JSObject::NotifyObservers(
+    const char* type_raw, String* name_raw, Object* oldValue_raw) {
+  Isolate* isolate = GetIsolate();
+  HandleScope scope;
+  Handle<String> type = isolate->factory()->LookupAsciiSymbol(type_raw);
+  Handle<JSObject> object(this);
+  Handle<String> name(name_raw);
+  bool threw;
+  if (oldValue_raw->IsTheHole()) {

[rafaelw, 2012/10/31 14:44:31]

nit: This can be cleaner if you make argv a ScopedVector<Handle<Object> >, which
contains either 3 or 4 values based on whether oldValue_raw->IsTheHole().

[rossberg, 2012/11/05 17:11:08]

Well, that would make the code pretty verbose as well. I removed the duplication
using a simpler trick.

+    Handle<Object> args[] = { type, object, name };
+    Execution::Call(Handle<JSFunction>(isolate->observers_notify_change()),
+                    Handle<Object>(isolate->heap()->undefined_value()),
+                    ARRAY_SIZE(args), args,
+                    &threw);
+  } else {
+    Handle<Object> oldValue(oldValue_raw);
+    Handle<Object> args[] = { type, object, name, oldValue };
+    Execution::Call(Handle<JSFunction>(isolate->observers_notify_change()),
+                    Handle<Object>(isolate->heap()->undefined_value()),
+                    ARRAY_SIZE(args), args,
+                    &threw);
+  }
+  ASSERT(!threw);
 }
 
 
@@ -2860,6 +2894,7 @@ MaybeObject* JSObject::SetPropertyForResult(LookupResult* result,
     return self->AddProperty(
         *name, *value, attributes, strict_mode, store_mode);
   }
+
   if (result->IsProperty() && result->IsReadOnly()) {
     if (strict_mode == kStrictMode) {
       Handle<Object> args[] = { name, self };
@@ -2870,19 +2905,28 @@ MaybeObject* JSObject::SetPropertyForResult(LookupResult* result,
     }
   }
 
+  Object* oldValue = heap->the_hole_value();
+  if (FLAG_harmony_observation && map()->is_observed()) {
+    // TODO(observe): save oldValue
+  }
+
   // This is a real property that is not read-only, or it is a
   // transition or null descriptor and there are no setters in the prototypes.
+  MaybeObject* ret = *value;
   switch (result->type()) {
     case NORMAL:
-      return self->SetNormalizedProperty(result, *value);
+      ret = self->SetNormalizedProperty(result, *value);
+      break;
     case FIELD:
-      return self->FastPropertyAtPut(result->GetFieldIndex(), *value);
+      ret = self->FastPropertyAtPut(result->GetFieldIndex(), *value);
+      break;
     case CONSTANT_FUNCTION:
       // Only replace the function if necessary.
       if (*value == result->GetConstantFunction()) return *value;
       // Preserve the attributes of this existing property.
       attributes = result->GetAttributes();
-      return self->ConvertDescriptorToField(*name, *value, attributes);
+      ret = self->ConvertDescriptorToField(*name, *value, attributes);
+      break;
     case CALLBACKS: {
       Object* callback_object = result->GetCallbackObject();
       return self->SetPropertyWithCallback(callback_object,
@@ -2892,10 +2936,11 @@ MaybeObject* JSObject::SetPropertyForResult(LookupResult* result,
                                            strict_mode);
     }
     case INTERCEPTOR:
-      return self->SetPropertyWithInterceptor(*name,
-                                              *value,
-                                              attributes,
-                                              strict_mode);
+      ret = self->SetPropertyWithInterceptor(*name,
+                                             *value,
+                                             attributes,
+                                             strict_mode);
+      break;
     case TRANSITION: {
       Map* transition_map = result->GetTransitionTarget();
       int descriptor = transition_map->LastAdded();
@@ -2906,37 +2951,43 @@ MaybeObject* JSObject::SetPropertyForResult(LookupResult* result,
       if (details.type() == FIELD) {
         if (attributes == details.attributes()) {
           int field_index = descriptors->GetFieldIndex(descriptor);
-          return self->AddFastPropertyUsingMap(transition_map,
-                                               *name,
-                                               *value,
-                                               field_index);
+          ret = self->AddFastPropertyUsingMap(transition_map,
+                                              *name,
+                                              *value,
+                                              field_index);
+        } else {
+          ret = self->ConvertDescriptorToField(*name, *value, attributes);
         }
-        return self->ConvertDescriptorToField(*name, *value, attributes);
       } else if (details.type() == CALLBACKS) {
-        return ConvertDescriptorToField(*name, *value, attributes);
-      }
-
-      ASSERT(details.type() == CONSTANT_FUNCTION);
-
-      Object* constant_function = descriptors->GetValue(descriptor);
-      // If the same constant function is being added we can simply
-      // transition to the target map.
-      if (constant_function == *value) {
-        self->set_map(transition_map);
-        return constant_function;
+        ret = ConvertDescriptorToField(*name, *value, attributes);
+      } else {
+        ASSERT(details.type() == CONSTANT_FUNCTION);
+
+        Object* constant_function = descriptors->GetValue(descriptor);
+        if (constant_function == *value) {
+          // If the same constant function is being added we can simply
+          // transition to the target map.
+          self->set_map(transition_map);
+          ret = constant_function;
+        } else {
+          // Otherwise, replace with a map transition to a new map with a FIELD,
+          // even if the value is a constant function.
+          ret = ConvertTransitionToMapTransition(
+              result->GetTransitionIndex(), *name, *value, attributes);
+        }
       }
-      // Otherwise, replace with a map transition to a new map with a FIELD,
-      // even if the value is a constant function.
-      return ConvertTransitionToMapTransition(
-          result->GetTransitionIndex(), *name, *value, attributes);
+      break;
     }
     case HANDLER:
     case NONEXISTENT:
       UNREACHABLE();
-      return *value;
   }
-  UNREACHABLE();  // keep the compiler happy
-  return *value;
+
+  if (FLAG_harmony_observation && map()->is_observed() && !ret->IsFailure()) {
+    this->NotifyObservers("updated", *name, oldValue);
+  }
+
+  return ret;

[rafaelw, 2012/10/31 14:44:31]

same question about returning MaybeObject after invoking script.

[rossberg, 2012/11/05 17:11:08]

Done.

 }
 
 
@@ -2999,24 +3050,34 @@ MaybeObject* JSObject::SetLocalPropertyIgnoreAttributes(
     return AddProperty(name, value, attributes, kNonStrictMode);
   }
 
+  Object* oldValue = isolate->heap()->the_hole_value();
+  if (FLAG_harmony_observation && map()->is_observed()) {
+    // TODO(observe): save oldValue
+  }
+
   // Check of IsReadOnly removed from here in clone.
+  MaybeObject* ret = value;
   switch (result.type()) {
     case NORMAL: {
       PropertyDetails details = PropertyDetails(attributes, NORMAL);
-      return SetNormalizedProperty(name, value, details);
+      ret = SetNormalizedProperty(name, value, details);
+      break;
     }
     case FIELD:
-      return FastPropertyAtPut(result.GetFieldIndex(), value);
+      ret = FastPropertyAtPut(result.GetFieldIndex(), value);
+      break;
     case CONSTANT_FUNCTION:
       // Only replace the function if necessary.
       if (value == result.GetConstantFunction()) return value;
       // Preserve the attributes of this existing property.
       attributes = result.GetAttributes();
-      return ConvertDescriptorToField(name, value, attributes);
+      ret = ConvertDescriptorToField(name, value, attributes);
+      break;
     case CALLBACKS:
     case INTERCEPTOR:
       // Override callback in clone
-      return ConvertDescriptorToField(name, value, attributes);
+      ret = ConvertDescriptorToField(name, value, attributes);
+      break;
     case TRANSITION: {
       Map* transition_map = result.GetTransitionTarget();
       int descriptor = transition_map->LastAdded();
@@ -3027,29 +3088,37 @@ MaybeObject* JSObject::SetLocalPropertyIgnoreAttributes(
       if (details.type() == FIELD) {
         if (attributes == details.attributes()) {
           int field_index = descriptors->GetFieldIndex(descriptor);
-          return AddFastPropertyUsingMap(transition_map,
-                                         name,
-                                         value,
-                                         field_index);
+          ret = AddFastPropertyUsingMap(transition_map,
+                                        name,
+                                        value,
+                                        field_index);
+        } else {
+          ret = ConvertDescriptorToField(name, value, attributes);
         }
-        return ConvertDescriptorToField(name, value, attributes);
       } else if (details.type() == CALLBACKS) {
-        return ConvertDescriptorToField(name, value, attributes);
-      }
-
-      ASSERT(details.type() == CONSTANT_FUNCTION);
+        ret = ConvertDescriptorToField(name, value, attributes);
+      } else {
+        ASSERT(details.type() == CONSTANT_FUNCTION);
 
-      // Replace transition to CONSTANT FUNCTION with a map transition to a new
-      // map with a FIELD, even if the value is a function.
-      return ConvertTransitionToMapTransition(
-          result.GetTransitionIndex(), name, value, attributes);
+        // Replace transition to CONSTANT FUNCTION with a map transition to a new
+        // map with a FIELD, even if the value is a function.
+        ret = ConvertTransitionToMapTransition(
+            result.GetTransitionIndex(), name, value, attributes);
+      }
+      break;
     }
     case HANDLER:
     case NONEXISTENT:
       UNREACHABLE();
   }
-  UNREACHABLE();  // keep the compiler happy
-  return value;
+
+  if (FLAG_harmony_observation && map()->is_observed() && !ret->IsFailure()) {
+    const char* type =
+        attributes == result.GetAttributes() ? "updated" : "reconfigured";
+    this->NotifyObservers(type, name, oldValue);
+  }
+
+  return ret;

[rafaelw, 2012/10/31 14:44:31]

same question about returning MaybeObject

[rossberg, 2012/11/05 17:11:08]

Done.

 }
 
 
@@ -3953,38 +4022,51 @@ MaybeObject* JSObject::DeleteProperty(String* name, DeleteMode mode) {
   uint32_t index = 0;
   if (name->AsArrayIndex(&index)) {
     return DeleteElement(index, mode);
-  } else {
-    LookupResult result(isolate);
-    LocalLookup(name, &result);
-    if (!result.IsFound()) return isolate->heap()->true_value();
-    // Ignore attributes if forcing a deletion.
-    if (result.IsDontDelete() && mode != FORCE_DELETION) {
-      if (mode == STRICT_DELETION) {
-        // Deleting a non-configurable property in strict mode.
-        HandleScope scope(isolate);
-        Handle<Object> args[2] = { Handle<Object>(name), Handle<Object>(this) };
-        return isolate->Throw(*isolate->factory()->NewTypeError(
-            "strict_delete_property", HandleVector(args, 2)));
-      }
-      return isolate->heap()->false_value();
-    }
-    // Check for interceptor.
-    if (result.IsInterceptor()) {
-      // Skip interceptor if forcing a deletion.
-      if (mode == FORCE_DELETION) {
-        return DeletePropertyPostInterceptor(name, mode);
-      }
-      return DeletePropertyWithInterceptor(name);
+  }
+
+  LookupResult result(isolate);
+  LocalLookup(name, &result);
+  if (!result.IsFound()) return isolate->heap()->true_value();
+  // Ignore attributes if forcing a deletion.
+  if (result.IsDontDelete() && mode != FORCE_DELETION) {
+    if (mode == STRICT_DELETION) {
+      // Deleting a non-configurable property in strict mode.
+      HandleScope scope(isolate);
+      Handle<Object> args[2] = { Handle<Object>(name), Handle<Object>(this) };
+      return isolate->Throw(*isolate->factory()->NewTypeError(
+          "strict_delete_property", HandleVector(args, 2)));
     }
+    return isolate->heap()->false_value();
+  }
+
+  Object* oldValue = isolate->heap()->the_hole_value();
+  if (FLAG_harmony_observation && map()->is_observed()) {
+    // TODO(observe): save oldValue
+  }
+  MaybeObject* ret;
+
+  // Check for interceptor.
+  if (result.IsInterceptor()) {
+    // Skip interceptor if forcing a deletion.
+    if (mode == FORCE_DELETION) {
+      ret = DeletePropertyPostInterceptor(name, mode);
+    } else {
+      ret = DeletePropertyWithInterceptor(name);
+    }
+  } else {
     // Normalize object if needed.
     Object* obj;
-    { MaybeObject* maybe_obj =
-          NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
-      if (!maybe_obj->ToObject(&obj)) return maybe_obj;
-    }
+    ret = NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
+    if (!ret->ToObject(&obj)) return ret;
     // Make sure the properties are normalized before removing the entry.
-    return DeleteNormalizedProperty(name, mode);
+    ret = DeleteNormalizedProperty(name, mode);
+  }
+
+  if (FLAG_harmony_observation && map()->is_observed() && !ret->IsFailure()) {
+    this->NotifyObservers("deleted", name, oldValue);
   }
+
+  return ret;

[rafaelw, 2012/10/31 14:44:31]

ditto

[rossberg, 2012/11/05 17:11:08]

Done.

 }
 
 
@@ -4612,10 +4694,26 @@ MaybeObject* JSObject::DefineAccessor(String* name,
 
   if (!CanSetCallback(name)) return isolate->heap()->undefined_value();
 
+  Object* oldValue = isolate->heap()->the_hole_value();
+  bool preexists;
+  if (FLAG_harmony_observation && map()->is_observed()) {
+    LookupResult result(isolate);
+    LocalLookup(name, &result);
+    preexists = result.IsFound();
+    // TODO(observe): save oldValue
+  }
+
   uint32_t index = 0;
-  return name->AsArrayIndex(&index) ?
+  MaybeObject* ret = name->AsArrayIndex(&index) ?
       DefineElementAccessor(index, getter, setter, attributes) :
       DefinePropertyAccessor(name, getter, setter, attributes);
+
+  if (FLAG_harmony_observation && map()->is_observed() && !ret->IsFailure()) {
+    const char* type = preexists ? "reconfigured" : "new";
+    this->NotifyObservers(type, name, oldValue);
+  }
+
+  return ret;

[rafaelw, 2012/10/31 14:44:31]

ditto

[rossberg, 2012/11/05 17:11:08]

Done.

 }
 
 
@@ -4896,7 +4994,6 @@ MaybeObject* Map::RawCopy(int instance_size) {
   result->set_constructor(constructor());
   result->set_bit_field(bit_field());
   result->set_bit_field2(bit_field2());
-  result->set_bit_field3(bit_field3());
   int new_bit_field3 = bit_field3();
   new_bit_field3 = OwnsDescriptors::update(new_bit_field3, true);
   new_bit_field3 = NumberOfOwnDescriptorsBits::update(new_bit_field3, 0);
@@ -7519,6 +7616,8 @@ bool Map::EquivalentToForNormalization(Map* other,
     instance_type() == other->instance_type() &&
     bit_field() == other->bit_field() &&
     bit_field2() == other->bit_field2() &&
+    (bit_field3() & IsObserved::kMask) ==
+        (other->bit_field3() & IsObserved::kMask) &&

[Toon Verwaest, 2012/11/05 13:33:22]

Can we just use is_observed() == other->is_observed()?

[rossberg, 2012/11/05 17:11:08]

Done.

     function_with_prototype() == other->function_with_prototype();
 }