[wasm] Refactor and fix wasm serialization test

This fixes a few issues:
1) It avoids using the {DeserializeOrCompile} API method, which is not
   used in chrome any more and will be deprecated soon.
2) It switches to the {DeserializeNativeModule} internal method, which
   really checks deserialization in isolation and does not fall back to
   compiling the wire bytes if the serialized bytes are incorrect.
3) It disables a test which tried to invalidate the number of functions,
   but the respective bytes were already zero, so nothing was
   invalidated. This still needs to be fixed in a follow-up CL.
4) It serializes the modules in a separate isolate, which then gets
   disposed to free references to the NativeModule and remove it from
   the modules cache. Otherwise we will just never deserialize, but use
   the cached module instead.

R=thibaudm@chromium.org

Bug: v8:6847, v8:10146
Change-Id: I37ef524a9c96c32fec2e7466488d67395fa5ccea
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2010786
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65888}

src/wasm/wasm-serialization.h

@@ -35,8 +35,8 @@ class V8_EXPORT_PRIVATE WasmSerializer {
 bool IsSupportedVersion(Vector<const byte> data);
 
 // Deserializes the given data to create a Wasm module object.
-MaybeHandle<WasmModuleObject> DeserializeNativeModule(
-    Isolate* isolate, Vector<const byte> data, Vector<const byte> wire_bytes,
+V8_EXPORT_PRIVATE MaybeHandle<WasmModuleObject> DeserializeNativeModule(
+    Isolate*, Vector<const byte> data, Vector<const byte> wire_bytes,
     Vector<const char> source_url);
 
 }  // namespace wasm

test/cctest/wasm/test-wasm-serialization.cc

@@ -15,6 +15,7 @@
 #include "src/wasm/wasm-module.h"
 #include "src/wasm/wasm-objects-inl.h"
 #include "src/wasm/wasm-opcodes.h"
+#include "src/wasm/wasm-serialization.h"
 
 #include "test/cctest/cctest.h"
 #include "test/common/wasm/flag-utils.h"
@@ -27,22 +28,6 @@ namespace internal {
 namespace wasm {
 namespace test_wasm_serialization {
 
-namespace {
-void Cleanup(Isolate* isolate = CcTest::InitIsolateOnce()) {
-  // By sending a low memory notifications, we will try hard to collect all
-  // garbage and will therefore also invoke all weak callbacks of actually
-  // unreachable persistent handles.
-  reinterpret_cast<v8::Isolate*>(isolate)->LowMemoryNotification();
-}
-
-#define EMIT_CODE_WITH_END(f, code)  \
-  do {                               \
-    f->EmitCode(code, sizeof(code)); \
-    f->Emit(kExprEnd);               \
-  } while (false)
-
-}  // namespace
-
 // Approximate gtest TEST_F style, in case we adopt gtest.
 class WasmSerializationTest {
  public:
@@ -56,8 +41,8 @@ class WasmSerializationTest {
     TestSignatures sigs;
 
     WasmFunctionBuilder* f = builder->AddFunction(sigs.i_i());
-    byte code[] = {WASM_GET_LOCAL(0), kExprI32Const, 1, kExprI32Add};
-    EMIT_CODE_WITH_END(f, code);
+    byte code[] = {WASM_GET_LOCAL(0), kExprI32Const, 1, kExprI32Add, kExprEnd};
+    f->EmitCode(code, sizeof(code));
     builder->AddExport(CStrVector(kFunctionName), f);
 
     builder->WriteTo(buffer);
@@ -83,20 +68,16 @@ class WasmSerializationTest {
     *slot = 0u;
   }
 
-  v8::MaybeLocal<v8::WasmModuleObject> Deserialize() {
-    ErrorThrower thrower(current_isolate(), "");
-    v8::MaybeLocal<v8::WasmModuleObject> deserialized =
-        v8::WasmModuleObject::DeserializeOrCompile(
-            current_isolate_v8(), serialized_bytes_, wire_bytes_);
-    return deserialized;
+  MaybeHandle<WasmModuleObject> Deserialize() {
+    return DeserializeNativeModule(CcTest::i_isolate(),
+                                   VectorOf(serialized_bytes_),
+                                   VectorOf(wire_bytes_), {});
   }
 
   void DeserializeAndRun() {
-    ErrorThrower thrower(current_isolate(), "");
-    v8::Local<v8::WasmModuleObject> deserialized_module;
-    CHECK(Deserialize().ToLocal(&deserialized_module));
-    Handle<WasmModuleObject> module_object = Handle<WasmModuleObject>::cast(
-        v8::Utils::OpenHandle(*deserialized_module));
+    ErrorThrower thrower(CcTest::i_isolate(), "");
+    Handle<WasmModuleObject> module_object;
+    CHECK(Deserialize().ToHandle(&module_object));
     {
       DisallowHeapAllocation assume_no_gc;
       Vector<const byte> deserialized_module_wire_bytes =
@@ -107,44 +88,49 @@ class WasmSerializationTest {
                0);
     }
     Handle<WasmInstanceObject> instance =
-        current_isolate()
+        CcTest::i_isolate()
             ->wasm_engine()
-            ->SyncInstantiate(current_isolate(), &thrower, module_object,
+            ->SyncInstantiate(CcTest::i_isolate(), &thrower, module_object,
                               Handle<JSReceiver>::null(),
                               MaybeHandle<JSArrayBuffer>())
             .ToHandleChecked();
     Handle<Object> params[1] = {
-        Handle<Object>(Smi::FromInt(41), current_isolate())};
+        Handle<Object>(Smi::FromInt(41), CcTest::i_isolate())};
     int32_t result = testing::CallWasmFunctionForTesting(
-        current_isolate(), instance, &thrower, kFunctionName, 1, params);
+        CcTest::i_isolate(), instance, &thrower, kFunctionName, 1, params);
     CHECK_EQ(42, result);
   }
 
-  Isolate* current_isolate() {
-    return reinterpret_cast<Isolate*>(current_isolate_v8_);
-  }
-
-  ~WasmSerializationTest() {
-    // Don't call from here if we move to gtest
-    TearDown();
+  void CollectGarbage() {
+    // Try hard to collect all garbage and will therefore also invoke all weak
+    // callbacks of actually unreachable persistent handles.
+    CcTest::i_isolate()->heap()->CollectAllAvailableGarbage(
+        GarbageCollectionReason::kTesting);
   }
 
-  v8::Isolate* current_isolate_v8() { return current_isolate_v8_; }
-
  private:
   static const char* kFunctionName;
 
   Zone* zone() { return &zone_; }
 
   void SetUp() {
+    CcTest::InitIsolateOnce();
     ZoneBuffer buffer(&zone_);
     WasmSerializationTest::BuildWireBytes(zone(), &buffer);
 
-    Isolate* serialization_isolate = CcTest::InitIsolateOnce();
+    v8::Isolate::CreateParams create_params;
+    create_params.array_buffer_allocator =
+        CcTest::i_isolate()->array_buffer_allocator();
+
+    v8::Isolate* serialization_v8_isolate = v8::Isolate::New(create_params);
+    Isolate* serialization_isolate =
+        reinterpret_cast<Isolate*>(serialization_v8_isolate);
     ErrorThrower thrower(serialization_isolate, "");
     {
       HandleScope scope(serialization_isolate);
-      testing::SetupIsolateForWasmModule(serialization_isolate);
+      v8::Local<v8::Context> serialization_context =
+          v8::Context::New(serialization_v8_isolate);
+      serialization_context->Enter();
 
       auto enabled_features = WasmFeatures::FromIsolate(serialization_isolate);
       MaybeHandle<WasmModuleObject> maybe_module_object =
@@ -170,24 +156,17 @@ class WasmSerializationTest {
       // keep alive data_ until the end
       data_ = compiled_module.Serialize();
     }
+    // Dispose of serialization isolate to destroy the reference to the
+    // NativeModule, which removes it from the module cache in the wasm engine
+    // and forces de-serialization in the new isolate.
+    serialization_v8_isolate->Dispose();
 
     serialized_bytes_ = {data_.buffer.get(), data_.size};
 
-    v8::Isolate::CreateParams create_params;
-    create_params.array_buffer_allocator =
-        serialization_isolate->array_buffer_allocator();
-
-    current_isolate_v8_ = v8::Isolate::New(create_params);
-    v8::HandleScope new_scope(current_isolate_v8());
+    v8::HandleScope new_scope(CcTest::isolate());
     v8::Local<v8::Context> deserialization_context =
-        v8::Context::New(current_isolate_v8());
+        v8::Context::New(CcTest::isolate());
     deserialization_context->Enter();
-    testing::SetupIsolateForWasmModule(current_isolate());
-  }
-
-  void TearDown() {
-    current_isolate_v8()->Dispose();
-    current_isolate_v8_ = nullptr;
   }
 
   v8::internal::AccountingAllocator allocator_;
@@ -195,7 +174,6 @@ class WasmSerializationTest {
   v8::OwnedBuffer data_;
   v8::MemorySpan<const uint8_t> wire_bytes_ = {nullptr, 0};
   v8::MemorySpan<const uint8_t> serialized_bytes_ = {nullptr, 0};
-  v8::Isolate* current_isolate_v8_;
 };
 
 const char* WasmSerializationTest::kFunctionName = "increment";
@@ -203,56 +181,52 @@ const char* WasmSerializationTest::kFunctionName = "increment";
 TEST(DeserializeValidModule) {
   WasmSerializationTest test;
   {
-    HandleScope scope(test.current_isolate());
+    HandleScope scope(CcTest::i_isolate());
     test.DeserializeAndRun();
   }
-  Cleanup(test.current_isolate());
-  Cleanup();
+  test.CollectGarbage();
 }
 
 TEST(DeserializeMismatchingVersion) {
   WasmSerializationTest test;
   {
-    HandleScope scope(test.current_isolate());
+    HandleScope scope(CcTest::i_isolate());
     test.InvalidateVersion();
-    test.DeserializeAndRun();
+    CHECK(test.Deserialize().is_null());
   }
-  Cleanup(test.current_isolate());
-  Cleanup();
+  test.CollectGarbage();
 }
 
 TEST(DeserializeNoSerializedData) {
   WasmSerializationTest test;
   {
-    HandleScope scope(test.current_isolate());
+    HandleScope scope(CcTest::i_isolate());
     test.ClearSerializedData();
-    test.DeserializeAndRun();
+    CHECK(test.Deserialize().is_null());
   }
-  Cleanup(test.current_isolate());
-  Cleanup();
+  test.CollectGarbage();
 }
 
 TEST(DeserializeInvalidLength) {
   WasmSerializationTest test;
   {
-    HandleScope scope(test.current_isolate());
+    HandleScope scope(CcTest::i_isolate());
     test.InvalidateLength();
-    test.DeserializeAndRun();
+    // TODO(clemensb): Check why this still deserialized correctly.
+    // CHECK(test.Deserialize().is_null());
   }
-  Cleanup(test.current_isolate());
-  Cleanup();
+  test.CollectGarbage();
 }
 
 TEST(DeserializeWireBytesAndSerializedDataInvalid) {
   WasmSerializationTest test;
   {
-    HandleScope scope(test.current_isolate());
+    HandleScope scope(CcTest::i_isolate());
     test.InvalidateVersion();
     test.InvalidateWireBytes();
-    test.Deserialize();
+    CHECK(test.Deserialize().is_null());
   }
-  Cleanup(test.current_isolate());
-  Cleanup();
+  test.CollectGarbage();
 }
 
 bool False(v8::Local<v8::Context> context, v8::Local<v8::String> source) {
@@ -262,13 +236,11 @@ bool False(v8::Local<v8::Context> context, v8::Local<v8::String> source) {
 TEST(BlockWasmCodeGenAtDeserialization) {
   WasmSerializationTest test;
   {
-    HandleScope scope(test.current_isolate());
-    test.current_isolate_v8()->SetAllowWasmCodeGenerationCallback(False);
-    v8::MaybeLocal<v8::WasmModuleObject> nothing = test.Deserialize();
-    CHECK(nothing.IsEmpty());
+    HandleScope scope(CcTest::i_isolate());
+    CcTest::isolate()->SetAllowWasmCodeGenerationCallback(False);
+    CHECK(test.Deserialize().is_null());
   }
-  Cleanup(test.current_isolate());
-  Cleanup();
+  test.CollectGarbage();
 }
 
 UNINITIALIZED_TEST(CompiledWasmModulesTransfer) {
@@ -326,8 +298,6 @@ UNINITIALIZED_TEST(CompiledWasmModulesTransfer) {
   from_isolate->Dispose();
 }
 
-#undef EMIT_CODE_WITH_END
-
 }  // namespace test_wasm_serialization
 }  // namespace wasm
 }  // namespace internal