[vm] Fill in ArgumentValue.invalidObject when objects are rejected in isolate messages.

rmacnak-google · Commit Queue · commit 9319d07e11b2 · 2022-10-24T17:49:29.000Z
TEST=ci Change-Id: I5a2816ce7fd40463aaffd971d867955a7b2bd7e6 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/265043 Reviewed-by: Alexander Aprelev <aam@google.com> Reviewed-by: Martin Kustermann <kustermann@google.com> Commit-Queue: Ryan Macnak <rmacnak@google.com>
diff --git a/runtime/lib/isolate.cc b/runtime/lib/isolate.cc
@@ -188,13 +188,9 @@ static ObjectPtr ValidateMessageObject(Zone* zone,
 
   Class& klass = Class::Handle(zone);
   Closure& closure = Closure::Handle(zone);
-
-  bool error_found = false;
-  Function& erroneous_closure_function = Function::Handle(zone);
-  Class& erroneous_nativewrapper_class = Class::Handle(zone);
-  Class& erroneous_finalizable_class = Class::Handle(zone);
   Array& array = Array::Handle(zone);
-  const char* error_message = nullptr;
+  Object& illegal_object = Object::Handle(zone);
+  const char* exception_message = nullptr;
   Thread* thread = Thread::Current();
 
   // working_set contains only elements that have not been visited yet that
@@ -214,7 +210,7 @@ static ObjectPtr ValidateMessageObject(Zone* zone,
   visited->SetValueExclusive(obj.ptr(), 1);
   working_set->Add(obj.ptr());
 
-  while (!working_set->is_empty() && !error_found) {
+  while (!working_set->is_empty() && (exception_message == nullptr)) {
     thread->CheckForSafepoint();
 
     ObjectPtr raw = working_set->RemoveLast();
@@ -265,9 +261,8 @@ static ObjectPtr ValidateMessageObject(Zone* zone,
 
 #define MESSAGE_SNAPSHOT_ILLEGAL(type)                                         \
   case k##type##Cid:                                                           \
-    error_message =                                                            \
-        "Illegal argument in isolate message : (object is a " #type ")";       \
-    error_found = true;                                                        \
+    illegal_object = raw;                                                      \
+    exception_message = "is a " #type;                                         \
     break;
 
         MESSAGE_SNAPSHOT_ILLEGAL(DynamicLibrary);
@@ -284,45 +279,33 @@ static ObjectPtr ValidateMessageObject(Zone* zone,
         if (cid >= kNumPredefinedCids) {
           klass = class_table->At(cid);
           if (klass.num_native_fields() != 0) {
-            erroneous_nativewrapper_class = klass.ptr();
-            error_found = true;
+            illegal_object = raw;
+            exception_message = "is a NativeWrapper";
             break;
           }
           if (klass.implements_finalizable()) {
-            erroneous_finalizable_class = klass.ptr();
-            error_found = true;
+            illegal_object = raw;
+            exception_message = "is a Finalizable";
             break;
           }
         }
     }
     raw->untag()->VisitPointers(&visitor);
   }
-  if (error_found) {
-    const char* exception_message;
-    if (error_message != nullptr) {
-      exception_message = error_message;
-    } else if (!erroneous_closure_function.IsNull()) {
-      exception_message = OS::SCreate(zone,
-                                      "Illegal argument in isolate message"
-                                      " : (object is a closure - %s)",
-                                      erroneous_closure_function.ToCString());
-    } else if (!erroneous_nativewrapper_class.IsNull()) {
-      exception_message =
-          OS::SCreate(zone,
-                      "Illegal argument in isolate message"
-                      " : (object extends NativeWrapper - %s)",
-                      erroneous_nativewrapper_class.ToCString());
-    } else {
-      ASSERT(!erroneous_finalizable_class.IsNull());
-      exception_message = OS::SCreate(zone,
-                                      "Illegal argument in isolate message"
-                                      " : (object implements Finalizable - %s)",
-                                      erroneous_finalizable_class.ToCString());
-    }
+
+  ASSERT((exception_message == nullptr) == illegal_object.IsNull());
+  if (exception_message != nullptr) {
     working_set->Clear();
-    return Exceptions::CreateUnhandledException(
-        zone, Exceptions::kArgumentValue, exception_message);
+
+    const Array& args = Array::Handle(zone, Array::New(3));
+    args.SetAt(0, illegal_object);
+    args.SetAt(2, String::Handle(zone, String::New(exception_message)));
+    const Object& exception = Object::Handle(
+        zone, Exceptions::Create(Exceptions::kArgumentValue, args));
+    return UnhandledException::New(Instance::Cast(exception),
+                                   StackTrace::Handle(zone));
   }
+
   ASSERT(working_set->length() == 0);
   isolate->set_forward_table_new(nullptr);
   return obj.ptr();
diff --git a/runtime/tests/vm/dart/sendandexit_test.dart b/runtime/tests/vm/dart/sendandexit_test.dart
@@ -30,21 +30,25 @@ class NativeWrapperClass extends NativeFieldWrapperClass1 {}
 
 verifyCantSendNative() async {
   final receivePort = ReceivePort();
+  final unsendable = NativeWrapperClass();
   Expect.throws(
-      () => Isolate.exit(receivePort.sendPort, NativeWrapperClass()),
-      (e) => e.toString().startsWith('Invalid argument: '
-          '"Illegal argument in isolate message : '
-          '(object extends NativeWrapper'));
+      () => Isolate.exit(receivePort.sendPort, unsendable),
+      (e) =>
+          e is ArgumentError &&
+          e.invalidValue == unsendable &&
+          e.toString().contains("NativeWrapper"));
   receivePort.close();
 }
 
 verifyCantSendReceivePort() async {
-  final receivePort = ReceivePort();
+  final receivePort = RawReceivePort();
+  final unsendable = receivePort;
   Expect.throws(
       () => Isolate.exit(receivePort.sendPort, receivePort),
-      (e) => e.toString().startsWith(
-          'Invalid argument: "Illegal argument in isolate message : '
-          '(object is a ReceivePort)\"'));
+      (e) =>
+          e is ArgumentError &&
+          e.invalidValue == unsendable &&
+          e.toString().contains("ReceivePort"));
   receivePort.close();
 }
 
diff --git a/runtime/tests/vm/dart_2/sendandexit_test.dart b/runtime/tests/vm/dart_2/sendandexit_test.dart
@@ -32,21 +32,25 @@ class NativeWrapperClass extends NativeFieldWrapperClass1 {}
 
 verifyCantSendNative() async {
   final receivePort = ReceivePort();
+  final unsendable = NativeWrapperClass();
   Expect.throws(
-      () => Isolate.exit(receivePort.sendPort, NativeWrapperClass()),
-      (e) => e.toString().startsWith('Invalid argument: '
-          '"Illegal argument in isolate message : '
-          '(object extends NativeWrapper'));
+      () => Isolate.exit(receivePort.sendPort, unsendable),
+      (e) =>
+          e is ArgumentError &&
+          e.invalidValue == unsendable &&
+          e.toString().contains("NativeWrapper"));
   receivePort.close();
 }
 
 verifyCantSendReceivePort() async {
-  final receivePort = ReceivePort();
+  final receivePort = RawReceivePort();
+  final unsendable = receivePort;
   Expect.throws(
-      () => Isolate.exit(receivePort.sendPort, receivePort),
-      (e) => e.toString().startsWith(
-          'Invalid argument: "Illegal argument in isolate message : '
-          '(object is a ReceivePort)\"'));
+      () => Isolate.exit(receivePort.sendPort, unsendable),
+      (e) =>
+          e is ArgumentError &&
+          e.invalidValue == unsendable &&
+          e.toString().contains("ReceivePort"));
   receivePort.close();
 }
 
diff --git a/runtime/vm/dart_api_impl_test.cc b/runtime/vm/dart_api_impl_test.cc
@@ -8526,7 +8526,7 @@ TEST_CASE(DartAPI_NativePortPostUserClass) {
     Dart_Handle dart_args[1];
     dart_args[0] = send_port;
     Dart_Handle result = Dart_Invoke(lib, NewString("callPort"), 1, dart_args);
-    EXPECT_ERROR(result, "Illegal argument in isolate message");
+    EXPECT_ERROR(result, "Invalid argument");
   }
 
   // Test send with port closed.
@@ -8537,7 +8537,7 @@ TEST_CASE(DartAPI_NativePortPostUserClass) {
     Dart_Handle dart_args[1];
     dart_args[0] = send_port;
     Dart_Handle result = Dart_Invoke(lib, NewString("callPort"), 1, dart_args);
-    EXPECT_ERROR(result, "Illegal argument in isolate message");
+    EXPECT_ERROR(result, "Invalid argument");
   }
 
   Dart_ExitScope();
diff --git a/runtime/vm/message_snapshot.cc b/runtime/vm/message_snapshot.cc
@@ -276,7 +276,6 @@ class MessageSerializer : public BaseSerializer {
     WriteUnsigned(index);
   }
 
-  const char* exception_message() const { return exception_message_; }
   Thread* thread() const {
     return static_cast<Thread*>(StackResource::thread());
   }
@@ -291,7 +290,6 @@ class MessageSerializer : public BaseSerializer {
   WeakTable* forward_table_new_;
   WeakTable* forward_table_old_;
   GrowableArray<Object*> stack_;
-  const char* exception_message_;
 };
 
 class ApiMessageSerializer : public BaseSerializer {
@@ -2161,10 +2159,8 @@ class TransferableTypedDataMessageSerializationCluster
     TransferableTypedDataPeer* tpeer =
         reinterpret_cast<TransferableTypedDataPeer*>(peer);
     if (tpeer->data() == nullptr) {
-      s->IllegalObject(
-          *object,
-          "Illegal argument in isolate message"
-          " : (TransferableTypedData has been transferred already)");
+      s->IllegalObject(*object,
+                       "TransferableTypedData has been transferred already");
     }
   }
 
@@ -3218,8 +3214,7 @@ MessageSerializer::MessageSerializer(Thread* thread)
     : BaseSerializer(thread, thread->zone()),
       forward_table_new_(),
       forward_table_old_(),
-      stack_(thread->zone(), 0),
-      exception_message_(nullptr) {
+      stack_(thread->zone(), 0) {
   isolate()->set_forward_table_new(new WeakTable());
   isolate()->set_forward_table_old(new WeakTable());
 }
@@ -3274,27 +3269,20 @@ void MessageSerializer::Trace(Object* object) {
       if ((clazz.library() != object_store->core_library()) &&
           (clazz.library() != object_store->collection_library()) &&
           (clazz.library() != object_store->typed_data_library())) {
-        IllegalObject(*object,
-                      "Illegal argument in isolate message"
-                      " : (object is a regular Dart Instance)");
+        IllegalObject(*object, "is a regular instance");
       }
       if (clazz.num_native_fields() != 0) {
-        char* chars = OS::SCreate(thread()->zone(),
-                                  "Illegal argument in isolate message"
-                                  " : (object extends NativeWrapper - %s)",
-                                  clazz.ToCString());
-        IllegalObject(*object, chars);
+        IllegalObject(*object, "is a NativeWrapper");
       }
     }
 
     // Keep the list in sync with the one in lib/isolate.cc
 #define ILLEGAL(type)                                                          \
   if (cid == k##type##Cid) {                                                   \
-    IllegalObject(*object,                                                     \
-                  "Illegal argument in isolate message"                        \
-                  " : (object is a " #type ")");                               \
+    IllegalObject(*object, "is a " #type);                                     \
   }
 
+    ILLEGAL(Closure)
     ILLEGAL(FunctionType)
     ILLEGAL(MirrorReference)
     ILLEGAL(ReceivePort)
@@ -3314,15 +3302,6 @@ void MessageSerializer::Trace(Object* object) {
 
 #undef ILLEGAL
 
-    if (cid == kClosureCid) {
-      Closure* closure = static_cast<Closure*>(object);
-      const char* message = OS::SCreate(
-          zone(),
-          "Illegal argument in isolate message : (object is a closure - %s)",
-          Function::Handle(closure->function()).ToCString());
-      IllegalObject(*object, message);
-    }
-
     if (cid >= kNumPredefinedCids || cid == kInstanceCid ||
         cid == kByteBufferCid) {
       Push(isolate_group()->class_table()->At(cid));
@@ -3570,8 +3549,10 @@ bool ApiMessageSerializer::Trace(Dart_CObject* object) {
 
 void MessageSerializer::IllegalObject(const Object& object,
                                       const char* message) {
-  exception_message_ = message;
-  thread()->long_jump_base()->Jump(1, Object::snapshot_writer_error());
+  const Array& args = Array::Handle(zone(), Array::New(3));
+  args.SetAt(0, object);
+  args.SetAt(2, String::Handle(zone(), String::New(message)));
+  Exceptions::ThrowByType(Exceptions::kArgumentValue, args);
 }
 
 BaseDeserializer::BaseDeserializer(Zone* zone, Message* message)
@@ -3994,31 +3975,7 @@ std::unique_ptr<Message> WriteMessage(bool same_group,
 
   Thread* thread = Thread::Current();
   MessageSerializer serializer(thread);
-
-  volatile bool has_exception = false;
-  {
-    LongJumpScope jump(thread);
-    if (setjmp(*jump.Set()) == 0) {
-      serializer.Serialize(obj);
-    } else {
-      has_exception = true;
-    }
-  }
-
-  if (has_exception) {
-    {
-      NoSafepointScope no_safepoint;
-      ErrorPtr error = thread->StealStickyError();
-      ASSERT(error == Object::snapshot_writer_error().ptr());
-    }
-
-    const String& msg_obj =
-        String::Handle(String::New(serializer.exception_message()));
-    const Array& args = Array::Handle(Array::New(1));
-    args.SetAt(0, msg_obj);
-    Exceptions::ThrowByType(Exceptions::kArgument, args);
-  }
-
+  serializer.Serialize(obj);
   return serializer.Finish(dest_port, priority);
 }
 
diff --git a/tests/ffi/vmspecific_native_finalizer_isolates_test.dart b/tests/ffi/vmspecific_native_finalizer_isolates_test.dart
@@ -56,13 +56,13 @@ Future<void> testSendAndExitFinalizable() async {
         Isolate.exit(sendPort, MyFinalizable());
       } catch (e) {
         print('Expected exception: $e.');
-        Isolate.exit(sendPort, e);
+        Isolate.exit(sendPort, e.toString());
       }
     },
     receivePort.sendPort,
   );
   final result = await receivePort.first;
-  Expect.type<ArgumentError>(result);
+  Expect.contains("Invalid argument: is a Finalizable", result);
 }
 
 Future<void> testSendAndExitFinalizer() async {
@@ -73,11 +73,11 @@ Future<void> testSendAndExitFinalizer() async {
         Isolate.exit(sendPort, MyFinalizable());
       } catch (e) {
         print('Expected exception: $e.');
-        Isolate.exit(sendPort, e);
+        Isolate.exit(sendPort, e.toString());
       }
     },
     receivePort.sendPort,
   );
   final result = await receivePort.first;
-  Expect.type<ArgumentError>(result);
+  Expect.contains("Invalid argument: is a Finalizable", result);
 }
diff --git a/tests/ffi_2/vmspecific_native_finalizer_isolates_test.dart b/tests/ffi_2/vmspecific_native_finalizer_isolates_test.dart
@@ -58,13 +58,13 @@ Future<void> testSendAndExitFinalizable() async {
         Isolate.exit(sendPort, MyFinalizable());
       } catch (e) {
         print('Expected exception: $e.');
-        Isolate.exit(sendPort, e);
+        Isolate.exit(sendPort, e.toString());
       }
     },
     receivePort.sendPort,
   );
   final result = await receivePort.first;
-  Expect.type<ArgumentError>(result);
+  Expect.contains("Invalid argument: is a Finalizable", result);
 }
 
 Future<void> testSendAndExitFinalizer() async {
@@ -75,11 +75,11 @@ Future<void> testSendAndExitFinalizer() async {
         Isolate.exit(sendPort, MyFinalizable());
       } catch (e) {
         print('Expected exception: $e.');
-        Isolate.exit(sendPort, e);
+        Isolate.exit(sendPort, e.toString());
       }
     },
     receivePort.sendPort,
   );
   final result = await receivePort.first;
-  Expect.type<ArgumentError>(result);
+  Expect.contains("Invalid argument: is a Finalizable", result);
 }
