[vm/ffi] Fix snapshot serialization of FFI objects.

Change-Id: I78557211da79f5282339fbcf30b3f81ab56cb361
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/106345
Commit-Queue: Samir Jindel <sjindel@google.com>
Auto-Submit: Samir Jindel <sjindel@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
diff --git a/runtime/vm/clustered_snapshot.cc b/runtime/vm/clustered_snapshot.cc
index 39249519..5796ec3 100644
--- a/runtime/vm/clustered_snapshot.cc
+++ b/runtime/vm/clustered_snapshot.cc
@@ -6,6 +6,7 @@
 
 #include "platform/assert.h"
 #include "vm/bootstrap.h"
+#include "vm/class_id.h"
 #include "vm/compiler/backend/code_statistics.h"
 #include "vm/compiler/frontend/bytecode_reader.h"
 #include "vm/compiler/relocation.h"
@@ -768,6 +769,74 @@
 };
 
 #if !defined(DART_PRECOMPILED_RUNTIME)
+class FfiTrampolineDataSerializationCluster : public SerializationCluster {
+ public:
+  FfiTrampolineDataSerializationCluster()
+      : SerializationCluster("FfiTrampolineData") {}
+  ~FfiTrampolineDataSerializationCluster() {}
+
+  void Trace(Serializer* s, RawObject* object) {
+    RawFfiTrampolineData* data = FfiTrampolineData::RawCast(object);
+    objects_.Add(data);
+    PushFromTo(data);
+  }
+
+  void WriteAlloc(Serializer* s) {
+    s->WriteCid(kFfiTrampolineDataCid);
+    const intptr_t count = objects_.length();
+    s->WriteUnsigned(count);
+    for (intptr_t i = 0; i < count; i++) {
+      s->AssignRef(objects_[i]);
+    }
+  }
+
+  void WriteFill(Serializer* s) {
+    intptr_t count = objects_.length();
+    for (intptr_t i = 0; i < count; i++) {
+      RawFfiTrampolineData* const data = objects_[i];
+      AutoTraceObject(data);
+      WriteFromTo(data);
+
+      // TODO(37295): FFI callbacks shouldn't be written to a snapshot. They
+      // should only be referenced by the callback registry in Thread.
+      ASSERT(data->ptr()->callback_id_ == 0);
+    }
+  }
+
+ private:
+  GrowableArray<RawFfiTrampolineData*> objects_;
+};
+#endif  // !DART_PRECOMPILED_RUNTIME
+
+class FfiTrampolineDataDeserializationCluster : public DeserializationCluster {
+ public:
+  FfiTrampolineDataDeserializationCluster() {}
+  ~FfiTrampolineDataDeserializationCluster() {}
+
+  void ReadAlloc(Deserializer* d) {
+    start_index_ = d->next_index();
+    PageSpace* old_space = d->heap()->old_space();
+    intptr_t count = d->ReadUnsigned();
+    for (intptr_t i = 0; i < count; i++) {
+      d->AssignRef(
+          AllocateUninitialized(old_space, FfiTrampolineData::InstanceSize()));
+    }
+    stop_index_ = d->next_index();
+  }
+
+  void ReadFill(Deserializer* d) {
+    for (intptr_t id = start_index_; id < stop_index_; id++) {
+      RawFfiTrampolineData* data =
+          reinterpret_cast<RawFfiTrampolineData*>(d->Ref(id));
+      Deserializer::InitializeHeader(data, kFfiTrampolineDataCid,
+                                     FfiTrampolineData::InstanceSize());
+      ReadFromTo(data);
+      data->ptr()->callback_id_ = 0;
+    }
+  }
+};
+
+#if !defined(DART_PRECOMPILED_RUNTIME)
 class RedirectionDataSerializationCluster : public SerializationCluster {
  public:
   RedirectionDataSerializationCluster()
@@ -4308,6 +4377,8 @@
       return new (Z) SignatureDataSerializationCluster();
     case kRedirectionDataCid:
       return new (Z) RedirectionDataSerializationCluster();
+    case kFfiTrampolineDataCid:
+      return new (Z) FfiTrampolineDataSerializationCluster();
     case kFieldCid:
       return new (Z) FieldSerializationCluster();
     case kScriptCid:
@@ -4944,6 +5015,8 @@
       return new (Z) SignatureDataDeserializationCluster();
     case kRedirectionDataCid:
       return new (Z) RedirectionDataDeserializationCluster();
+    case kFfiTrampolineDataCid:
+      return new (Z) FfiTrampolineDataDeserializationCluster();
     case kFieldCid:
       return new (Z) FieldDeserializationCluster();
     case kScriptCid:
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index 4294af9..ad74bbe 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -8271,7 +8271,7 @@
       Object::Allocate(FfiTrampolineData::kClassId,
                        FfiTrampolineData::InstanceSize(), Heap::kOld);
   RawFfiTrampolineData* data = reinterpret_cast<RawFfiTrampolineData*>(raw);
-  data->ptr()->callback_id_ = -1;
+  data->ptr()->callback_id_ = 0;
   return data;
 }
 
diff --git a/runtime/vm/raw_object.h b/runtime/vm/raw_object.h
index 91ff01e..10f1b00 100644
--- a/runtime/vm/raw_object.h
+++ b/runtime/vm/raw_object.h
@@ -1034,13 +1034,17 @@
   RawFunction* callback_target_;
 
   VISIT_TO(RawObject*, callback_target_);
+  RawObject** to_snapshot(Snapshot::Kind kind) { return to(); }
 
-  // Callback id for callbacks, otherwise 0.
+  // Callback id for callbacks.
   //
   // The callbacks ids are used so that native callbacks can lookup their own
   // code objects, since native code doesn't pass code objects into function
   // calls. The callback id is also used to for verifying that callbacks are
   // called on the correct isolate. See DLRT_VerifyCallbackIsolate for details.
+  //
+  // Will be 0 for non-callbacks. Check 'callback_target_' to determine if this
+  // is a callback or not.
   uint32_t callback_id_;
 };
 
diff --git a/runtime/vm/snapshot.cc b/runtime/vm/snapshot.cc
index 3824991..65258de 100644
--- a/runtime/vm/snapshot.cc
+++ b/runtime/vm/snapshot.cc
@@ -1134,7 +1134,12 @@
 
 #define SNAPSHOT_WRITE(clazz) case kFfi##clazz##Cid:
 
-    CLASS_LIST_FFI(SNAPSHOT_WRITE) { UNREACHABLE(); }
+      CLASS_LIST_FFI(SNAPSHOT_WRITE) {
+        SetWriteException(Exceptions::kArgument,
+                          "Native objects (from dart:ffi) such as Pointers and "
+                          "Structs cannot be passed between isolates.");
+        UNREACHABLE();
+      }
 #undef SNAPSHOT_WRITE
     default:
       break;
diff --git a/tests/ffi/ffi.status b/tests/ffi/ffi.status
index 0e982e4..61d2ce6 100644
--- a/tests/ffi/ffi.status
+++ b/tests/ffi/ffi.status
@@ -13,14 +13,6 @@
 function_structs_test: Skip
 structs_test: Skip
 
-# dartbug.com/35934
-[ $compiler == app_jitk ]
-dynamic_library_test: Skip
-function_callbacks_test: Skip
-function_structs_test: Skip
-function_test: Skip
-negative_function_test: Skip
-
 [ $arch == x64 || $arch == arm64 || $arch == simdbc64 ]
 enable_structs_test: SkipByDesign  # Tests that structs don't work on 32-bit systems.
 
@@ -44,3 +36,10 @@
 
 [ $arch == simdbc64 ]
 function_callbacks_test: Skip  # Issue 37140
+
+# These tests trigger and catch an abort (intentionally) and terminate the VM
+# before it can generate a snapshot.
+[ $compiler == app_jitk ]
+function_callbacks_test/01: Skip
+function_callbacks_test/02: Skip
+function_callbacks_test/03: Skip
diff --git a/tests/ffi/snapshot_test.dart b/tests/ffi/snapshot_test.dart
new file mode 100644
index 0000000..02b4cc4
--- /dev/null
+++ b/tests/ffi/snapshot_test.dart
@@ -0,0 +1,24 @@
+// Copyright (c) 2019, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+//
+// Checks that the VM throws an appropriate exception when FFI objects are
+// passed between isolates.
+
+import 'dart:async';
+import 'dart:ffi';
+import 'dart:io';
+import 'dart:isolate';
+
+import 'package:expect/expect.dart';
+
+main(args) async {
+  try {
+    await Isolate.spawn(print, fromAddress<Pointer<Void>>(1));
+  } catch (e) {
+    Expect.type<ArgumentError>(e);
+    return;
+  }
+
+  throw "Test didn't throw an exception!";
+}