Stop sending Object.observe notifications for API accessor properties

Such properties never notified prior to r21558, but the combination of
that change and r23163 led to sending notifications when they were
set via Object.defineProperty (but not when set via other means).

This also allows some cleanup in v8natives.js and objects.cc,
both of which were doing unnecessary contortions to produce the right
change records.

BUG=v8:3745
LOG=n

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

Cr-Commit-Position: refs/heads/master@{#25806}

src/objects.cc

@@ -3996,20 +3996,11 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
 
       case LookupIterator::ACCESSOR: {
         PropertyDetails details = it.property_details();
-        Handle<Object> old_value = it.isolate()->factory()->the_hole_value();
         // Ensure the context isn't changed after calling into accessors.
         AssertNoContextChange ncc(it.isolate());
 
         Handle<Object> accessors = it.GetAccessors();
 
-        if (is_observed && accessors->IsAccessorInfo()) {
-          ASSIGN_RETURN_ON_EXCEPTION(
-              it.isolate(), old_value,
-              GetPropertyWithAccessor(it.GetReceiver(), it.name(),
-                                      it.GetHolder<JSObject>(), accessors),
-              Object);
-        }
-
         // Special handling for ExecutableAccessorInfo, which behaves like a
         // data property.
         if (handling == DONT_FORCE_FIELD &&
@@ -4024,21 +4015,6 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
           DCHECK(result->SameValue(*value));
 
           if (details.attributes() == attributes) {
-            // Regular property update if the attributes match.
-            if (is_observed && !old_value->SameValue(*value)) {
-              // If we are setting the prototype of a function and are
-              // observed, don't send change records because the prototype
-              // handles that itself.
-              if (!object->IsJSFunction() ||
-                  !Name::Equals(it.isolate()->factory()->prototype_string(),
-                                name) ||
-                  !Handle<JSFunction>::cast(object)->should_have_prototype()) {
-                RETURN_ON_EXCEPTION(
-                    it.isolate(),
-                    EnqueueChangeRecord(object, "update", name, old_value),
-                    Object);
-              }
-            }
             return value;
           }
 
@@ -4052,12 +4028,10 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
           if (attributes & READ_ONLY) new_data->clear_setter();
           SetPropertyCallback(object, name, new_data, attributes);
           if (is_observed) {
-            if (old_value->SameValue(*value)) {
-              old_value = it.isolate()->factory()->the_hole_value();
-            }
             RETURN_ON_EXCEPTION(
                 it.isolate(),
-                EnqueueChangeRecord(object, "reconfigure", name, old_value),
+                EnqueueChangeRecord(object, "reconfigure", name,
+                                    it.isolate()->factory()->the_hole_value()),
                 Object);
           }
           return value;
@@ -4068,12 +4042,10 @@ MaybeHandle<Object> JSObject::SetOwnPropertyIgnoreAttributes(
         it.WriteDataValue(value);
 
         if (is_observed) {
-          if (old_value->SameValue(*value)) {
-            old_value = it.isolate()->factory()->the_hole_value();
-          }
           RETURN_ON_EXCEPTION(
               it.isolate(),
-              EnqueueChangeRecord(object, "reconfigure", name, old_value),
+              EnqueueChangeRecord(object, "reconfigure", name,
+                                  it.isolate()->factory()->the_hole_value()),
               Object);
         }
 

src/v8natives.js

@@ -887,15 +887,6 @@ function DefineArrayProperty(obj, p, desc, should_throw) {
         break;
       }
     }
-    // Make sure the below call to DefineObjectProperty() doesn't overwrite
-    // any magic "length" property by removing the value.
-    // TODO(mstarzinger): This hack should be removed once we have addressed the
-    // respective TODO in Runtime_DefineDataPropertyUnchecked.
-    // For the time being, we need a hack to prevent Object.observe from
-    // generating two change records.
-    obj.length = new_length;
-    desc.value_ = UNDEFINED;
-    desc.hasValue_ = false;
     threw = !DefineObjectProperty(obj, "length", desc, should_throw) || threw;
     if (emit_splice) {
       EndPerformSplice(obj);

test/cctest/test-object-observe.cc

@@ -805,3 +805,33 @@ TEST(ObjectObserveCallsFunctionTemplateInstance) {
       "obj.bar = 2;");
   CHECK_EQ(2, numRecordsSent);
 }
+
+
+static void AccessorGetter(Local<String> property,
+                           const PropertyCallbackInfo<Value>& info) {
+  info.GetReturnValue().Set(Integer::New(info.GetIsolate(), 42));
+}
+
+
+static void AccessorSetter(Local<String> property, Local<Value> value,
+                           const PropertyCallbackInfo<void>& info) {
+  info.GetReturnValue().SetUndefined();
+}
+
+
+TEST(APIAccessorsShouldNotNotify) {
+  Isolate* isolate = CcTest::isolate();
+  HandleScope handle_scope(isolate);
+  LocalContext context(isolate);
+  Handle<Object> object = Object::New(isolate);
+  object->SetAccessor(String::NewFromUtf8(isolate, "accessor"), &AccessorGetter,
+                      &AccessorSetter);
+  context->Global()->Set(String::NewFromUtf8(isolate, "obj"), object);
+  CompileRun(
+      "var records = null;"
+      "Object.observe(obj, function(r) { records = r });"
+      "obj.accessor = 43;");
+  CHECK(CompileRun("records")->IsNull());
+  CompileRun("Object.defineProperty(obj, 'accessor', { value: 44 });");
+  CHECK(CompileRun("records")->IsNull());
+}