[d8] bounds-check before getting Shell::Worker internal field

src/d8.cc

Index: src/d8.cc
diff --git a/src/d8.cc b/src/d8.cc
index 7db6f3ed9e442081528e05140bd25b8911e26cb6..b47dc4f07003afb51fa17ce2562c7f777c0e7f9e 100644
--- a/src/d8.cc
+++ b/src/d8.cc
@@ -717,14 +717,18 @@ void Shell::WorkerPostMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
   Local<Context> context = isolate->GetCurrentContext();
+  Local<Value> this_value;
 
   if (args.Length() < 1) {
     Throw(isolate, "Invalid argument");
     return;
   }
 
-  Local<Value> this_value = args.This()->GetInternalField(0);
-  if (!this_value->IsExternal()) {
+  // Bounds-check to avoid fatal error in debug mode

[adamk, 2015/07/07 18:48:26]

I'd just remove this comment: it's not a "bounds-check" in the traditional
sense, it's actually checking for the existence of the internal field.

[caitp (gmail), 2015/07/07 19:00:17]

Acknowledged.

+  if (args.This()->InternalFieldCount() > 0) {
+    this_value = args.This()->GetInternalField(0);
+  }
+  if (this_value.IsEmpty() || !this_value->IsExternal()) {

[adamk, 2015/07/07 18:48:26]

Can the second part of this conditional ever fail? I'm guessing not...

[caitp (gmail), 2015/07/07 19:00:17]

right now, I don't think so

     Throw(isolate, "this is not a Worker");
     return;
   }
@@ -770,9 +774,12 @@ void Shell::WorkerPostMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
 void Shell::WorkerGetMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
-
-  Local<Value> this_value = args.This()->GetInternalField(0);
-  if (!this_value->IsExternal()) {
+  Local<Value> this_value;
+  // Bounds-check to avoid fatal error in debug mode

[adamk, 2015/07/07 18:48:26]

Ditto, comment seems off.

+  if (args.This()->InternalFieldCount() > 0) {
+    this_value = args.This()->GetInternalField(0);
+  }
+  if (this_value.IsEmpty() || !this_value->IsExternal()) {

[adamk, 2015/07/07 18:48:26]

Ditto, I don't think IsExternal can fail now

     Throw(isolate, "this is not a Worker");
     return;
   }
@@ -795,8 +802,12 @@ void Shell::WorkerGetMessage(const v8::FunctionCallbackInfo<v8::Value>& args) {
 void Shell::WorkerTerminate(const v8::FunctionCallbackInfo<v8::Value>& args) {
   Isolate* isolate = args.GetIsolate();
   HandleScope handle_scope(isolate);
-  Local<Value> this_value = args.This()->GetInternalField(0);
-  if (!this_value->IsExternal()) {
+  Local<Value> this_value;
+  // Bounds-check to avoid fatal error in debug mode

[adamk, 2015/07/07 18:48:26]

And here...

+  if (args.This()->InternalFieldCount() > 0) {
+    this_value = args.This()->GetInternalField(0);
+  }
+  if (this_value.IsEmpty() || !this_value->IsExternal()) {

[adamk, 2015/07/07 18:48:26]

And here

     Throw(isolate, "this is not a Worker");
     return;
   }