[api] Throw in defineProperty() when necessary.

test/cctest/test-api-interceptors.cc

Index: test/cctest/test-api-interceptors.cc
diff --git a/test/cctest/test-api-interceptors.cc b/test/cctest/test-api-interceptors.cc
index ae96b1de4422a7cc125bd953f9699a7843c42a74..dd632e093e6ea6dc3d5ac8f3690a2d2db0c5568b 100644
--- a/test/cctest/test-api-interceptors.cc
+++ b/test/cctest/test-api-interceptors.cc
@@ -488,11 +488,6 @@ void GetterCallback(Local<Name> property,
   get_was_called = true;
 }
 
-void SetterCallback(Local<Name> property, Local<Value> value,
-                    const v8::PropertyCallbackInfo<v8::Value>& info) {
-  set_was_called = true;
-}
-
 }  // namespace
 
 // Check that get callback is called in defineProperty with accessor descriptor.
@@ -501,7 +496,7 @@ THREADED_TEST(DefinerCallbackAccessorInterceptor) {
   v8::Local<v8::FunctionTemplate> templ =
       v8::FunctionTemplate::New(CcTest::isolate());
   templ->InstanceTemplate()->SetHandler(
-      v8::NamedPropertyHandlerConfiguration(GetterCallback, SetterCallback));
+      v8::NamedPropertyHandlerConfiguration(GetterCallback));
   LocalContext env;
   env->Global()
       ->Set(env.local(), v8_str("obj"), templ->GetFunction(env.local())
@@ -511,13 +506,11 @@ THREADED_TEST(DefinerCallbackAccessorInterceptor) {
       .FromJust();
 
   CHECK_EQ(get_was_called, false);
-  CHECK_EQ(set_was_called, false);
 
   v8_compile("Object.defineProperty(obj, 'x', {set: function() {return 17;}});")
       ->Run(env.local())
       .ToLocalChecked();
   CHECK_EQ(get_was_called, true);
-  CHECK_EQ(set_was_called, false);
 }
 
 bool get_was_called_in_order = false;
@@ -547,7 +540,7 @@ THREADED_TEST(DefinerCallbackGetAndDefine) {
   v8::Local<v8::FunctionTemplate> templ =
       v8::FunctionTemplate::New(CcTest::isolate());
   templ->InstanceTemplate()->SetHandler(v8::NamedPropertyHandlerConfiguration(
-      GetterCallbackOrder, SetterCallback, 0, 0, 0, DefinerCallbackOrder));
+      GetterCallbackOrder, 0, 0, 0, 0, DefinerCallbackOrder));
   LocalContext env;
   env->Global()
       ->Set(env.local(), v8_str("obj"), templ->GetFunction(env.local())
@@ -1322,6 +1315,74 @@ THREADED_TEST(NamedPropertyHandlerGetter) {
             .FromJust());
 }
 
+// See https://bugs.chromium.org/p/chromium/issues/detail?id=645542
+THREADED_TEST(PropertySetterCallbackPresentDuringDefineProperty) {
+  {  // defineProperty with a data descriptor is OK in presence of named
+    // setter callback.
+    v8::HandleScope scope(CcTest::isolate());
+    LocalContext env;
+    v8::Local<v8::FunctionTemplate> templ =
+        v8::FunctionTemplate::New(CcTest::isolate());
+    templ->InstanceTemplate()->SetHandler(
+        v8::NamedPropertyHandlerConfiguration(0, EmptyInterceptorSetter));
+    env->Global()
+        ->Set(env.local(), v8_str("obj"), templ->GetFunction(env.local())
+                                              .ToLocalChecked()
+                                              ->NewInstance(env.local())
+                                              .ToLocalChecked())
+        .FromJust();
+    const char* code =
+        "Object.defineProperty(obj, 'x', {value: 42});"
+        "obj.x;";
+    CHECK_EQ(42, v8_compile(code)
+                     ->Run(env.local())
+                     .ToLocalChecked()
+                     ->Int32Value(env.local())
+                     .FromJust());
+  }
+
+  {  // defineProperty with a descriptor that is not a data descriptor should
+     // throw if object has a
+     // named setter callback.
+    v8::HandleScope scope(CcTest::isolate());
+    LocalContext env;
+    v8::Local<v8::FunctionTemplate> templ =
+        v8::FunctionTemplate::New(CcTest::isolate());
+    templ->InstanceTemplate()->SetHandler(
+        v8::NamedPropertyHandlerConfiguration(0, EmptyInterceptorSetter));
+    env->Global()
+        ->Set(env.local(), v8_str("obj"), templ->GetFunction(env.local())
+                                              .ToLocalChecked()
+                                              ->NewInstance(env.local())
+                                              .ToLocalChecked())
+        .FromJust();
+
+    v8::TryCatch try_catch(env->GetIsolate());
+    CompileRun("Object.defineProperty(obj, 'x', {get: function() {}});");
+    CHECK(try_catch.HasCaught());
+  }
+
+  {  // defineProperty with a descriptor that is not a data descriptor, e.g.,
+     // empty descriptor, should throw if object has a named setter callback.
+    v8::HandleScope scope(CcTest::isolate());
+    LocalContext env;
+    v8::Local<v8::FunctionTemplate> templ =
+        v8::FunctionTemplate::New(CcTest::isolate());
+    templ->InstanceTemplate()->SetHandler(
+        v8::NamedPropertyHandlerConfiguration(0, EmptyInterceptorSetter));
+    env->Global()
+        ->Set(env.local(), v8_str("obj"), templ->GetFunction(env.local())
+                                              .ToLocalChecked()
+                                              ->NewInstance(env.local())
+                                              .ToLocalChecked())
+        .FromJust();
+
+    v8::TryCatch try_catch(env->GetIsolate());
+    CompileRun("Object.defineProperty(obj, 'x', {});");
+    CHECK(try_catch.HasCaught());
+  }
+}
+
 namespace {
 void NotInterceptingPropertyDefineCallback(
     Local<Name> name, const v8::PropertyDescriptor& desc,
@@ -2206,7 +2267,7 @@ THREADED_TEST(SwitchFromAccessorToInterceptorWithInheritance) {
 THREADED_TEST(SwitchFromInterceptorToJSAccessor) {
   v8::HandleScope scope(CcTest::isolate());
   Local<FunctionTemplate> templ = FunctionTemplate::New(CcTest::isolate());
-  AddInterceptor(templ, InterceptorGetter, InterceptorSetter);
+  AddInterceptor(templ, InterceptorGetter, nullptr);
   LocalContext env;
   env->Global()
       ->Set(env.local(), v8_str("Obj"),
@@ -2219,8 +2280,6 @@ THREADED_TEST(SwitchFromInterceptorToJSAccessor) {
       "function setAge(i) { obj.age = i; };"
       "Object.defineProperty(obj, 'age', { get:getter, set:setter });"
       "for(var i = 0; i <= 10000; i++) setAge(i);");
-  // All i < 10000 go to the interceptor.
-  ExpectInt32("obj.interceptor_age", 9999);
   // The last i goes to the JavaScript accessor.
   ExpectInt32("obj.accessor_age", 10000);
   // The installed JavaScript getter is still intact.
@@ -2235,7 +2294,7 @@ THREADED_TEST(SwitchFromInterceptorToJSAccessor) {
 THREADED_TEST(SwitchFromJSAccessorToInterceptor) {
   v8::HandleScope scope(CcTest::isolate());
   Local<FunctionTemplate> templ = FunctionTemplate::New(CcTest::isolate());
-  AddInterceptor(templ, InterceptorGetter, InterceptorSetter);
+  AddInterceptor(templ, InterceptorGetter, nullptr);
   LocalContext env;
   env->Global()
       ->Set(env.local(), v8_str("Obj"),
@@ -2248,14 +2307,8 @@ THREADED_TEST(SwitchFromJSAccessorToInterceptor) {
       "function setAge(i) { obj.age = i; };"
       "Object.defineProperty(obj, 'age', { get:getter, set:setter });"
       "for(var i = 20000; i >= 9999; i--) setAge(i);");
-  // All i >= 10000 go to the accessor.
-  ExpectInt32("obj.accessor_age", 10000);
-  // The last i goes to the interceptor.
-  ExpectInt32("obj.interceptor_age", 9999);
-  // The installed JavaScript getter is still intact.
-  // This last part is a regression test for issue 1651 and relies on the fact
-  // that both interceptor and accessor are being installed on the same object.
-  ExpectInt32("obj.age", 10000);
+  ExpectInt32("obj.accessor_age", 9999);
+  ExpectInt32("obj.age", 9999);
   ExpectBoolean("obj.hasOwnProperty('age')", true);
   ExpectUndefined("Object.getOwnPropertyDescriptor(obj, 'age').value");
 }
@@ -2491,22 +2544,12 @@ static void IndexedPropertyGetter(
 }
 
 
-static void IndexedPropertySetter(
-    uint32_t index, Local<Value> value,
-    const v8::PropertyCallbackInfo<v8::Value>& info) {
-  ApiTestFuzzer::Fuzz();
-  if (index == 39) {
-    info.GetReturnValue().Set(value);
-  }
-}
-
-
 THREADED_TEST(IndexedInterceptorWithIndexedAccessor) {
   v8::Isolate* isolate = CcTest::isolate();
   v8::HandleScope scope(isolate);
   Local<ObjectTemplate> templ = ObjectTemplate::New(isolate);
-  templ->SetHandler(v8::IndexedPropertyHandlerConfiguration(
-      IndexedPropertyGetter, IndexedPropertySetter));
+  templ->SetHandler(
+      v8::IndexedPropertyHandlerConfiguration(IndexedPropertyGetter, nullptr));
   LocalContext context;
   context->Global()
       ->Set(context.local(), v8_str("obj"),
@@ -2518,17 +2561,11 @@ THREADED_TEST(IndexedInterceptorWithIndexedAccessor) {
       "obj.__defineSetter__(\"17\", function(val){this.foo = val;});"
       "obj[17] = 23;"
       "obj.foo;");
-  Local<Script> interceptor_setter_script = v8_compile(
-      "obj.__defineSetter__(\"39\", function(val){this.foo = \"hit\";});"
-      "obj[39] = 47;"
-      "obj.foo;");  // This setter should not run, due to the interceptor.
   Local<Script> interceptor_getter_script = v8_compile("obj[37];");
   Local<Value> result = getter_script->Run(context.local()).ToLocalChecked();
   CHECK(v8_num(5)->Equals(context.local(), result).FromJust());
   result = setter_script->Run(context.local()).ToLocalChecked();
   CHECK(v8_num(23)->Equals(context.local(), result).FromJust());
-  result = interceptor_setter_script->Run(context.local()).ToLocalChecked();
-  CHECK(v8_num(23)->Equals(context.local(), result).FromJust());
   result = interceptor_getter_script->Run(context.local()).ToLocalChecked();
   CHECK(v8_num(625)->Equals(context.local(), result).FromJust());
 }
@@ -3964,7 +4001,7 @@ THREADED_TEST(CrankshaftInterceptorSetter) {
   i::FLAG_allow_natives_syntax = true;
   v8::HandleScope scope(CcTest::isolate());
   Local<FunctionTemplate> templ = FunctionTemplate::New(CcTest::isolate());
-  AddInterceptor(templ, InterceptorGetter, InterceptorSetter);
+  AddInterceptor(templ, InterceptorGetter, nullptr);
   LocalContext env;
   env->Global()
       ->Set(env.local(), v8_str("Obj"),
@@ -3984,9 +4021,8 @@ THREADED_TEST(CrankshaftInterceptorSetter) {
       "setAge(3);"
       "%OptimizeFunctionOnNextCall(setAge);"
       "setAge(4);");
-  // All stores went through the interceptor.
-  ExpectInt32("obj.interceptor_age", 4);
-  ExpectInt32("obj.accessor_age", 42);
+  ExpectUndefined("obj.interceptor_age");
+  ExpectInt32("obj.accessor_age", 4);
 }