[compiler,api] Pass non-strings to the modifying callback when unconditional codegen is on.

In the current state, when unconditional compilation is on, strings are evaluated and other objects are passed through unchanged. After this, non-strings are passed to the modifying callback which could unwrap and eval them. eval(string) is not affected.

If a non-modifying callback is set, it still takes the precedence, and the non-string object is returned as it would be currently (line 1933).

Change-Id: I835b976b3420635baba245c08f8563a9e5b3b246
Bug: chromium:1024786
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1917147
Commit-Queue: Stefano Sanfilippo <ssanfilippo@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67570}

src/codegen/compiler.cc

@@ -1898,7 +1898,6 @@ bool CodeGenerationFromStringsAllowed(Isolate* isolate, Handle<Context> context,
 // (via v8::Isolate::SetModifyCodeGenerationFromStringsCallback)
 bool ModifyCodeGenerationFromStrings(Isolate* isolate, Handle<Context> context,
                                      Handle<i::Object>* source) {
-  DCHECK(context->allow_code_gen_from_strings().IsFalse(isolate));
   DCHECK(isolate->modify_code_gen_callback());
   DCHECK(source);
 
@@ -1939,10 +1938,8 @@ std::pair<MaybeHandle<String>, bool> Compiler::ValidateDynamicCompilationSource(
   // allow_code_gen_from_strings can be many things, so we'll always check
   // against the 'false' literal, so that e.g. undefined and 'true' are treated
   // the same.
-  if (!context->allow_code_gen_from_strings().IsFalse(isolate)) {
-    if (!original_source->IsString()) {
-      return {MaybeHandle<String>(), true};
-    }
+  if (!context->allow_code_gen_from_strings().IsFalse(isolate) &&
+      original_source->IsString()) {
     return {Handle<String>::cast(original_source), false};
   }
 

test/cctest/test-api.cc

@@ -19351,6 +19351,52 @@ TEST(ModifyCodeGenFromStrings) {
   try_catch.Reset();
 }
 
+v8::ModifyCodeGenerationFromStringsResult RejectStringsIncrementNumbers(
+    Local<Context> context, Local<Value> source) {
+  if (source->IsString()) {
+    return {false, v8::MaybeLocal<String>()};
+  }
+
+  Local<v8::Number> number;
+  if (!source->ToNumber(context).ToLocal(&number)) {
+    return {true, v8::MaybeLocal<String>()};
+  }
+
+  Local<v8::String> incremented =
+      String::NewFromUtf8(context->GetIsolate(),
+                          std::to_string(number->Value() + 1).c_str(),
+                          v8::NewStringType::kNormal)
+          .ToLocalChecked();
+
+  return {true, incremented};
+}
+
+TEST(AllowFromStringsOrModifyCodegen) {
+  LocalContext context;
+  v8::HandleScope scope(context->GetIsolate());
+  context->GetIsolate()->SetModifyCodeGenerationFromStringsCallback(
+      &RejectStringsIncrementNumbers);
+
+  context->AllowCodeGenerationFromStrings(false);
+
+  TryCatch try_catch(CcTest::isolate());
+  Local<Value> result = CompileRun("eval('40+2')");
+  CHECK(result.IsEmpty());
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+
+  result = CompileRun("eval(42)");
+  CHECK_EQ(43, result->Int32Value(context.local()).FromJust());
+
+  context->AllowCodeGenerationFromStrings(true);
+
+  result = CompileRun("eval('40+2')");
+  CHECK_EQ(42, result->Int32Value(context.local()).FromJust());
+
+  result = CompileRun("eval(42)");
+  CHECK_EQ(43, result->Int32Value(context.local()).FromJust());
+}
+
 TEST(SetErrorMessageForCodeGenFromStrings) {
   LocalContext context;
   v8::HandleScope scope(context->GetIsolate());