From b0f5abbea372b0cc4d27f267de271766f5c8a11a Mon Sep 17 00:00:00 2001
From: neis <neis@chromium.org>
Date: Wed, 18 Jan 2017 22:59:20 -0800
Subject: [PATCH] [modules] Add an IsModule flag to ScriptOriginOptions.

Since the script origin is part of the key used in the compilation
cache, this ensures that the cache never confuses a module with a
non-module script.

BUG=v8:1569,v8:5685

Review-Url: https://codereview.chromium.org/2611643002
Cr-Commit-Position: refs/heads/master@{#42490}
---
 include/v8.h                                  | 34 ++++++++++++++-----
 src/api.cc                                    | 25 ++++++++------
 src/bootstrapper.cc                           |  5 ++-
 src/compiler.cc                               |  5 ++-
 src/compiler.h                                |  2 +-
 src/d8.cc                                     |  4 ++-
 src/objects.h                                 |  2 +-
 test/cctest/compiler/test-linkage.cc          |  2 +-
 .../bytecode-expectations-printer.cc          |  6 +++-
 test/cctest/test-compiler.cc                  |  2 +-
 test/cctest/test-modules.cc                   | 16 ++++++---
 test/cctest/test-serialize.cc                 |  5 ++-
 12 files changed, 70 insertions(+), 38 deletions(-)

diff --git a/include/v8.h b/include/v8.h
index 2f90933772..3ac47f8a89 100644
--- a/include/v8.h
+++ b/include/v8.h
@@ -962,20 +962,31 @@ class V8_EXPORT Data {
 class ScriptOriginOptions {
  public:
   V8_INLINE ScriptOriginOptions(bool is_shared_cross_origin = false,
-                                bool is_opaque = false, bool is_wasm = false)
+                                bool is_opaque = false, bool is_wasm = false,
+                                bool is_module = false)
       : flags_((is_shared_cross_origin ? kIsSharedCrossOrigin : 0) |
-               (is_wasm ? kIsWasm : 0) | (is_opaque ? kIsOpaque : 0)) {}
+               (is_wasm ? kIsWasm : 0) | (is_opaque ? kIsOpaque : 0) |
+               (is_module ? kIsModule : 0)) {}
   V8_INLINE ScriptOriginOptions(int flags)
-      : flags_(flags & (kIsSharedCrossOrigin | kIsOpaque | kIsWasm)) {}
+      : flags_(flags &
+               (kIsSharedCrossOrigin | kIsOpaque | kIsWasm | kIsModule)) {}
+
   bool IsSharedCrossOrigin() const {
     return (flags_ & kIsSharedCrossOrigin) != 0;
   }
   bool IsOpaque() const { return (flags_ & kIsOpaque) != 0; }
   bool IsWasm() const { return (flags_ & kIsWasm) != 0; }
+  bool IsModule() const { return (flags_ & kIsModule) != 0; }
+
   int Flags() const { return flags_; }
 
  private:
-  enum { kIsSharedCrossOrigin = 1, kIsOpaque = 1 << 1, kIsWasm = 1 << 2 };
+  enum {
+    kIsSharedCrossOrigin = 1,
+    kIsOpaque = 1 << 1,
+    kIsWasm = 1 << 2,
+    kIsModule = 1 << 3
+  };
   const int flags_;
 };
 
@@ -992,7 +1003,8 @@ class ScriptOrigin {
       Local<Integer> script_id = Local<Integer>(),
       Local<Value> source_map_url = Local<Value>(),
       Local<Boolean> resource_is_opaque = Local<Boolean>(),
-      Local<Boolean> is_wasm = Local<Boolean>());
+      Local<Boolean> is_wasm = Local<Boolean>(),
+      Local<Boolean> is_module = Local<Boolean>());
 
   V8_INLINE Local<Value> ResourceName() const;
   V8_INLINE Local<Integer> ResourceLineOffset() const;
@@ -1183,6 +1195,8 @@ class V8_EXPORT ScriptCompiler {
     // alive.
     V8_INLINE const CachedData* GetCachedData() const;
 
+    V8_INLINE const ScriptOriginOptions& GetResourceOptions() const;
+
     // Prevent copying.
     Source(const Source&) = delete;
     Source& operator=(const Source&) = delete;
@@ -1425,7 +1439,7 @@ class V8_EXPORT ScriptCompiler {
 
  private:
   static V8_WARN_UNUSED_RESULT MaybeLocal<UnboundScript> CompileUnboundInternal(
-      Isolate* isolate, Source* source, CompileOptions options, bool is_module);
+      Isolate* isolate, Source* source, CompileOptions options);
 };
 
 
@@ -8987,14 +9001,15 @@ ScriptOrigin::ScriptOrigin(Local<Value> resource_name,
                            Local<Integer> script_id,
                            Local<Value> source_map_url,
                            Local<Boolean> resource_is_opaque,
-                           Local<Boolean> is_wasm)
+                           Local<Boolean> is_wasm, Local<Boolean> is_module)
     : resource_name_(resource_name),
       resource_line_offset_(resource_line_offset),
       resource_column_offset_(resource_column_offset),
       options_(!resource_is_shared_cross_origin.IsEmpty() &&
                    resource_is_shared_cross_origin->IsTrue(),
                !resource_is_opaque.IsEmpty() && resource_is_opaque->IsTrue(),
-               !is_wasm.IsEmpty() && is_wasm->IsTrue()),
+               !is_wasm.IsEmpty() && is_wasm->IsTrue(),
+               !is_module.IsEmpty() && is_module->IsTrue()),
       script_id_(script_id),
       source_map_url_(source_map_url) {}
 
@@ -9043,6 +9058,9 @@ const ScriptCompiler::CachedData* ScriptCompiler::Source::GetCachedData()
   return cached_data;
 }
 
+const ScriptOriginOptions& ScriptCompiler::Source::GetResourceOptions() const {
+  return resource_options;
+}
 
 Local<Boolean> Boolean::New(Isolate* isolate, bool value) {
   return value ? True(isolate) : False(isolate);
diff --git a/src/api.cc b/src/api.cc
index a2cfe61a8d..d86739ff23 100644
--- a/src/api.cc
+++ b/src/api.cc
@@ -283,7 +283,8 @@ static ScriptOrigin GetScriptOriginForScript(i::Isolate* isolate,
       v8::Integer::New(v8_isolate, script->id()),
       Utils::ToLocal(source_map_url),
       v8::Boolean::New(v8_isolate, options.IsOpaque()),
-      v8::Boolean::New(v8_isolate, script->type() == i::Script::TYPE_WASM));
+      v8::Boolean::New(v8_isolate, script->type() == i::Script::TYPE_WASM),
+      v8::Boolean::New(v8_isolate, options.IsModule()));
   return origin;
 }
 
@@ -2103,8 +2104,7 @@ MaybeLocal<Value> Module::Evaluate(Local<Context> context) {
 }
 
 MaybeLocal<UnboundScript> ScriptCompiler::CompileUnboundInternal(
-    Isolate* v8_isolate, Source* source, CompileOptions options,
-    bool is_module) {
+    Isolate* v8_isolate, Source* source, CompileOptions options) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
   PREPARE_FOR_EXECUTION_WITH_ISOLATE(isolate, ScriptCompiler, CompileUnbound,
                                      UnboundScript);
@@ -2149,7 +2149,7 @@ MaybeLocal<UnboundScript> ScriptCompiler::CompileUnboundInternal(
     result = i::Compiler::GetSharedFunctionInfoForScript(
         str, name_obj, line_offset, column_offset, source->resource_options,
         source_map_url, isolate->native_context(), NULL, &script_data, options,
-        i::NOT_NATIVES_CODE, is_module);
+        i::NOT_NATIVES_CODE);
     has_pending_exception = result.is_null();
     if (has_pending_exception && script_data != NULL) {
       // This case won't happen during normal operation; we have compiled
@@ -2178,24 +2178,26 @@ MaybeLocal<UnboundScript> ScriptCompiler::CompileUnboundInternal(
 
 MaybeLocal<UnboundScript> ScriptCompiler::CompileUnboundScript(
     Isolate* v8_isolate, Source* source, CompileOptions options) {
-  return CompileUnboundInternal(v8_isolate, source, options, false);
+  DCHECK(!source->GetResourceOptions().IsModule());
+  return CompileUnboundInternal(v8_isolate, source, options);
 }
 
 
 Local<UnboundScript> ScriptCompiler::CompileUnbound(Isolate* v8_isolate,
                                                     Source* source,
                                                     CompileOptions options) {
-  RETURN_TO_LOCAL_UNCHECKED(
-      CompileUnboundInternal(v8_isolate, source, options, false),
-      UnboundScript);
+  DCHECK(!source->GetResourceOptions().IsModule());
+  RETURN_TO_LOCAL_UNCHECKED(CompileUnboundInternal(v8_isolate, source, options),
+                            UnboundScript);
 }
 
 
 MaybeLocal<Script> ScriptCompiler::Compile(Local<Context> context,
                                            Source* source,
                                            CompileOptions options) {
+  DCHECK(!source->GetResourceOptions().IsModule());
   auto isolate = context->GetIsolate();
-  auto maybe = CompileUnboundInternal(isolate, source, options, false);
+  auto maybe = CompileUnboundInternal(isolate, source, options);
   Local<UnboundScript> result;
   if (!maybe.ToLocal(&result)) return MaybeLocal<Script>();
   v8::Context::Scope scope(context);
@@ -2215,7 +2217,8 @@ MaybeLocal<Module> ScriptCompiler::CompileModule(Isolate* isolate,
                                                  Source* source) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
 
-  auto maybe = CompileUnboundInternal(isolate, source, kNoCompileOptions, true);
+  DCHECK(source->GetResourceOptions().IsModule());
+  auto maybe = CompileUnboundInternal(isolate, source, kNoCompileOptions);
   Local<UnboundScript> unbound;
   if (!maybe.ToLocal(&unbound)) return MaybeLocal<Module>();
 
@@ -9311,7 +9314,7 @@ MaybeLocal<UnboundScript> debug::CompileInspectorScript(Isolate* v8_isolate,
     result = i::Compiler::GetSharedFunctionInfoForScript(
         str, i::Handle<i::Object>(), 0, 0, origin_options,
         i::Handle<i::Object>(), isolate->native_context(), NULL, &script_data,
-        ScriptCompiler::kNoCompileOptions, i::INSPECTOR_CODE, false);
+        ScriptCompiler::kNoCompileOptions, i::INSPECTOR_CODE);
     has_pending_exception = result.is_null();
     RETURN_ON_FAILED_EXECUTION(UnboundScript);
   }
diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc
index a5c238a98c..f44f7389c5 100644
--- a/src/bootstrapper.cc
+++ b/src/bootstrapper.cc
@@ -2992,8 +2992,7 @@ bool Bootstrapper::CompileNative(Isolate* isolate, Vector<const char> name,
   Handle<SharedFunctionInfo> function_info =
       Compiler::GetSharedFunctionInfoForScript(
           source, script_name, 0, 0, ScriptOriginOptions(), Handle<Object>(),
-          context, NULL, NULL, ScriptCompiler::kNoCompileOptions, natives_flag,
-          false);
+          context, NULL, NULL, ScriptCompiler::kNoCompileOptions, natives_flag);
   if (function_info.is_null()) return false;
 
   DCHECK(context->IsNativeContext());
@@ -3056,7 +3055,7 @@ bool Genesis::CompileExtension(Isolate* isolate, v8::Extension* extension) {
     function_info = Compiler::GetSharedFunctionInfoForScript(
         source, script_name, 0, 0, ScriptOriginOptions(), Handle<Object>(),
         context, extension, NULL, ScriptCompiler::kNoCompileOptions,
-        EXTENSION_CODE, false);
+        EXTENSION_CODE);
     if (function_info.is_null()) return false;
     cache->Add(name, function_info);
   }
diff --git a/src/compiler.cc b/src/compiler.cc
index 6f123ea2b3..ac34e79e52 100644
--- a/src/compiler.cc
+++ b/src/compiler.cc
@@ -1490,8 +1490,7 @@ Handle<SharedFunctionInfo> Compiler::GetSharedFunctionInfoForScript(
     int column_offset, ScriptOriginOptions resource_options,
     Handle<Object> source_map_url, Handle<Context> context,
     v8::Extension* extension, ScriptData** cached_data,
-    ScriptCompiler::CompileOptions compile_options, NativesFlag natives,
-    bool is_module) {
+    ScriptCompiler::CompileOptions compile_options, NativesFlag natives) {
   Isolate* isolate = source->GetIsolate();
   if (compile_options == ScriptCompiler::kNoCompileOptions) {
     cached_data = NULL;
@@ -1578,7 +1577,7 @@ Handle<SharedFunctionInfo> Compiler::GetSharedFunctionInfoForScript(
     Zone zone(isolate->allocator(), ZONE_NAME);
     ParseInfo parse_info(&zone, script);
     CompilationInfo info(&parse_info, Handle<JSFunction>::null());
-    if (is_module) parse_info.set_module();
+    if (resource_options.IsModule()) parse_info.set_module();
     if (compile_options != ScriptCompiler::kNoCompileOptions) {
       parse_info.set_cached_data(cached_data);
     }
diff --git a/src/compiler.h b/src/compiler.h
index dfbd520f9d..26c772ec42 100644
--- a/src/compiler.h
+++ b/src/compiler.h
@@ -115,7 +115,7 @@ class Compiler : public AllStatic {
       Handle<Object> source_map_url, Handle<Context> context,
       v8::Extension* extension, ScriptData** cached_data,
       ScriptCompiler::CompileOptions compile_options,
-      NativesFlag is_natives_code, bool is_module);
+      NativesFlag is_natives_code);
 
   // Create a shared function info object for a Script that has already been
   // parsed while the script was being loaded from a streamed source.
diff --git a/src/d8.cc b/src/d8.cc
index 74a0b56e4f..f36e3a1739 100644
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -694,7 +694,9 @@ MaybeLocal<Module> Shell::FetchModuleTree(Local<Context> context,
   }
   ScriptOrigin origin(
       String::NewFromUtf8(isolate, file_name.c_str(), NewStringType::kNormal)
-          .ToLocalChecked());
+          .ToLocalChecked(),
+      Local<Integer>(), Local<Integer>(), Local<Boolean>(), Local<Integer>(),
+      Local<Value>(), Local<Boolean>(), Local<Boolean>(), True(isolate));
   ScriptCompiler::Source source(source_text, origin);
   Local<Module> module;
   if (!ScriptCompiler::CompileModule(isolate, &source).ToLocal(&module)) {
diff --git a/src/objects.h b/src/objects.h
index f13a9313b4..1f2a24813b 100644
--- a/src/objects.h
+++ b/src/objects.h
@@ -6967,7 +6967,7 @@ class Script: public Struct {
   static const int kCompilationTypeBit = 0;
   static const int kCompilationStateBit = 1;
   static const int kOriginOptionsShift = 2;
-  static const int kOriginOptionsSize = 3;
+  static const int kOriginOptionsSize = 4;
   static const int kOriginOptionsMask = ((1 << kOriginOptionsSize) - 1)
                                         << kOriginOptionsShift;
 
diff --git a/test/cctest/compiler/test-linkage.cc b/test/cctest/compiler/test-linkage.cc
index fef3415984..c704a7ff54 100644
--- a/test/cctest/compiler/test-linkage.cc
+++ b/test/cctest/compiler/test-linkage.cc
@@ -34,7 +34,7 @@ static Handle<JSFunction> Compile(const char* source) {
   Handle<SharedFunctionInfo> shared = Compiler::GetSharedFunctionInfoForScript(
       source_code, Handle<String>(), 0, 0, v8::ScriptOriginOptions(),
       Handle<Object>(), Handle<Context>(isolate->native_context()), NULL, NULL,
-      v8::ScriptCompiler::kNoCompileOptions, NOT_NATIVES_CODE, false);
+      v8::ScriptCompiler::kNoCompileOptions, NOT_NATIVES_CODE);
   return isolate->factory()->NewFunctionFromSharedFunctionInfo(
       shared, isolate->native_context());
 }
diff --git a/test/cctest/interpreter/bytecode-expectations-printer.cc b/test/cctest/interpreter/bytecode-expectations-printer.cc
index 8f34308ee0..dcc4883058 100644
--- a/test/cctest/interpreter/bytecode-expectations-printer.cc
+++ b/test/cctest/interpreter/bytecode-expectations-printer.cc
@@ -70,7 +70,11 @@ v8::Local<v8::Script> BytecodeExpectationsPrinter::CompileScript(
 
 v8::Local<v8::Module> BytecodeExpectationsPrinter::CompileModule(
     const char* program) const {
-  v8::ScriptCompiler::Source source(V8StringFromUTF8(program));
+  ScriptOrigin origin(
+      Local<v8::Value>(), Local<v8::Integer>(), Local<v8::Integer>(),
+      Local<v8::Boolean>(), Local<v8::Integer>(), Local<v8::Value>(),
+      Local<v8::Boolean>(), Local<v8::Boolean>(), True(isolate_));
+  v8::ScriptCompiler::Source source(V8StringFromUTF8(program), origin);
   return v8::ScriptCompiler::CompileModule(isolate_, &source).ToLocalChecked();
 }
 
diff --git a/test/cctest/test-compiler.cc b/test/cctest/test-compiler.cc
index 7191ceb78e..c0d8dc9b0d 100644
--- a/test/cctest/test-compiler.cc
+++ b/test/cctest/test-compiler.cc
@@ -65,7 +65,7 @@ static Handle<JSFunction> Compile(const char* source) {
   Handle<SharedFunctionInfo> shared = Compiler::GetSharedFunctionInfoForScript(
       source_code, Handle<String>(), 0, 0, v8::ScriptOriginOptions(),
       Handle<Object>(), Handle<Context>(isolate->native_context()), NULL, NULL,
-      v8::ScriptCompiler::kNoCompileOptions, NOT_NATIVES_CODE, false);
+      v8::ScriptCompiler::kNoCompileOptions, NOT_NATIVES_CODE);
   return isolate->factory()->NewFunctionFromSharedFunctionInfo(
       shared, isolate->native_context());
 }
diff --git a/test/cctest/test-modules.cc b/test/cctest/test-modules.cc
index e086a407f9..9a9bfef990 100644
--- a/test/cctest/test-modules.cc
+++ b/test/cctest/test-modules.cc
@@ -19,6 +19,14 @@ using v8::ScriptOrigin;
 using v8::String;
 using v8::Value;
 
+ScriptOrigin ModuleOrigin(Local<v8::Value> resource_name, Isolate* isolate) {
+  ScriptOrigin origin(resource_name, Local<v8::Integer>(), Local<v8::Integer>(),
+                      Local<v8::Boolean>(), Local<v8::Integer>(),
+                      Local<v8::Value>(), Local<v8::Boolean>(),
+                      Local<v8::Boolean>(), True(isolate));
+  return origin;
+}
+
 MaybeLocal<Module> AlwaysEmptyResolveCallback(Local<Context> context,
                                               Local<String> specifier,
                                               Local<Module> referrer) {
@@ -31,7 +39,7 @@ MaybeLocal<Module> FailOnSecondCallResolveCallback(Local<Context> context,
                                                    Local<Module> referrer) {
   if (g_count++ > 0) return MaybeLocal<Module>();
   Local<String> source_text = v8_str("");
-  ScriptOrigin origin(v8_str("module.js"));
+  ScriptOrigin origin = ModuleOrigin(v8_str("module.js"), CcTest::isolate());
   ScriptCompiler::Source source(source_text, origin);
   return ScriptCompiler::CompileModule(CcTest::isolate(), &source)
       .ToLocalChecked();
@@ -45,7 +53,7 @@ TEST(ModuleInstantiationFailures) {
   Local<String> source_text = v8_str(
       "import './foo.js';"
       "export {} from './bar.js';");
-  ScriptOrigin origin(v8_str("file.js"));
+  ScriptOrigin origin = ModuleOrigin(v8_str("file.js"), CcTest::isolate());
   ScriptCompiler::Source source(source_text, origin);
   Local<Module> module =
       ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
@@ -66,7 +74,7 @@ TEST(ModuleInstantiationFailures) {
 
 static MaybeLocal<Module> CompileSpecifierAsModuleResolveCallback(
     Local<Context> context, Local<String> specifier, Local<Module> referrer) {
-  ScriptOrigin origin(v8_str("module.js"));
+  ScriptOrigin origin = ModuleOrigin(v8_str("module.js"), CcTest::isolate());
   ScriptCompiler::Source source(specifier, origin);
   return ScriptCompiler::CompileModule(CcTest::isolate(), &source)
       .ToLocalChecked();
@@ -80,7 +88,7 @@ TEST(ModuleEvaluation) {
   Local<String> source_text = v8_str(
       "import 'Object.expando = 5';"
       "import 'Object.expando *= 2';");
-  ScriptOrigin origin(v8_str("file.js"));
+  ScriptOrigin origin = ModuleOrigin(v8_str("file.js"), CcTest::isolate());
   ScriptCompiler::Source source(source_text, origin);
   Local<Module> module =
       ScriptCompiler::CompileModule(isolate, &source).ToLocalChecked();
diff --git a/test/cctest/test-serialize.cc b/test/cctest/test-serialize.cc
index 9644d09624..3da487d098 100644
--- a/test/cctest/test-serialize.cc
+++ b/test/cctest/test-serialize.cc
@@ -931,7 +931,7 @@ static Handle<SharedFunctionInfo> CompileScript(
   return Compiler::GetSharedFunctionInfoForScript(
       source, name, 0, 0, v8::ScriptOriginOptions(), Handle<Object>(),
       Handle<Context>(isolate->native_context()), NULL, cached_data, options,
-      NOT_NATIVES_CODE, false);
+      NOT_NATIVES_CODE);
 }
 
 TEST(CodeSerializerOnePlusOne) {
@@ -1846,8 +1846,7 @@ TEST(Regress503552) {
   Handle<SharedFunctionInfo> shared = Compiler::GetSharedFunctionInfoForScript(
       source, Handle<String>(), 0, 0, v8::ScriptOriginOptions(),
       Handle<Object>(), Handle<Context>(isolate->native_context()), NULL,
-      &script_data, v8::ScriptCompiler::kProduceCodeCache, NOT_NATIVES_CODE,
-      false);
+      &script_data, v8::ScriptCompiler::kProduceCodeCache, NOT_NATIVES_CODE);
   delete script_data;
 
   heap::SimulateIncrementalMarking(isolate->heap());