[runtime] Intercept function declarations.

We used to intercept function definitions, but not declarations.
GenericNamedPropertySetterCallback now also intercepts function declarations.

For definitions, we call DeclareGlobal and then InitializeVarGlobal. For
declarations, we never call InitializeVarGlobal, thus we must check for
interceptors in DeclareGlobal.

If the semantics of a redeclaration are wrong, e.g., redeclaring a read-only
property, an exception is thrown independent of whether an interceptor is
installed. Usually, i.e., not during a declaration, we only throw if
the call is not successfully intercepted.

BUG=v8:5375

Review-Url: https://codereview.chromium.org/2334733002
Cr-Commit-Position: refs/heads/master@{#39450}

src/runtime/runtime-scopes.cc

@@ -44,7 +44,7 @@ Object* ThrowRedeclarationError(Isolate* isolate, Handle<String> name,
 Object* DeclareGlobal(
     Isolate* isolate, Handle<JSGlobalObject> global, Handle<String> name,
     Handle<Object> value, PropertyAttributes attr, bool is_var,
-    bool is_function, RedeclarationType redeclaration_type,
+    bool is_function_declaration, RedeclarationType redeclaration_type,
     Handle<TypeFeedbackVector> feedback_vector = Handle<TypeFeedbackVector>(),
     FeedbackVectorSlot slot = FeedbackVectorSlot::Invalid()) {
   Handle<ScriptContextTable> script_contexts(
@@ -60,7 +60,14 @@ Object* DeclareGlobal(
   }
 
   // Do the lookup own properties only, see ES5 erratum.
-  LookupIterator it(global, name, global, LookupIterator::OWN_SKIP_INTERCEPTOR);
+  LookupIterator::Configuration lookup_config(
+      LookupIterator::Configuration::OWN_SKIP_INTERCEPTOR);
+  if (is_function_declaration) {
+    // For function declarations, use the interceptor on the declaration. For
+    // non-functions, use it only on initialization.
+    lookup_config = LookupIterator::Configuration::OWN;
+  }
+  LookupIterator it(global, name, global, lookup_config);
   Maybe<PropertyAttributes> maybe = JSReceiver::GetPropertyAttributes(&it);
   if (!maybe.IsJust()) return isolate->heap()->exception();
 
@@ -71,7 +78,7 @@ Object* DeclareGlobal(
     // Skip var re-declarations.
     if (is_var) return isolate->heap()->undefined_value();
 
-    DCHECK(is_function);
+    DCHECK(is_function_declaration);
     if ((old_attributes & DONT_DELETE) != 0) {
       // Only allow reconfiguring globals to functions in user code (no
       // natives, which are marked as read-only).
@@ -83,9 +90,9 @@ Object* DeclareGlobal(
       if (old_details.IsReadOnly() || old_details.IsDontEnum() ||
           (it.state() == LookupIterator::ACCESSOR &&
            it.GetAccessors()->IsAccessorPair())) {
-        // ES#sec-globaldeclarationinstantiation 5.d:
+        // ECMA-262 section 15.1.11 GlobalDeclarationInstantiation 5.d:
         // If hasRestrictedGlobal is true, throw a SyntaxError exception.
-        // ES#sec-evaldeclarationinstantiation 8.a.iv.1.b:
+        // ECMA-262 section 18.2.1.3 EvalDeclarationInstantiation 8.a.iv.1.b:
         // If fnDefinable is false, throw a TypeError exception.
         return ThrowRedeclarationError(isolate, name, redeclaration_type);
       }
@@ -102,6 +109,10 @@ Object* DeclareGlobal(
     if (it.state() == LookupIterator::ACCESSOR) it.Delete();
   }
 
+  if (is_function_declaration) {
+    it.Restart();
+  }
+
   // Define or redefine own property.
   RETURN_FAILURE_ON_EXCEPTION(
       isolate, JSObject::DefineOwnPropertyIgnoreAttributes(&it, value, attr));

test/cctest/test-api-interceptors.cc

@@ -482,6 +482,8 @@ THREADED_TEST(QueryInterceptor) {
 bool get_was_called = false;
 bool set_was_called = false;
 
+int set_was_called_counter = 0;
+
 namespace {
 void GetterCallback(Local<Name> property,
                     const v8::PropertyCallbackInfo<v8::Value>& info) {
@@ -491,6 +493,7 @@ void GetterCallback(Local<Name> property,
 void SetterCallback(Local<Name> property, Local<Value> value,
                     const v8::PropertyCallbackInfo<v8::Value>& info) {
   set_was_called = true;
+  set_was_called_counter++;
 }
 
 }  // namespace
@@ -510,8 +513,8 @@ THREADED_TEST(DefinerCallbackAccessorInterceptor) {
                                             .ToLocalChecked())
       .FromJust();
 
-  CHECK_EQ(get_was_called, false);
-  CHECK_EQ(set_was_called, false);
+  get_was_called = false;
+  set_was_called = false;
 
   v8_compile("Object.defineProperty(obj, 'x', {set: function() {return 17;}});")
       ->Run(env.local())
@@ -520,6 +523,93 @@ THREADED_TEST(DefinerCallbackAccessorInterceptor) {
   CHECK_EQ(set_was_called, false);
 }
 
+// Check that set callback is called for function declarations.
+THREADED_TEST(SetterCallbackFunctionDeclarationInterceptor) {
+  v8::HandleScope scope(CcTest::isolate());
+  LocalContext env;
+  v8::Local<v8::FunctionTemplate> templ =
+      v8::FunctionTemplate::New(CcTest::isolate());
+
+  v8::Local<ObjectTemplate> object_template = templ->InstanceTemplate();
+  object_template->SetHandler(
+      v8::NamedPropertyHandlerConfiguration(nullptr, SetterCallback));
+  v8::Local<v8::Context> ctx =
+      v8::Context::New(CcTest::isolate(), nullptr, object_template);
+
+  set_was_called_counter = 0;
+
+  // Declare function.
+  v8::Local<v8::String> code = v8_str("function x() {return 42;}; x();");
+  CHECK_EQ(42, v8::Script::Compile(ctx, code)
+                   .ToLocalChecked()
+                   ->Run(ctx)
+                   .ToLocalChecked()
+                   ->Int32Value(ctx)
+                   .FromJust());
+  CHECK_EQ(set_was_called_counter, 1);
+
+  // Redeclare function.
+  code = v8_str("function x() {return 43;}; x();");
+  CHECK_EQ(43, v8::Script::Compile(ctx, code)
+                   .ToLocalChecked()
+                   ->Run(ctx)
+                   .ToLocalChecked()
+                   ->Int32Value(ctx)
+                   .FromJust());
+  CHECK_EQ(set_was_called_counter, 2);
+
+  // Redefine function.
+  code = v8_str("x = function() {return 44;}; x();");
+  CHECK_EQ(44, v8::Script::Compile(ctx, code)
+                   .ToLocalChecked()
+                   ->Run(ctx)
+                   .ToLocalChecked()
+                   ->Int32Value(ctx)
+                   .FromJust());
+  CHECK_EQ(set_was_called_counter, 3);
+}
+
+// Check that function re-declarations throw if they are read-only.
+THREADED_TEST(SetterCallbackFunctionDeclarationInterceptorThrow) {
+  v8::HandleScope scope(CcTest::isolate());
+  LocalContext env;
+  v8::Local<v8::FunctionTemplate> templ =
+      v8::FunctionTemplate::New(CcTest::isolate());
+
+  v8::Local<ObjectTemplate> object_template = templ->InstanceTemplate();
+  object_template->SetHandler(
+      v8::NamedPropertyHandlerConfiguration(nullptr, SetterCallback));
+  v8::Local<v8::Context> ctx =
+      v8::Context::New(CcTest::isolate(), nullptr, object_template);
+
+  set_was_called = false;
+
+  v8::Local<v8::String> code = v8_str(
+      "function x() {return 42;};"
+      "Object.defineProperty(this, 'x', {"
+      "configurable: false, "
+      "writable: false});"
+      "x();");
+  CHECK_EQ(42, v8::Script::Compile(ctx, code)
+                   .ToLocalChecked()
+                   ->Run(ctx)
+                   .ToLocalChecked()
+                   ->Int32Value(ctx)
+                   .FromJust());
+
+  CHECK_EQ(set_was_called, true);
+
+  v8::TryCatch try_catch(CcTest::isolate());
+  set_was_called = false;
+
+  // Redeclare function that is read-only.
+  code = v8_str("function x() {return 43;};");
+  CHECK(v8::Script::Compile(ctx, code).ToLocalChecked()->Run(ctx).IsEmpty());
+  CHECK(try_catch.HasCaught());
+
+  CHECK_EQ(set_was_called, false);
+}
+
 bool get_was_called_in_order = false;
 bool define_was_called_in_order = false;
 

test/cctest/test-decls.cc

@@ -249,9 +249,7 @@ TEST(Unknown) {
   { DeclarationContext context;
     context.Check("function x() { }; x",
                   1,  // access
-                  0,
-                  0,
-                  EXPECT_RESULT);
+                  1, 1, EXPECT_RESULT);
   }
 }
 
@@ -285,9 +283,7 @@ TEST(Absent) {
   { AbsentPropertyContext context;
     context.Check("function x() { }; x",
                   1,  // access
-                  0,
-                  0,
-                  EXPECT_RESULT);
+                  1, 1, EXPECT_RESULT);
   }
 
   { AbsentPropertyContext context;
@@ -355,9 +351,7 @@ TEST(Appearing) {
   { AppearingPropertyContext context;
     context.Check("function x() { }; x",
                   1,  // access
-                  0,
-                  0,
-                  EXPECT_RESULT);
+                  1, 1, EXPECT_RESULT);
   }
 }
 
@@ -486,11 +480,7 @@ TEST(ExistsInHiddenPrototype) {
   }
 
   { ExistsInHiddenPrototypeContext context;
-    context.Check("function x() { }; x",
-                  0,
-                  0,
-                  0,
-                  EXPECT_RESULT);
+    context.Check("function x() { }; x", 0, 1, 1, EXPECT_RESULT);
   }
 }