Do not create Script if an error occurred

As a preparation for
https://chromium-review.googlesource.com/1179488,
this CL stops creating ClassicScript when ErrorOccurred() is true.

Instead of setting |error_occurred| bool, this CL makes
PendingScript::GetSource() return nullptr if an error occurred.
This is consistent with the spec:

- GetSource() corresponds to "the script's script".
- GetSource() == nullptr (previously |error_occurred| == true)
  corresponds to "the script's script is null".

Bug: 875153, 686281
Change-Id: I08d891ce2207894b4a8f8a2d1171f9912c79b8ce
Reviewed-on: https://chromium-review.googlesource.com/1179557Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584371}

third_party/blink/renderer/bindings/core/v8/script_streamer_test.cc

@@ -75,6 +75,12 @@ class ScriptStreamingTest : public testing::Test {
     return pending_script_.Get();
   }
 
+  ScriptSourceCode GetScriptSourceCode() const {
+    ClassicScript* classic_script = GetPendingScript()->GetSource(NullURL());
+    DCHECK(classic_script);
+    return classic_script->GetScriptSourceCode();
+  }
+
   Settings* GetSettings() const {
     return &dummy_page_holder_->GetPage().GetSettings();
   }
@@ -165,11 +171,7 @@ TEST_F(ScriptStreamingTest, CompilingStreamedScript) {
   // has completed its tasks.
   ProcessTasksUntilStreamingComplete();
   EXPECT_TRUE(client->Finished());
-  bool error_occurred = false;
-  ScriptSourceCode source_code = GetPendingScript()
-                                     ->GetSource(NullURL(), error_occurred)
-                                     ->GetScriptSourceCode();
-  EXPECT_FALSE(error_occurred);
+  ScriptSourceCode source_code = GetScriptSourceCode();
   EXPECT_TRUE(source_code.Streamer());
   v8::TryCatch try_catch(scope.GetIsolate());
   v8::Local<v8::Script> script;
@@ -212,11 +214,7 @@ TEST_F(ScriptStreamingTest, CompilingStreamedScriptWithParseError) {
   Finish();
   EXPECT_TRUE(client->Finished());
 
-  bool error_occurred = false;
-  ScriptSourceCode source_code = GetPendingScript()
-                                     ->GetSource(NullURL(), error_occurred)
-                                     ->GetScriptSourceCode();
-  EXPECT_FALSE(error_occurred);
+  ScriptSourceCode source_code = GetScriptSourceCode();
   EXPECT_TRUE(source_code.Streamer());
   v8::TryCatch try_catch(scope.GetIsolate());
   v8::Local<v8::Script> script;
@@ -288,11 +286,7 @@ TEST_F(ScriptStreamingTest, SuppressingStreaming) {
   ProcessTasksUntilStreamingComplete();
   EXPECT_TRUE(client->Finished());
 
-  bool error_occurred = false;
-  ScriptSourceCode source_code = GetPendingScript()
-                                     ->GetSource(NullURL(), error_occurred)
-                                     ->GetScriptSourceCode();
-  EXPECT_FALSE(error_occurred);
+  ScriptSourceCode source_code = GetScriptSourceCode();
   // ScriptSourceCode doesn't refer to the streamer, since we have suppressed
   // the streaming and resumed the non-streaming code path for script
   // compilation.
@@ -318,11 +312,7 @@ TEST_F(ScriptStreamingTest, EmptyScripts) {
   // through a background thread.
   EXPECT_TRUE(client->Finished());
 
-  bool error_occurred = false;
-  ScriptSourceCode source_code = GetPendingScript()
-                                     ->GetSource(NullURL(), error_occurred)
-                                     ->GetScriptSourceCode();
-  EXPECT_FALSE(error_occurred);
+  ScriptSourceCode source_code = GetScriptSourceCode();
   EXPECT_FALSE(source_code.Streamer());
 }
 
@@ -347,11 +337,7 @@ TEST_F(ScriptStreamingTest, SmallScripts) {
   // through a background thread.
   EXPECT_TRUE(client->Finished());
 
-  bool error_occurred = false;
-  ScriptSourceCode source_code = GetPendingScript()
-                                     ->GetSource(NullURL(), error_occurred)
-                                     ->GetScriptSourceCode();
-  EXPECT_FALSE(error_occurred);
+  ScriptSourceCode source_code = GetScriptSourceCode();
   EXPECT_FALSE(source_code.Streamer());
 }
 
@@ -379,11 +365,7 @@ TEST_F(ScriptStreamingTest, ScriptsWithSmallFirstChunk) {
 
   ProcessTasksUntilStreamingComplete();
   EXPECT_TRUE(client->Finished());
-  bool error_occurred = false;
-  ScriptSourceCode source_code = GetPendingScript()
-                                     ->GetSource(NullURL(), error_occurred)
-                                     ->GetScriptSourceCode();
-  EXPECT_FALSE(error_occurred);
+  ScriptSourceCode source_code = GetScriptSourceCode();
   EXPECT_TRUE(source_code.Streamer());
   v8::TryCatch try_catch(scope.GetIsolate());
   v8::Local<v8::Script> script;
@@ -423,11 +405,7 @@ TEST_F(ScriptStreamingTest, EncodingChanges) {
 
   ProcessTasksUntilStreamingComplete();
   EXPECT_TRUE(client->Finished());
-  bool error_occurred = false;
-  ScriptSourceCode source_code = GetPendingScript()
-                                     ->GetSource(NullURL(), error_occurred)
-                                     ->GetScriptSourceCode();
-  EXPECT_FALSE(error_occurred);
+  ScriptSourceCode source_code = GetScriptSourceCode();
   EXPECT_TRUE(source_code.Streamer());
   v8::TryCatch try_catch(scope.GetIsolate());
   v8::Local<v8::Script> script;
@@ -468,11 +446,7 @@ TEST_F(ScriptStreamingTest, EncodingFromBOM) {
   Finish();
   ProcessTasksUntilStreamingComplete();
   EXPECT_TRUE(client->Finished());
-  bool error_occurred = false;
-  ScriptSourceCode source_code = GetPendingScript()
-                                     ->GetSource(NullURL(), error_occurred)
-                                     ->GetScriptSourceCode();
-  EXPECT_FALSE(error_occurred);
+  ScriptSourceCode source_code = GetScriptSourceCode();
   EXPECT_TRUE(source_code.Streamer());
   v8::TryCatch try_catch(scope.GetIsolate());
   v8::Local<v8::Script> script;

third_party/blink/renderer/core/script/classic_pending_script.cc

@@ -331,12 +331,13 @@ static SingleCachedMetadataHandler* GetInlineCacheHandler(const String& source,
   return document_cache_handler->HandlerForSource(source);
 }
 
-ClassicScript* ClassicPendingScript::GetSource(const KURL& document_url,
-                                               bool& error_occurred) const {
+ClassicScript* ClassicPendingScript::GetSource(const KURL& document_url) const {
   CheckState();
   DCHECK(IsReady());
 
-  error_occurred = ErrorOccurred();
+  if (ErrorOccurred())
+    return nullptr;
+
   if (!is_external_) {
     SingleCachedMetadataHandler* cache_handler = nullptr;
     // We only create an inline cache handler for html-embedded scripts, not
@@ -370,7 +371,7 @@ ClassicScript* ClassicPendingScript::GetSource(const KURL& document_url,
   if (!AllowedByNosniff::MimeTypeAsScript(
           GetElement()->GetDocument().ContextDocument(),
           resource->GetResponse())) {
-    error_occurred = true;
+    return nullptr;
   }
 
   // Check if we can use the script streamer.

third_party/blink/renderer/core/script/classic_pending_script.h

@@ -59,8 +59,7 @@ class CORE_EXPORT ClassicPendingScript final : public PendingScript,
     return blink::ScriptType::kClassic;
   }
 
-  ClassicScript* GetSource(const KURL& document_url,
-                           bool& error_occurred) const override;
+  ClassicScript* GetSource(const KURL& document_url) const override;
   bool IsReady() const override;
   bool IsExternal() const override { return is_external_; }
   bool ErrorOccurred() const override;

third_party/blink/renderer/core/script/module_pending_script.cc

@@ -65,10 +65,12 @@ void ModulePendingScript::NotifyModuleTreeLoadFinished() {
   PendingScriptFinished();
 }
 
-Script* ModulePendingScript::GetSource(const KURL& document_url,
-                                       bool& error_occurred) const {
+Script* ModulePendingScript::GetSource(const KURL& document_url) const {
   CHECK(IsReady());
-  error_occurred = ErrorOccurred();
+  if (ErrorOccurred())
+    return nullptr;
+
+  DCHECK(GetModuleScript());
   return GetModuleScript();
 }
 

third_party/blink/renderer/core/script/module_pending_script.h

@@ -72,8 +72,7 @@ class CORE_EXPORT ModulePendingScript : public PendingScript {
 
   // PendingScript
   ScriptType GetScriptType() const override { return ScriptType::kModule; }
-  Script* GetSource(const KURL& document_url,
-                    bool& error_occurred) const override;
+  Script* GetSource(const KURL& document_url) const override;
   bool IsReady() const override { return ready_; }
   bool IsExternal() const override { return is_external_; }
   bool ErrorOccurred() const override;

third_party/blink/renderer/core/script/pending_script.cc

@@ -146,10 +146,9 @@ void PendingScript::ExecuteScriptBlock(const KURL& document_url) {
     return;
   }
 
-  bool error_occurred = false;
-  Script* script = GetSource(document_url, error_occurred);
+  Script* script = GetSource(document_url);
 
-  if (!error_occurred && !IsExternal()) {
+  if (script && !IsExternal()) {
     bool should_bypass_main_world_csp =
         frame->GetScriptController().ShouldBypassMainWorldCSP();
 
@@ -160,7 +159,7 @@ void PendingScript::ExecuteScriptBlock(const KURL& document_url) {
             ContentSecurityPolicy::InlineType::kBlock)) {
       // Consider as if "the script's script is null" retrospectively,
       // if the CSP check fails, which is considered as load failure.
-      error_occurred = true;
+      script = nullptr;
     }
   }
 
@@ -175,15 +174,13 @@ void PendingScript::ExecuteScriptBlock(const KURL& document_url) {
 
   // ExecuteScriptBlockInternal() is split just in order to prevent accidential
   // access to |this| after Dispose().
-  ExecuteScriptBlockInternal(script, error_occurred, element, was_canceled,
-                             is_external, created_during_document_write,
-                             parser_blocking_load_start_time,
-                             is_controlled_by_script_runner);
+  ExecuteScriptBlockInternal(
+      script, element, was_canceled, is_external, created_during_document_write,
+      parser_blocking_load_start_time, is_controlled_by_script_runner);
 }
 
 void PendingScript::ExecuteScriptBlockInternal(
     Script* script,
-    bool error_occurred,
     ScriptElementBase* element,
     bool was_canceled,
     bool is_external,
@@ -195,7 +192,7 @@ void PendingScript::ExecuteScriptBlockInternal(
 
   // <spec step="2">If the script's script is null, fire an event named error at
   // the element, and return.</spec>
-  if (error_occurred) {
+  if (!script) {
     element->DispatchErrorEvent();
     return;
   }

third_party/blink/renderer/core/script/pending_script.h

@@ -87,8 +87,8 @@ class CORE_EXPORT PendingScript
   virtual void Trace(blink::Visitor*);
   const char* NameInHeapSnapshot() const override { return "PendingScript"; }
 
-  virtual Script* GetSource(const KURL& document_url,
-                            bool& error_occurred) const = 0;
+  // Returns nullptr when "script's script is null", i.e. an error occurred.
+  virtual Script* GetSource(const KURL& document_url) const = 0;
 
   // https://html.spec.whatwg.org/multipage/scripting.html#the-script-is-ready
   virtual bool IsReady() const = 0;
@@ -150,7 +150,6 @@ class CORE_EXPORT PendingScript
  private:
   static void ExecuteScriptBlockInternal(
       Script*,
-      bool error_occurred,
       ScriptElementBase*,
       bool was_canceled,
       bool is_external,

third_party/blink/renderer/core/script/script_runner_test.cc

@@ -37,7 +37,7 @@ class MockPendingScript : public PendingScript {
 
   MOCK_CONST_METHOD0(GetScriptType, ScriptType());
   MOCK_CONST_METHOD1(CheckMIMETypeBeforeRunScript, bool(Document*));
-  MOCK_CONST_METHOD2(GetSource, Script*(const KURL&, bool&));
+  MOCK_CONST_METHOD1(GetSource, Script*(const KURL&));
   MOCK_CONST_METHOD0(IsExternal, bool());
   MOCK_CONST_METHOD0(ErrorOccurred, bool());
   MOCK_CONST_METHOD0(WasCanceled, bool());