Make exception thrown via v8 public API propagate to v8::TryCatch as JS thrown exceptions do.

src/messages.cc

 
 // Copyright 2006-2008 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
@@ skipping 117 matching lines @@
 
 void MessageHandler::ReportMessage(MessageLocation* loc,
                                    Handle<Object> message) {
   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);
   } else {
+    // We are calling into embedder's code.  Clean pending exception
+    // and ignore scheduled exceptions the callback can throw.
+    Top::clear_pending_exception();

[Mads Ager (chromium), 2011/01/28 11:39:18]

Should this be moved to the caller? I think we only call this if there is
pending exception. In the caller I think we always set the pending_exception
back at the end. So, in the caller, maybe we should clear the pending exception
before calling this?

If not, I think the comment should be extended to state that this is only called
from a place where the pending exception is restored on return?

Ignoring new exceptions while report the current one seems like the right thing
to do.

[antonm, 2011/01/28 13:37:25]

Let me try modified second approach: I'll move all the logic of thread_local_
save and restore over here, does that look reasonable?

On 2011/01/28 11:39:18, Mads Ager wrote:

     for (int i = 0; i < global_length; i++) {
       HandleScope scope;
       if (global_listeners.get(i)->IsUndefined()) continue;
       v8::NeanderObject listener(JSObject::cast(global_listeners.get(i)));
       Handle<Proxy> callback_obj(Proxy::cast(listener.get(0)));
       v8::MessageCallback callback =
           FUNCTION_CAST<v8::MessageCallback>(callback_obj->proxy());
       Handle<Object> callback_data(listener.get(1));
       callback(api_message_obj, v8::Utils::ToLocal(callback_data));
+      if (Top::has_scheduled_exception()) {
+        // Consider logging it somehow.

[Vitaly Repeshko, 2011/01/28 14:03:33]

What if we get an OOM or termination exception here? There are probably not safe
to ignore.

[antonm, 2011/01/28 14:43:18]

That sounds like a very good point.  I think we should be more or less safe for
OOM which will mark the context, but termination exception might be clobbered
here.

But let's investigate and address it later, ok?  Before my change throwing
exception from callbacks would just crash/trigger asserts in v8 anyway.

+        Top::clear_scheduled_exception();
+      }
     }
   }
 }
 
 
 Handle<String> MessageHandler::GetMessage(Handle<Object> data) {
   Handle<String> fmt_str = Factory::LookupAsciiSymbol("FormatMessage");
   Handle<JSFunction> fun =
       Handle<JSFunction>(JSFunction::cast(
           Top::builtins()->GetPropertyNoExceptionThrown(*fmt_str)));
@@ skipping 17 matching lines @@
 }
 
 
 SmartPointer<char> MessageHandler::GetLocalizedMessage(Handle<Object> data) {
   HandleScope scope;
   return GetMessage(data)->ToCString(DISALLOW_NULLS);
 }
 
 
 } }  // namespace v8::internal