Allow uncaught exception messaging in Object.observe callbacks.

This also naturally handles pausing on uncaught exceptions in Object.observe callbacks.

R=adamk@chromium.org, yangguo@chromium.org, yurys@chromium.org
BUG=chromium:335660
LOG=Y

Review URL: https://codereview.chromium.org/692313003

Cr-Commit-Position: refs/heads/master@{#25126}
git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@25126 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/object-observe.js b/src/object-observe.js
index 3af2d51..01ce805 100644 (file)
--- a/src/object-observe.js
+++ b/src/object-observe.js
@@ -567,12 +567,11 @@ function CallbackDeliverPending(callback) {
   if (!IS_NULL(pendingObservers))
     delete pendingObservers[priority];
 
+  // TODO: combine the following runtime calls for perf optimization.
   var delivered = [];
   %MoveArrayContents(callbackInfo, delivered);
+  %DeliverObservationChangeRecords(callback, delivered);
 
-  try {
-    %_CallFunction(UNDEFINED, delivered, callback);
-  } catch (ex) {}  // TODO(rossberg): perhaps log uncaught exceptions.
   return true;
 }
 

diff --git a/src/runtime/runtime-observe.cc b/src/runtime/runtime-observe.cc
index ab7f250..4579136 100644 (file)
--- a/src/runtime/runtime-observe.cc
+++ b/src/runtime/runtime-observe.cc
@@ -52,6 +52,28 @@ RUNTIME_FUNCTION(Runtime_RunMicrotasks) {
 }
 
 
+RUNTIME_FUNCTION(Runtime_DeliverObservationChangeRecords) {
+  HandleScope scope(isolate);
+  DCHECK(args.length() == 2);
+  CONVERT_ARG_HANDLE_CHECKED(JSFunction, callback, 0);
+  CONVERT_ARG_HANDLE_CHECKED(Object, argument, 1);
+  v8::TryCatch catcher;
+  // We should send a message on uncaught exception thrown during
+  // Object.observe delivery while not interrupting further delivery, thus
+  // we make a call inside a verbose TryCatch.
+  catcher.SetVerbose(true);
+  Handle<Object> argv[] = {argument};
+  USE(Execution::Call(isolate, callback, isolate->factory()->undefined_value(),
+                      arraysize(argv), argv));
+  if (isolate->has_pending_exception()) {
+    isolate->ReportPendingMessages();
+    isolate->clear_pending_exception();
+    isolate->set_external_caught_exception(false);
+  }
+  return isolate->heap()->undefined_value();
+}
+
+
 RUNTIME_FUNCTION(Runtime_GetObservationState) {
   SealHandleScope shs(isolate);
   DCHECK(args.length() == 0);

diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h
index 448010a..5d6ccac 100644 (file)
--- a/src/runtime/runtime.h
+++ b/src/runtime/runtime.h
@@ -350,6 +350,7 @@ namespace internal {
   F(GetObjectContextObjectObserve, 1, 1)               \
   F(GetObjectContextObjectGetNotifier, 1, 1)           \
   F(GetObjectContextNotifierPerformChange, 1, 1)       \
+  F(DeliverObservationChangeRecords, 2, 1)             \
                                                        \
   /* Harmony typed arrays */                           \
   F(ArrayBufferInitialize, 2, 1)                       \

diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index 068a07e..5fa2a2f 100644 (file)
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -8810,6 +8810,33 @@ TEST(ApiUncaughtException) {
   v8::V8::RemoveMessageListeners(ApiUncaughtExceptionTestListener);
 }
 
+
+TEST(ApiUncaughtExceptionInObjectObserve) {
+  v8::internal::FLAG_stack_size = 150;
+  report_count = 0;
+  LocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  v8::HandleScope scope(isolate);
+  v8::V8::AddMessageListener(ApiUncaughtExceptionTestListener);
+  CompileRun(
+      "var obj = {};"
+      "var observe_count = 0;"
+      "function observer1() { ++observe_count; };"
+      "function observer2() { ++observe_count; };"
+      "function observer_throws() { throw new Error(); };"
+      "function stack_overflow() { return (function f(x) { f(x+1); })(0); };"
+      "Object.observe(obj, observer_throws.bind());"
+      "Object.observe(obj, observer1);"
+      "Object.observe(obj, stack_overflow);"
+      "Object.observe(obj, observer2);"
+      "Object.observe(obj, observer_throws.bind());"
+      "obj.foo = 'bar';");
+  CHECK_EQ(3, report_count);
+  ExpectInt32("observe_count", 2);
+  v8::V8::RemoveMessageListeners(ApiUncaughtExceptionTestListener);
+}
+
+
 static const char* script_resource_name = "ExceptionInNativeScript.js";
 static void ExceptionInNativeScriptTestListener(v8::Handle<v8::Message> message,
                                                 v8::Handle<Value>) {

diff --git a/test/cctest/test-debug.cc b/test/cctest/test-debug.cc
index 575f938..5d38a16 100644 (file)
--- a/test/cctest/test-debug.cc
+++ b/test/cctest/test-debug.cc
@@ -7594,3 +7594,29 @@ TEST(DebugPromiseRejectedByCallback) {
   CHECK(CompileRun("r")->Equals(v8_str("rejectedrejection")));
   CHECK_EQ(1, exception_event_counter);
 }
+
+
+TEST(DebugBreakOnExceptionInObserveCallback) {
+  DebugLocalContext env;
+  v8::Isolate* isolate = env->GetIsolate();
+  v8::HandleScope scope(isolate);
+  v8::Debug::SetDebugEventListener(&DebugEventCountException);
+  // Break on uncaught exception
+  ChangeBreakOnException(false, true);
+  exception_event_counter = 0;
+
+  v8::Handle<v8::FunctionTemplate> fun =
+      v8::FunctionTemplate::New(isolate, ThrowCallback);
+  env->Global()->Set(v8_str("fun"), fun->GetFunction());
+
+  CompileRun(
+      "var obj = {};"
+      "var callbackRan = false;"
+      "Object.observe(obj, function() {"
+      "   callbackRan = true;"
+      "   throw Error('foo');"
+      "});"
+      "obj.prop = 1");
+  CHECK(CompileRun("callbackRan")->BooleanValue());
+  CHECK_EQ(1, exception_event_counter);
+}

diff --git a/test/cctest/test-object-observe.cc b/test/cctest/test-object-observe.cc
index d208a26..8851d88 100644 (file)
--- a/test/cctest/test-object-observe.cc
+++ b/test/cctest/test-object-observe.cc
@@ -130,6 +130,59 @@ TEST(DeliveryOrdering) {
 }
 
 
+TEST(DeliveryCallbackThrows) {
+  HandleScope scope(CcTest::isolate());
+  LocalContext context(CcTest::isolate());
+  CompileRun(
+      "var obj = {};"
+      "var ordering = [];"
+      "function observer1() { ordering.push(1); };"
+      "function observer2() { ordering.push(2); };"
+      "function observer_throws() {"
+      "  ordering.push(0);"
+      "  throw new Error();"
+      "  ordering.push(-1);"
+      "};"
+      "Object.observe(obj, observer_throws.bind());"
+      "Object.observe(obj, observer1);"
+      "Object.observe(obj, observer_throws.bind());"
+      "Object.observe(obj, observer2);"
+      "Object.observe(obj, observer_throws.bind());"
+      "obj.foo = 'bar';");
+  CHECK_EQ(5, CompileRun("ordering.length")->Int32Value());
+  CHECK_EQ(0, CompileRun("ordering[0]")->Int32Value());
+  CHECK_EQ(1, CompileRun("ordering[1]")->Int32Value());
+  CHECK_EQ(0, CompileRun("ordering[2]")->Int32Value());
+  CHECK_EQ(2, CompileRun("ordering[3]")->Int32Value());
+  CHECK_EQ(0, CompileRun("ordering[4]")->Int32Value());
+}
+
+
+TEST(DeliveryChangesMutationInCallback) {
+  HandleScope scope(CcTest::isolate());
+  LocalContext context(CcTest::isolate());
+  CompileRun(
+      "var obj = {};"
+      "var ordering = [];"
+      "function observer1(records) {"
+      "  ordering.push(100 + records.length);"
+      "  records.push(11);"
+      "  records.push(22);"
+      "};"
+      "function observer2(records) {"
+      "  ordering.push(200 + records.length);"
+      "  records.push(33);"
+      "  records.push(44);"
+      "};"
+      "Object.observe(obj, observer1);"
+      "Object.observe(obj, observer2);"
+      "obj.foo = 'bar';");
+  CHECK_EQ(2, CompileRun("ordering.length")->Int32Value());
+  CHECK_EQ(101, CompileRun("ordering[0]")->Int32Value());
+  CHECK_EQ(201, CompileRun("ordering[1]")->Int32Value());
+}
+
+
 TEST(DeliveryOrderingReentrant) {
   HandleScope scope(CcTest::isolate());
   LocalContext context(CcTest::isolate());