[wasm] Delay error messages for lazy compilation

With streaming compilation we delay the generation of errors until after
all bytes are received, so that potentially better error messages get
generated. With this CL we also delay the generation of errors in the
combination of lazy compilation and streaming compilation.

In particular, this CL does the following:
* It avoids the creation of a `DecodeFail` task in
`FinishAsyncCompileJobWithError`, which would create an error immediately before a potential name section arrived.
* It calls `CompilationStateImpl::SetError()` so that an error is
created once the stream finishes.
* It removes the return value of `ProcessFunctionBody` so that wire
bytes continue to be received even after a validation error.
* It adds an early exit to `ProcessFunctionBody` if
`CompilationStateImpl::failed()` is true, so that we don't continue
validation after the first detected error.

R=clemensb@chromium.org

Bug: v8:12852
Change-Id: Ie8c6be243a257ef62cbb29fea6b8e0c205060680
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3802691Reviewed-by: Clemens Backes <clemensb@chromium.org>
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82181}

src/wasm/module-compiler.cc

@@ -2095,7 +2095,7 @@ class AsyncStreamingProcessor final : public StreamingProcessor {
                                 int code_section_start,
                                 int code_section_length) override;
 
-  bool ProcessFunctionBody(base::Vector<const uint8_t> bytes,
+  void ProcessFunctionBody(base::Vector<const uint8_t> bytes,
                            uint32_t offset) override;
 
   void OnFinishedChunk() override;
@@ -2110,8 +2110,10 @@ class AsyncStreamingProcessor final : public StreamingProcessor {
                    base::Vector<const uint8_t> module_bytes) override;
 
  private:
+  enum ErrorLocation { kErrorInFunction, kErrorInSection };
   // Finishes the AsyncCompileJob with an error.
-  void FinishAsyncCompileJobWithError(const WasmError&);
+  void FinishAsyncCompileJobWithError(
+      const WasmError&, ErrorLocation error_location = kErrorInSection);
 
   void CommitCompilationUnits();
 
@@ -2725,7 +2727,7 @@ AsyncStreamingProcessor::~AsyncStreamingProcessor() {
 }
 
 void AsyncStreamingProcessor::FinishAsyncCompileJobWithError(
-    const WasmError& error) {
+    const WasmError& error, ErrorLocation error_location) {
   DCHECK(error.has_error());
   // Make sure all background tasks stopped executing before we change the state
   // of the AsyncCompileJob to DecodeFail.
@@ -2744,12 +2746,17 @@ void AsyncStreamingProcessor::FinishAsyncCompileJobWithError(
   // Check if there is already a CompiledModule, in which case we have to clean
   // up the CompilationStateImpl as well.
   if (job_->native_module_) {
-    Impl(job_->native_module_->compilation_state())
-        ->CancelCompilation(CompilationStateImpl::kCancelUnconditionally);
-
-    job_->DoSync<AsyncCompileJob::DecodeFail,
-                 AsyncCompileJob::kUseExistingForegroundTask>(error);
+    CompilationStateImpl* impl =
+        Impl(job_->native_module_->compilation_state());
 
+    if (error_location == kErrorInFunction) {
+      impl->SetError();
+    }
+    impl->CancelCompilation(CompilationStateImpl::kCancelUnconditionally);
+    if (error_location == kErrorInSection) {
+      job_->DoSync<AsyncCompileJob::DecodeFail,
+                   AsyncCompileJob::kUseExistingForegroundTask>(error);
+    }
     // Clear the {compilation_unit_builder_} if it exists. This is needed
     // because there is a check in the destructor of the
     // {CompilationUnitBuilder} that it is empty.
@@ -2867,9 +2874,17 @@ bool AsyncStreamingProcessor::ProcessCodeSectionHeader(
 }
 
 // Process a function body.
-bool AsyncStreamingProcessor::ProcessFunctionBody(
+void AsyncStreamingProcessor::ProcessFunctionBody(
     base::Vector<const uint8_t> bytes, uint32_t offset) {
   TRACE_STREAMING("Process function body %d ...\n", num_functions_);
+  // We first have to check that the compilation state exists, because with a
+  // prefix_cache_hit_ it would not exist.
+  if (job_->native_module_ &&
+      job_->native_module_->compilation_state()->failed()) {
+    // There has already been an error, there is no need to do any more
+    // validation or compiling.
+    return;
+  }
 
   decoder_.DecodeFunctionBody(
       num_functions_, static_cast<uint32_t>(bytes.length()), offset, false);
@@ -2893,23 +2908,21 @@ bool AsyncStreamingProcessor::ProcessFunctionBody(
                                                  allocator_, enabled_features);
 
     if (result.failed()) {
-      FinishAsyncCompileJobWithError(result.error());
-      return false;
+      FinishAsyncCompileJobWithError(result.error(), kErrorInFunction);
+      return;
     }
   }
 
   // Don't compile yet if we might have a cache hit.
   if (prefix_cache_hit_) {
-    num_functions_++;
-    return true;
+    ++num_functions_;
+    return;
   }
 
   auto* compilation_state = Impl(job_->native_module_->compilation_state());
   compilation_state->AddCompilationUnit(compilation_unit_builder_.get(),
                                         func_index);
   ++num_functions_;
-
-  return true;
 }
 
 void AsyncStreamingProcessor::CommitCompilationUnits() {

src/wasm/streaming-decoder.cc

@@ -195,7 +195,7 @@ class V8_EXPORT_PRIVATE AsyncStreamingDecoder : public StreamingDecoder {
   void ProcessFunctionBody(base::Vector<const uint8_t> bytes,
                            uint32_t module_offset) {
     if (!ok()) return;
-    if (!processor_->ProcessFunctionBody(bytes, module_offset)) Fail();
+    processor_->ProcessFunctionBody(bytes, module_offset);
   }
 
   void Fail() {

src/wasm/streaming-decoder.h

@@ -46,9 +46,8 @@ class V8_EXPORT_PRIVATE StreamingProcessor {
                                         int code_section_start,
                                         int code_section_length) = 0;
 
-  // Process a function body. Returns true if the processing finished
-  // successfully and the decoding should continue.
-  virtual bool ProcessFunctionBody(base::Vector<const uint8_t> bytes,
+  // Process a function body.
+  virtual void ProcessFunctionBody(base::Vector<const uint8_t> bytes,
                                    uint32_t offset) = 0;
 
   // Report the end of a chunk.

test/mjsunit/regress/wasm/regress-956771b.js

@@ -12,8 +12,11 @@ d8.file.execute('test/mjsunit/wasm/wasm-module-builder.js');
   builder.addFunction("some", kSig_i_ii);
   let bytes = builder.toBuffer();
   assertPromiseResult(WebAssembly.compileStreaming(Promise.resolve(bytes))
-    .then(assertUnreachable,
-          error => assertEquals("WebAssembly.compileStreaming(): function " +
-                                "body must end with \"end\" opcode @+26",
-                                error.message)));
+                          .then(
+                              assertUnreachable,
+                              error => assertEquals(
+                                  'WebAssembly.compileStreaming(): Compiling ' +
+                                      'function #0:"some" failed: function ' +
+                                      'body must end with "end" opcode @+26',
+                                  error.message)));
 })();

test/mjsunit/wasm/compilation-hints-streaming-compilation.js

@@ -71,13 +71,17 @@ d8.file.execute('test/mjsunit/wasm/wasm-module-builder.js');
                              kCompilationHintTierDefault)
          .exportFunc();
   let bytes = builder.toBuffer();
-  assertPromiseResult(WebAssembly.instantiateStreaming(Promise.resolve(bytes),
-                                                       {mod: {pow: Math.pow}})
-    .then(assertUnreachable,
-          error => assertEquals("WebAssembly.instantiateStreaming(): call[0] " +
-                                "expected type f32, found local.get of type " +
-                                "i32 @+92",
-                                error.message)));
+  assertPromiseResult(
+      WebAssembly
+          .instantiateStreaming(Promise.resolve(bytes), {mod: {pow: Math.pow}})
+          .then(
+              assertUnreachable,
+              error => assertEquals(
+                  'WebAssembly.instantiateStreaming(): Compiling ' +
+                      'function #1:"upow" failed: call[0] ' +
+                      'expected type f32, found local.get of type ' +
+                      'i32 @+83',
+                  error.message)));
 })();
 
 (function testInstantiateStreamingEmptyModule() {

test/unittests/wasm/streaming-decoder-unittest.cc

@@ -72,10 +72,9 @@ class MockStreamingProcessor : public StreamingProcessor {
   }
 
   // Process a function body.
-  bool ProcessFunctionBody(base::Vector<const uint8_t> bytes,
+  void ProcessFunctionBody(base::Vector<const uint8_t> bytes,
                            uint32_t offset) override {
     ++result_->num_functions;
-    return true;
   }
 
   void OnFinishedChunk() override {}