Make exception thrown via v8 public API propagate to v8::TryCatch as JS thrown except...
authorantonm@chromium.org <antonm@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Thu, 7 Apr 2011 19:52:24 +0000 (19:52 +0000)
committerantonm@chromium.org <antonm@chromium.org@ce2b1a6d-e550-0410-aec6-3dcde31c8c00>
Thu, 7 Apr 2011 19:52:24 +0000 (19:52 +0000)
Correctly process failures which can be returned by Object::GetProperty
when performing GetRealNamedProperty* queries.

Callback properties can produce exceptions so we need to wrap access to them
into exception checks.  However, despite of many other methods with exception
checks, property access doesn't mandatroy go via JavaScript and hence we
need to inject code to propagate exception to public API TryCatch handlers.

Review URL: http://codereview.chromium.org/6685087

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@7548 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/api.cc
src/debug.cc
src/handles.cc
src/handles.h
src/isolate.cc
src/isolate.h
src/messages.cc
src/messages.h
src/top.cc
test/cctest/test-api.cc

index 056c0be..c2b2df8 100644 (file)
@@ -2580,6 +2580,9 @@ bool v8::Object::SetPrototype(Handle<Value> value) {
   ENTER_V8(isolate);
   i::Handle<i::JSObject> self = Utils::OpenHandle(this);
   i::Handle<i::Object> value_obj = Utils::OpenHandle(*value);
+  // We do not allow exceptions thrown while setting the prototype
+  // to propagate outside.
+  TryCatch try_catch;
   EXCEPTION_PREAMBLE(isolate);
   i::Handle<i::Object> result = i::SetPrototype(self, value_obj);
   has_pending_exception = result.is_null();
@@ -2792,6 +2795,26 @@ bool v8::Object::HasIndexedLookupInterceptor() {
 }
 
 
+static Local<Value> GetPropertyByLookup(i::Isolate* isolate,
+                                        i::Handle<i::JSObject> receiver,
+                                        i::Handle<i::String> name,
+                                        i::LookupResult* lookup) {
+  if (!lookup->IsProperty()) {
+    // No real property was found.
+    return Local<Value>();
+  }
+
+  // If the property being looked up is a callback, it can throw
+  // an exception.
+  EXCEPTION_PREAMBLE(isolate);
+  i::Handle<i::Object> result = i::GetProperty(receiver, name, lookup);
+  has_pending_exception = result.is_null();
+  EXCEPTION_BAILOUT_CHECK(isolate, Local<Value>());
+
+  return Utils::ToLocal(result);
+}
+
+
 Local<Value> v8::Object::GetRealNamedPropertyInPrototypeChain(
       Handle<String> key) {
   i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();
@@ -2803,17 +2826,7 @@ Local<Value> v8::Object::GetRealNamedPropertyInPrototypeChain(
   i::Handle<i::String> key_obj = Utils::OpenHandle(*key);
   i::LookupResult lookup;
   self_obj->LookupRealNamedPropertyInPrototypes(*key_obj, &lookup);
-  if (lookup.IsProperty()) {
-    PropertyAttributes attributes;
-    i::Object* property =
-        self_obj->GetProperty(*self_obj,
-                              &lookup,
-                              *key_obj,
-                              &attributes)->ToObjectUnchecked();
-    i::Handle<i::Object> result(property);
-    return Utils::ToLocal(result);
-  }
-  return Local<Value>();  // No real property was found in prototype chain.
+  return GetPropertyByLookup(isolate, self_obj, key_obj, &lookup);
 }
 
 
@@ -2826,17 +2839,7 @@ Local<Value> v8::Object::GetRealNamedProperty(Handle<String> key) {
   i::Handle<i::String> key_obj = Utils::OpenHandle(*key);
   i::LookupResult lookup;
   self_obj->LookupRealNamedProperty(*key_obj, &lookup);
-  if (lookup.IsProperty()) {
-    PropertyAttributes attributes;
-    i::Object* property =
-        self_obj->GetProperty(*self_obj,
-                              &lookup,
-                              *key_obj,
-                              &attributes)->ToObjectUnchecked();
-    i::Handle<i::Object> result(property);
-    return Utils::ToLocal(result);
-  }
-  return Local<Value>();  // No real property was found in prototype chain.
+  return GetPropertyByLookup(isolate, self_obj, key_obj, &lookup);
 }
 
 
index d6f91d8..fc62e30 100644 (file)
@@ -810,7 +810,7 @@ bool Debug::CompileDebuggerScript(int index) {
     Handle<Object> message = MessageHandler::MakeMessageObject(
         "error_loading_debugger", NULL, Vector<Handle<Object> >::empty(),
         Handle<String>(), Handle<JSArray>());
-    MessageHandler::ReportMessage(NULL, message);
+    MessageHandler::ReportMessage(Isolate::Current(), NULL, message);
     return false;
   }
 
index 97a06d9..326de86 100644 (file)
@@ -369,6 +369,17 @@ Handle<Object> GetProperty(Handle<Object> obj,
 }
 
 
+Handle<Object> GetProperty(Handle<JSObject> obj,
+                           Handle<String> name,
+                           LookupResult* result) {
+  PropertyAttributes attributes;
+  Isolate* isolate = Isolate::Current();
+  CALL_HEAP_FUNCTION(isolate,
+                     obj->GetProperty(*obj, result, *name, &attributes),
+                     Object);
+}
+
+
 Handle<Object> GetElement(Handle<Object> obj,
                           uint32_t index) {
   Isolate* isolate = Isolate::Current();
index a357a00..3839f37 100644 (file)
@@ -244,6 +244,11 @@ Handle<Object> GetProperty(Handle<JSObject> obj,
 Handle<Object> GetProperty(Handle<Object> obj,
                            Handle<Object> key);
 
+Handle<Object> GetProperty(Handle<JSObject> obj,
+                           Handle<String> name,
+                           LookupResult* result);
+
+
 Handle<Object> GetElement(Handle<Object> obj,
                           uint32_t index);
 
index cc9bc37..b8a7fb7 100644 (file)
@@ -701,6 +701,33 @@ void Isolate::InitializeThreadLocal() {
 }
 
 
+void Isolate::PropagatePendingExceptionToExternalTryCatch() {
+  ASSERT(has_pending_exception());
+
+  bool external_caught = IsExternallyCaught();
+  thread_local_top_.external_caught_exception_ = external_caught;
+
+  if (!external_caught) return;
+
+  if (thread_local_top_.pending_exception_ == Failure::OutOfMemoryException()) {
+    // Do not propagate OOM exception: we should kill VM asap.
+  } else if (thread_local_top_.pending_exception_ ==
+             heap()->termination_exception()) {
+    try_catch_handler()->can_continue_ = false;
+    try_catch_handler()->exception_ = heap()->null_value();
+  } else {
+    // At this point all non-object (failure) exceptions have
+    // been dealt with so this shouldn't fail.
+    ASSERT(!pending_exception()->IsFailure());
+    try_catch_handler()->can_continue_ = true;
+    try_catch_handler()->exception_ = pending_exception();
+    if (!thread_local_top_.pending_message_obj_->IsTheHole()) {
+      try_catch_handler()->message_ = thread_local_top_.pending_message_obj_;
+    }
+  }
+}
+
+
 bool Isolate::Init(Deserializer* des) {
   ASSERT(state_ != INITIALIZED);
 
index 638658b..856d2ce 100644 (file)
@@ -189,6 +189,9 @@ class ThreadLocalTop BASE_EMBEDDED {
   // unify them later.
   MaybeObject* scheduled_exception_;
   bool external_caught_exception_;
+  // True if unhandled message is being currently reported by
+  // MessageHandler::ReportMessage.
+  bool in_exception_reporting_;
   SaveContext* save_context_;
   v8::TryCatch* catcher_;
 
@@ -495,6 +498,9 @@ class Isolate {
   bool external_caught_exception() {
     return thread_local_top_.external_caught_exception_;
   }
+  void set_external_caught_exception(bool value) {
+    thread_local_top_.external_caught_exception_ = value;
+  }
   void set_pending_exception(MaybeObject* exception) {
     thread_local_top_.pending_exception_ = exception;
   }
@@ -522,6 +528,18 @@ class Isolate {
   bool* external_caught_exception_address() {
     return &thread_local_top_.external_caught_exception_;
   }
+  bool in_exception_reporting() {
+    return thread_local_top_.in_exception_reporting_;
+  }
+  void set_in_exception_reporting(bool value) {
+    thread_local_top_.in_exception_reporting_ = value;
+  }
+  v8::TryCatch* catcher() {
+    return thread_local_top_.catcher_;
+  }
+  void set_catcher(v8::TryCatch* catcher) {
+    thread_local_top_.catcher_ = catcher;
+  }
 
   MaybeObject** scheduled_exception_address() {
     return &thread_local_top_.scheduled_exception_;
@@ -592,6 +610,27 @@ class Isolate {
   // JavaScript code.  If an exception is scheduled true is returned.
   bool OptionalRescheduleException(bool is_bottom_call);
 
+  class ExceptionScope {
+   public:
+    explicit ExceptionScope(Isolate* isolate) :
+      // Scope currently can only be used for regular exceptions, not
+      // failures like OOM or termination exception.
+      isolate_(isolate),
+      pending_exception_(isolate_->pending_exception()->ToObjectUnchecked()),
+      catcher_(isolate_->catcher())
+    { }
+
+    ~ExceptionScope() {
+      isolate_->set_catcher(catcher_);
+      isolate_->set_pending_exception(*pending_exception_);
+    }
+
+   private:
+    Isolate* isolate_;
+    Handle<Object> pending_exception_;
+    v8::TryCatch* catcher_;
+  };
+
   void SetCaptureStackTraceForUncaughtExceptions(
       bool capture,
       int frame_limit,
@@ -1017,6 +1056,8 @@ class Isolate {
 
   void FillCache();
 
+  void PropagatePendingExceptionToExternalTryCatch();
+
   int stack_trace_nesting_level_;
   StringStream* incomplete_message_;
   // The preallocated memory thread singleton.
index cab982c..5e15b93 100644 (file)
@@ -106,14 +106,34 @@ Handle<JSMessageObject> MessageHandler::MakeMessageObject(
 }
 
 
-void MessageHandler::ReportMessage(MessageLocation* loc,
+void MessageHandler::ReportMessage(Isolate* isolate,
+                                   MessageLocation* loc,
                                    Handle<Object> message) {
+  // If we are in process of message reporting, just ignore all other requests
+  // to report a message as they are due to unhandled exceptions thrown in
+  // message callbacks.
+  if (isolate->in_exception_reporting()) {
+    ReportMessage("uncaught exception thrown while reporting");
+    return;
+  }
+  isolate->set_in_exception_reporting(true);
+
+  // We are calling into embedder's code which can throw exceptions.
+  // Thus we need to save current exception state, reset it to the clean one
+  // and ignore scheduled exceptions callbacks can throw.
+  Isolate::ExceptionScope exception_scope(isolate);
+  isolate->clear_pending_exception();
+  isolate->set_external_caught_exception(false);
+
   v8::Local<v8::Message> api_message_obj = v8::Utils::MessageToLocal(message);
 
   v8::NeanderArray global_listeners(FACTORY->message_listeners());
   int global_length = global_listeners.length();
   if (global_length == 0) {
     DefaultMessageReport(loc, message);
+    if (isolate->has_scheduled_exception()) {
+      isolate->clear_scheduled_exception();
+    }
   } else {
     for (int i = 0; i < global_length; i++) {
       HandleScope scope;
@@ -124,8 +144,13 @@ void MessageHandler::ReportMessage(MessageLocation* loc,
           FUNCTION_CAST<v8::MessageCallback>(callback_obj->proxy());
       Handle<Object> callback_data(listener.get(1));
       callback(api_message_obj, v8::Utils::ToLocal(callback_data));
+      if (isolate->has_scheduled_exception()) {
+        isolate->clear_scheduled_exception();
+      }
     }
   }
+
+  isolate->set_in_exception_reporting(false);
 }
 
 
index 48f3244..0d5f9f7 100644 (file)
@@ -101,7 +101,9 @@ class MessageHandler {
       Handle<JSArray> stack_frames);
 
   // Report a formatted message (needs JS allocation).
-  static void ReportMessage(MessageLocation* loc, Handle<Object> message);
+  static void ReportMessage(Isolate* isolate,
+                            MessageLocation* loc,
+                            Handle<Object> message);
 
   static void DefaultMessageReport(const MessageLocation* loc,
                                    Handle<Object> message_obj);
index de0d92d..7a1ce1b 100644 (file)
@@ -72,6 +72,7 @@ void ThreadLocalTop::Initialize() {
   int id = Isolate::Current()->thread_manager()->CurrentId();
   thread_id_ = (id == 0) ? ThreadManager::kInvalidId : id;
   external_caught_exception_ = false;
+  in_exception_reporting_ = false;
   failed_access_check_callback_ = NULL;
   save_context_ = NULL;
   catcher_ = NULL;
@@ -794,55 +795,38 @@ bool Isolate::IsExternallyCaught() {
 
 void Isolate::ReportPendingMessages() {
   ASSERT(has_pending_exception());
+  PropagatePendingExceptionToExternalTryCatch();
+
   // If the pending exception is OutOfMemoryException set out_of_memory in
   // the global context.  Note: We have to mark the global context here
   // since the GenerateThrowOutOfMemory stub cannot make a RuntimeCall to
   // set it.
-  bool external_caught = IsExternallyCaught();
-  thread_local_top()->external_caught_exception_ = external_caught;
-  HandleScope scope(this);
-  if (thread_local_top()->pending_exception_ ==
-      Failure::OutOfMemoryException()) {
+  HandleScope scope;
+  if (thread_local_top_.pending_exception_ == Failure::OutOfMemoryException()) {
     context()->mark_out_of_memory();
-  } else if (thread_local_top()->pending_exception_ ==
-             heap_.termination_exception()) {
-    if (external_caught) {
-      try_catch_handler()->can_continue_ = false;
-      try_catch_handler()->exception_ = heap_.null_value();
-    }
+  } else if (thread_local_top_.pending_exception_ ==
+             heap()->termination_exception()) {
+    // Do nothing: if needed, the exception has been already propagated to
+    // v8::TryCatch.
   } else {
-    // At this point all non-object (failure) exceptions have
-    // been dealt with so this shouldn't fail.
-    Object* pending_exception_object = pending_exception()->ToObjectUnchecked();
-    Handle<Object> exception(pending_exception_object);
-    thread_local_top()->external_caught_exception_ = false;
-    if (external_caught) {
-      try_catch_handler()->can_continue_ = true;
-      try_catch_handler()->exception_ = thread_local_top()->pending_exception_;
-      if (!thread_local_top()->pending_message_obj_->IsTheHole()) {
-        try_catch_handler()->message_ =
-            thread_local_top()->pending_message_obj_;
-      }
-    }
-    if (thread_local_top()->has_pending_message_) {
-      thread_local_top()->has_pending_message_ = false;
-      if (thread_local_top()->pending_message_ != NULL) {
-        MessageHandler::ReportMessage(thread_local_top()->pending_message_);
-      } else if (!thread_local_top()->pending_message_obj_->IsTheHole()) {
-        Handle<Object> message_obj(thread_local_top()->pending_message_obj_);
-        if (thread_local_top()->pending_message_script_ != NULL) {
-          Handle<Script> script(thread_local_top()->pending_message_script_);
-          int start_pos = thread_local_top()->pending_message_start_pos_;
-          int end_pos = thread_local_top()->pending_message_end_pos_;
+    if (thread_local_top_.has_pending_message_) {
+      thread_local_top_.has_pending_message_ = false;
+      if (thread_local_top_.pending_message_ != NULL) {
+        MessageHandler::ReportMessage(thread_local_top_.pending_message_);
+      } else if (!thread_local_top_.pending_message_obj_->IsTheHole()) {
+        HandleScope scope;
+        Handle<Object> message_obj(thread_local_top_.pending_message_obj_);
+        if (thread_local_top_.pending_message_script_ != NULL) {
+          Handle<Script> script(thread_local_top_.pending_message_script_);
+          int start_pos = thread_local_top_.pending_message_start_pos_;
+          int end_pos = thread_local_top_.pending_message_end_pos_;
           MessageLocation location(script, start_pos, end_pos);
-          MessageHandler::ReportMessage(&location, message_obj);
+          MessageHandler::ReportMessage(this, &location, message_obj);
         } else {
-          MessageHandler::ReportMessage(NULL, message_obj);
+          MessageHandler::ReportMessage(this, NULL, message_obj);
         }
       }
     }
-    thread_local_top()->external_caught_exception_ = external_caught;
-    set_pending_exception(*exception);
   }
   clear_pending_message();
 }
@@ -854,6 +838,9 @@ void Isolate::TraceException(bool flag) {
 
 
 bool Isolate::OptionalRescheduleException(bool is_bottom_call) {
+  ASSERT(has_pending_exception());
+  PropagatePendingExceptionToExternalTryCatch();
+
   // Allways reschedule out of memory exceptions.
   if (!is_out_of_memory()) {
     bool is_termination_exception =
@@ -966,7 +953,7 @@ char* Isolate::RestoreThread(char* from) {
   memcpy(reinterpret_cast<char*>(thread_local_top()), from,
          sizeof(ThreadLocalTop));
   // This might be just paranoia, but it seems to be needed in case a
-  // thread_local_ is restored on a separate OS thread.
+  // thread_local_top_ is restored on a separate OS thread.
 #ifdef USE_SIMULATOR
 #ifdef V8_TARGET_ARCH_ARM
   thread_local_top()->simulator_ = Simulator::current(this);
index b7f1f61..cf982af 100644 (file)
@@ -50,18 +50,26 @@ static bool IsNaN(double x) {
 }
 
 using ::v8::AccessorInfo;
+using ::v8::Arguments;
 using ::v8::Context;
 using ::v8::Extension;
 using ::v8::Function;
+using ::v8::FunctionTemplate;
+using ::v8::Handle;
 using ::v8::HandleScope;
 using ::v8::Local;
+using ::v8::Message;
+using ::v8::MessageCallback;
 using ::v8::Object;
 using ::v8::ObjectTemplate;
 using ::v8::Persistent;
 using ::v8::Script;
+using ::v8::StackTrace;
 using ::v8::String;
-using ::v8::Value;
+using ::v8::TryCatch;
+using ::v8::Undefined;
 using ::v8::V8;
+using ::v8::Value;
 
 namespace i = ::i;
 
@@ -8574,6 +8582,128 @@ THREADED_TEST(NamedPropertyHandlerGetterAttributes) {
 }
 
 
+static Handle<Value> ThrowingGetter(Local<String> name,
+                                    const AccessorInfo& info) {
+  ApiTestFuzzer::Fuzz();
+  ThrowException(Handle<Value>());
+  return Undefined();
+}
+
+
+THREADED_TEST(VariousGetPropertiesAndThrowingCallbacks) {
+  HandleScope scope;
+  LocalContext context;
+
+  Local<FunctionTemplate> templ = FunctionTemplate::New();
+  Local<ObjectTemplate> instance_templ = templ->InstanceTemplate();
+  instance_templ->SetAccessor(v8_str("f"), ThrowingGetter);
+
+  Local<Object> instance = templ->GetFunction()->NewInstance();
+
+  Local<Object> another = Object::New();
+  another->SetPrototype(instance);
+
+  Local<Object> with_js_getter = CompileRun(
+      "o = {};\n"
+      "o.__defineGetter__('f', function() { throw undefined; });\n"
+      "o\n").As<Object>();
+  CHECK(!with_js_getter.IsEmpty());
+
+  TryCatch try_catch;
+
+  Local<Value> result = instance->GetRealNamedProperty(v8_str("f"));
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+  CHECK(result.IsEmpty());
+
+  result = another->GetRealNamedProperty(v8_str("f"));
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+  CHECK(result.IsEmpty());
+
+  result = another->GetRealNamedPropertyInPrototypeChain(v8_str("f"));
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+  CHECK(result.IsEmpty());
+
+  result = another->Get(v8_str("f"));
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+  CHECK(result.IsEmpty());
+
+  result = with_js_getter->GetRealNamedProperty(v8_str("f"));
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+  CHECK(result.IsEmpty());
+
+  result = with_js_getter->Get(v8_str("f"));
+  CHECK(try_catch.HasCaught());
+  try_catch.Reset();
+  CHECK(result.IsEmpty());
+}
+
+
+static Handle<Value> ThrowingCallbackWithTryCatch(const Arguments& args) {
+  TryCatch try_catch;
+  // Verboseness is important: it triggers message delivery which can call into
+  // external code.
+  try_catch.SetVerbose(true);
+  CompileRun("throw 'from JS';");
+  CHECK(try_catch.HasCaught());
+  CHECK(!i::Isolate::Current()->has_pending_exception());
+  CHECK(!i::Isolate::Current()->has_scheduled_exception());
+  return Undefined();
+}
+
+
+static void WithTryCatch(Handle<Message> message, Handle<Value> data) {
+  TryCatch try_catch;
+}
+
+
+static void ThrowFromJS(Handle<Message> message, Handle<Value> data) {
+  CompileRun("throw 'ThrowInJS';");
+}
+
+
+static void ThrowViaApi(Handle<Message> message, Handle<Value> data) {
+  ThrowException(v8_str("ThrowViaApi"));
+}
+
+
+static void WebKitLike(Handle<Message> message, Handle<Value> data) {
+  Handle<String> errorMessageString = message->Get();
+  CHECK(!errorMessageString.IsEmpty());
+  message->GetStackTrace();
+  message->GetScriptResourceName();
+}
+
+THREADED_TEST(ExceptionsDoNotPropagatePastTryCatch) {
+  HandleScope scope;
+  LocalContext context;
+
+  Local<Function> func =
+      FunctionTemplate::New(ThrowingCallbackWithTryCatch)->GetFunction();
+  context->Global()->Set(v8_str("func"), func);
+
+  MessageCallback callbacks[] =
+      { NULL, WebKitLike, ThrowViaApi, ThrowFromJS, WithTryCatch };
+  for (unsigned i = 0; i < sizeof(callbacks)/sizeof(callbacks[0]); i++) {
+    MessageCallback callback = callbacks[i];
+    if (callback != NULL) {
+      V8::AddMessageListener(callback);
+    }
+    ExpectFalse(
+        "var thrown = false;\n"
+        "try { func(); } catch(e) { thrown = true; }\n"
+        "thrown\n");
+    if (callback != NULL) {
+      V8::RemoveMessageListeners(callback);
+    }
+  }
+}
+
+
 static v8::Handle<Value> ParentGetter(Local<String> name,
                                       const AccessorInfo& info) {
   ApiTestFuzzer::Fuzz();