Hide Isolate pointer from embedder

runtime/vm/dart_api_impl.cc

Index: runtime/vm/dart_api_impl.cc
diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc
index 1f2dfc3844263e120bd910b1158d735df3fa3601..17ef12f6e79beca78cfdfdfd59589f2a6aa7df5d 100644
--- a/runtime/vm/dart_api_impl.cc
+++ b/runtime/vm/dart_api_impl.cc
@@ -291,6 +291,13 @@ static RawObject* Send1Arg(const Instance& receiver,
 }
 
 
+// Convert a |Dart_Isolate| into an |Isolate|. Returns NULL if no isolate

[Ivan Posva, 2015/05/11 04:50:15]

Why not move this helper function to where it is being used in Api::CastIsolate?
Or even just inline it into the relevant casting function?

[Cutch, 2015/05/12 03:21:28]

Done.

+// could be found.
+static Isolate* LookupIsolate(Dart_Isolate isolate) {
+  return PortMap::GetIsolate(static_cast<Dart_Port>(isolate));
+}
+
+
 WeakReferenceSetBuilder* ApiState::NewWeakReferenceSetBuilder() {
   return new WeakReferenceSetBuilder(this);
 }
@@ -390,7 +397,15 @@ Dart_Handle Api::CheckAndFinalizePendingClasses(Isolate* isolate) {
 
 
 Dart_Isolate Api::CastIsolate(Isolate* isolate) {
-  return reinterpret_cast<Dart_Isolate>(isolate);
+  if (isolate == NULL) {
+    return ILLEGAL_ISOLATE;
+  }
+  return static_cast<Dart_Isolate>(isolate->main_port());
+}
+
+
+Isolate* Api::CastIsolate(Dart_Isolate isolate) {
+  return LookupIsolate(isolate);
 }
 
 
@@ -1301,12 +1316,12 @@ DART_EXPORT Dart_Isolate Dart_CreateIsolate(const char* script_uri,
         Library::CheckFunctionFingerprints();
       }
       START_TIMER(isolate, time_total_runtime);
-      return reinterpret_cast<Dart_Isolate>(isolate);
+      return Api::CastIsolate(isolate);
     }
     *error = strdup(error_obj.ToErrorCString());
   }
   Dart::ShutdownIsolate();
-  return reinterpret_cast<Dart_Isolate>(NULL);
+  return ILLEGAL_ISOLATE;
 }
 
 
@@ -1337,11 +1352,11 @@ DART_EXPORT void* Dart_CurrentIsolateData() {
 
 DART_EXPORT void* Dart_IsolateData(Dart_Isolate isolate) {
   TRACE_API_CALL(CURRENT_FUNC);
-  if (isolate == NULL) {
-    FATAL1("%s expects argument 'isolate' to be non-null.",  CURRENT_FUNC);
+  Isolate* iso = LookupIsolate(isolate);

[Ivan Posva, 2015/05/11 04:50:15]

Why not Api::CastIsolate?

[Cutch, 2015/05/12 03:21:28]

Done.

+  if (iso == NULL) {
+    FATAL1("%s expects argument 'isolate' to be a valid isolate.",
+           CURRENT_FUNC);

[Ivan Posva, 2015/05/11 04:50:15]

FATAL is a very unfriendly error from the position of the embedder. We should
evaluate our options here which would avoid killing the process.

[Cutch, 2015/05/12 03:21:29]

I've removed most uses of FATAL in this file. PTAL.

   }
-  // TODO(16615): Validate isolate parameter.
-  Isolate* iso = reinterpret_cast<Isolate*>(isolate);
   return iso->init_callback_data();
 }
 
@@ -1356,8 +1371,11 @@ DART_EXPORT Dart_Handle Dart_DebugName() {
 
 DART_EXPORT void Dart_EnterIsolate(Dart_Isolate isolate) {
   CHECK_NO_ISOLATE(Isolate::Current());
-  // TODO(16615): Validate isolate parameter.
-  Isolate* iso = reinterpret_cast<Isolate*>(isolate);
+  Isolate* iso = LookupIsolate(isolate);

[Ivan Posva, 2015/05/11 04:50:15]

ditto here and below.

[Cutch, 2015/05/12 03:21:28]

Done.

+  if (iso == NULL) {
+    FATAL1("%s expects argument 'isolate' to be a valid isolate.",
+           CURRENT_FUNC);
+  }
   if (iso->mutator_thread() != NULL) {
     FATAL("Multiple mutators within one isolate is not supported.");
   }
@@ -1482,11 +1500,11 @@ DART_EXPORT Dart_Handle Dart_CreateScriptSnapshot(uint8_t** buffer,
 
 DART_EXPORT void Dart_InterruptIsolate(Dart_Isolate isolate) {
   TRACE_API_CALL(CURRENT_FUNC);
-  if (isolate == NULL) {
-    FATAL1("%s expects argument 'isolate' to be non-null.",  CURRENT_FUNC);
+  Isolate* iso = LookupIsolate(isolate);
+  if (iso == NULL) {
+    FATAL1("%s expects argument 'isolate' to be a valid isolate.",
+           CURRENT_FUNC);
   }
-  // TODO(16615): Validate isolate parameter.
-  Isolate* iso = reinterpret_cast<Isolate*>(isolate);
   iso->ScheduleInterrupts(Isolate::kApiInterrupt);
   // Can't use Dart_Post() since there isn't a current isolate.
   Dart_CObject api_null = { Dart_CObject_kNull , { 0 } };
@@ -1496,11 +1514,11 @@ DART_EXPORT void Dart_InterruptIsolate(Dart_Isolate isolate) {
 
 DART_EXPORT bool Dart_IsolateMakeRunnable(Dart_Isolate isolate) {
   CHECK_NO_ISOLATE(Isolate::Current());
-  if (isolate == NULL) {
-    FATAL1("%s expects argument 'isolate' to be non-null.",  CURRENT_FUNC);
+  Isolate* iso = LookupIsolate(isolate);
+  if (iso == NULL) {
+    FATAL1("%s expects argument 'isolate' to be a valid isolate.",
+           CURRENT_FUNC);
   }
-  // TODO(16615): Validate isolate parameter.
-  Isolate* iso = reinterpret_cast<Isolate*>(isolate);
   if (iso->object_store()->root_library() == Library::null()) {
     // The embedder should have called Dart_LoadScript by now.
     return false;
@@ -5547,7 +5565,7 @@ DART_EXPORT Dart_Handle Dart_SetPeer(Dart_Handle object, void* peer) {
 // --- Service support ---
 
 DART_EXPORT bool Dart_IsServiceIsolate(Dart_Isolate isolate) {
-  Isolate* iso = reinterpret_cast<Isolate*>(isolate);
+  Isolate* iso = LookupIsolate(isolate);
   return ServiceIsolate::IsServiceIsolate(iso);
 }