refactor: add and use libdeno.setGlobalErrorHandler instead of window.onerror

7 files changed, 88 insertions, 56 deletions

diff --git a/js/globals.ts b/js/globals.ts
index 80e00ea15..0364aaf98 100644
--- a/js/globals.ts
+++ b/js/globals.ts
@@ -1,7 +1,6 @@
 // Copyright 2018 the Deno authors. All rights reserved. MIT license.
 
 import { Console } from "./console";
-import { exit } from "./os";
 import * as timers from "./timers";
 import { TextDecoder, TextEncoder } from "./text_encoding";
 import * as fetch_ from "./fetch";
@@ -12,13 +11,6 @@ declare global {
   interface Window {
     console: Console;
     define: Readonly<unknown>;
-    onerror?: (
-      message: string,
-      source: string,
-      lineno: number,
-      colno: number,
-      error: Error
-    ) => void;
   }
 
   const clearTimeout: typeof timers.clearTimer;
@@ -43,30 +35,12 @@ window.window = window;
 
 window.libdeno = null;
 
-// import "./url";
-
 window.setTimeout = timers.setTimeout;
 window.setInterval = timers.setInterval;
 window.clearTimeout = timers.clearTimer;
 window.clearInterval = timers.clearTimer;
 
 window.console = new Console(libdeno.print);
-// Uncaught exceptions are sent to window.onerror by the privileged binding.
-window.onerror = (
-  message: string,
-  source: string,
-  lineno: number,
-  colno: number,
-  error: Error
-) => {
-  // TODO Currently there is a bug in v8_source_maps.ts that causes a
-  // segfault if it is used within window.onerror. To workaround we
-  // uninstall the Error.prepareStackTrace handler. Users will get unmapped
-  // stack traces on uncaught exceptions until this issue is fixed.
-  //Error.prepareStackTrace = null;
-  console.log(error.stack);
-  exit(1);
-};
 window.TextEncoder = TextEncoder;
 window.TextDecoder = TextDecoder;
 
diff --git a/js/libdeno.ts b/js/libdeno.ts
index 83dc8efa0..142d779c2 100644
--- a/js/libdeno.ts
+++ b/js/libdeno.ts
@@ -10,6 +10,16 @@ interface Libdeno {
 
   print(x: string): void;
 
+  setGlobalErrorHandler: (
+    handler: (
+      message: string,
+      source: string,
+      line: number,
+      col: number,
+      error: Error
+    ) => void
+  ) => void;
+
   mainSource: string;
   mainSourceMap: RawSourceMap;
 }
diff --git a/js/main.ts b/js/main.ts
index eb90abb0e..51b5790a2 100644
--- a/js/main.ts
+++ b/js/main.ts
@@ -43,9 +43,21 @@ function onMessage(ui8: Uint8Array) {
   }
 }
 
+function onGlobalError(
+  message: string,
+  source: string,
+  lineno: number,
+  colno: number,
+  error: Error
+) {
+  console.log(error.stack);
+  os.exit(1);
+}
+
 /* tslint:disable-next-line:no-default-export */
 export default function denoMain() {
   libdeno.recv(onMessage);
+  libdeno.setGlobalErrorHandler(onGlobalError);
   const compiler = DenoCompiler.instance();
 
   // First we send an empty "Start" message to let the privlaged side know we
diff --git a/libdeno/binding.cc b/libdeno/binding.cc
index 0a107c95f..583b22fbb 100644
--- a/libdeno/binding.cc
+++ b/libdeno/binding.cc
@@ -13,8 +13,6 @@
 
 namespace deno {
 
-static bool skip_onerror = false;
-
 Deno* FromIsolate(v8::Isolate* isolate) {
   return static_cast<Deno*>(isolate->GetData(0));
 }
@@ -34,36 +32,37 @@ void HandleExceptionStr(v8::Local<v8::Context> context,
                         v8::Local<v8::Value> exception,
                         std::string* exception_str) {
   auto* isolate = context->GetIsolate();
+  Deno* d = FromIsolate(isolate);
+
   v8::HandleScope handle_scope(isolate);
   v8::Context::Scope context_scope(context);
 
   auto message = v8::Exception::CreateMessage(isolate, exception);
-  auto onerrorStr = v8::String::NewFromUtf8(isolate, "onerror");
-  auto onerror = context->Global()->Get(onerrorStr);
   auto stack_trace = message->GetStackTrace();
   auto line =
       v8::Integer::New(isolate, message->GetLineNumber(context).FromJust());
   auto column =
       v8::Integer::New(isolate, message->GetStartColumn(context).FromJust());
 
-  if (skip_onerror == false) {
-    if (onerror->IsFunction()) {
-      // window.onerror is set so we try to handle the exception in javascript.
-      auto func = v8::Local<v8::Function>::Cast(onerror);
-      v8::Local<v8::Value> args[5];
-      args[0] = exception->ToString();
-      args[1] = message->GetScriptResourceName();
-      args[2] = line;
-      args[3] = column;
-      args[4] = exception;
-      func->Call(context->Global(), 5, args);
-      /* message, source, lineno, colno, error */
-    }
+  auto global_error_handler = d->global_error_handler.Get(isolate);
+
+  if (!global_error_handler.IsEmpty()) {
+    // global_error_handler is set so we try to handle the exception in javascript.
+    v8::Local<v8::Value> args[5];
+    args[0] = exception->ToString();
+    args[1] = message->GetScriptResourceName();
+    args[2] = line;
+    args[3] = column;
+    args[4] = exception;
+    global_error_handler->Call(context->Global(), 5, args);
+    /* message, source, lineno, colno, error */
+
+    return;
   }
 
   char buf[12 * 1024];
   if (!stack_trace.IsEmpty()) {
-    // No javascript onerror handler, but we do have a stack trace. Format it
+    // No javascript error handler, but we do have a stack trace. Format it
     // into a string and add to last_exception.
     std::string msg;
     v8::String::Utf8Value exceptionStr(isolate, exception);
@@ -80,7 +79,7 @@ void HandleExceptionStr(v8::Local<v8::Context> context,
     }
     *exception_str += msg;
   } else {
-    // No javascript onerror handler, no stack trace. Format the little info we
+    // No javascript error handler, no stack trace. Format the little info we
     // have into a string and add to last_exception.
     v8::String::Utf8Value exceptionStr(isolate, exception);
     v8::String::Utf8Value script_name(isolate,
@@ -188,7 +187,7 @@ void Recv(const v8::FunctionCallbackInfo<v8::Value>& args) {
   v8::HandleScope handle_scope(isolate);
 
   if (!d->recv.IsEmpty()) {
-    isolate->ThrowException(v8_str("deno.recv already called."));
+    isolate->ThrowException(v8_str("libdeno.recv already called."));
     return;
   }
 
@@ -227,6 +226,27 @@ void Send(const v8::FunctionCallbackInfo<v8::Value>& args) {
   d->currentArgs = nullptr;
 }
 
+// Sets the global error handler.
+void SetGlobalErrorHandler(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  v8::Isolate* isolate = args.GetIsolate();
+  Deno* d = reinterpret_cast<Deno*>(isolate->GetData(0));
+  DCHECK_EQ(d->isolate, isolate);
+
+  v8::HandleScope handle_scope(isolate);
+
+  if (!d->global_error_handler.IsEmpty()) {
+    isolate->ThrowException(v8_str("libdeno.setGlobalErrorHandler already called."));
+    return;
+  }
+
+  v8::Local<v8::Value> v = args[0];
+  CHECK(v->IsFunction());
+  v8::Local<v8::Function> func = v8::Local<v8::Function>::Cast(v);
+
+  d->global_error_handler.Reset(isolate, func);
+}
+
+
 bool ExecuteV8StringSource(v8::Local<v8::Context> context,
                            const char* js_filename,
                            v8::Local<v8::String> source) {
@@ -293,7 +313,10 @@ void InitializeContext(v8::Isolate* isolate, v8::Local<v8::Context> context,
   auto send_val = send_tmpl->GetFunction(context).ToLocalChecked();
   CHECK(deno_val->Set(context, deno::v8_str("send"), send_val).FromJust());
 
-  skip_onerror = true;
+  auto set_global_error_handler_tmpl = v8::FunctionTemplate::New(isolate, SetGlobalErrorHandler);
+  auto set_global_error_handler_val = set_global_error_handler_tmpl->GetFunction(context).ToLocalChecked();
+  CHECK(deno_val->Set(context, deno::v8_str("setGlobalErrorHandler"), set_global_error_handler_val).FromJust());
+
   {
     auto source = deno::v8_str(js_source.c_str());
     CHECK(
@@ -326,7 +349,6 @@ void InitializeContext(v8::Isolate* isolate, v8::Local<v8::Context> context,
                 .FromJust());
     }
   }
-  skip_onerror = false;
 }
 
 void AddIsolate(Deno* d, v8::Isolate* isolate) {
@@ -384,7 +406,7 @@ int deno_send(Deno* d, deno_buf buf) {
 
   auto recv = d->recv.Get(d->isolate);
   if (recv.IsEmpty()) {
-    d->last_exception = "deno.recv has not been called.";
+    d->last_exception = "libdeno.recv has not been called.";
     return 0;
   }
 
diff --git a/libdeno/internal.h b/libdeno/internal.h
index c63ba532a..b1806618b 100644
--- a/libdeno/internal.h
+++ b/libdeno/internal.h
@@ -13,6 +13,7 @@ struct deno_s {
   const v8::FunctionCallbackInfo<v8::Value>* currentArgs;
   std::string last_exception;
   v8::Persistent<v8::Function> recv;
+  v8::Persistent<v8::Function> global_error_handler;
   v8::Persistent<v8::Context> context;
   deno_recv_cb cb;
   void* data;
@@ -28,9 +29,11 @@ struct InternalFieldData {
 void Print(const v8::FunctionCallbackInfo<v8::Value>& args);
 void Recv(const v8::FunctionCallbackInfo<v8::Value>& args);
 void Send(const v8::FunctionCallbackInfo<v8::Value>& args);
+void SetGlobalErrorHandler(const v8::FunctionCallbackInfo<v8::Value>& args);
 static intptr_t external_references[] = {reinterpret_cast<intptr_t>(Print),
                                          reinterpret_cast<intptr_t>(Recv),
-                                         reinterpret_cast<intptr_t>(Send), 0};
+                                         reinterpret_cast<intptr_t>(Send),
+                                         reinterpret_cast<intptr_t>(SetGlobalErrorHandler), 0};
 
 Deno* NewFromSnapshot(void* data, deno_recv_cb cb);
 
diff --git a/libdeno/libdeno_test.cc b/libdeno/libdeno_test.cc
index 4675e4d7b..d3650788f 100644
--- a/libdeno/libdeno_test.cc
+++ b/libdeno/libdeno_test.cc
@@ -176,18 +176,24 @@ TEST(LibDenoTest, SnapshotBug) {
   deno_delete(d);
 }
 
-TEST(LibDenoTest, ErrorHandling) {
+TEST(LibDenoTest, GlobalErrorHandling) {
   static int count = 0;
-  Deno* d = deno_new(nullptr, [](auto deno, auto buf) {
+  Deno* d = deno_new(nullptr, [](auto _, auto buf) {
     count++;
     EXPECT_EQ(static_cast<size_t>(1), buf.data_len);
     EXPECT_EQ(buf.data_ptr[0], 42);
   });
-  EXPECT_FALSE(deno_execute(d, "a.js", "ErrorHandling()"));
+  EXPECT_FALSE(deno_execute(d, "a.js", "GlobalErrorHandling()"));
   EXPECT_EQ(count, 1);
   deno_delete(d);
 }
 
+TEST(LibDenoTest, DoubleGlobalErrorHandlingFails) {
+  Deno* d = deno_new(nullptr, nullptr);
+  EXPECT_FALSE(deno_execute(d, "a.js", "DoubleGlobalErrorHandlingFails()"));
+  deno_delete(d);
+}
+
 TEST(LibDenoTest, SendNullAllocPtr) {
   static int count = 0;
   Deno* d = deno_new(nullptr, [](auto _, auto buf) { count++; });
diff --git a/libdeno/libdeno_test.js b/libdeno/libdeno_test.js
index 10905494c..d51973ef0 100644
--- a/libdeno/libdeno_test.js
+++ b/libdeno/libdeno_test.js
@@ -122,8 +122,8 @@ global.SnapshotBug = () => {
   assert("1,2,3" === String([1, 2, 3]));
 };
 
-global.ErrorHandling = () => {
-  global.onerror = (message, source, line, col, error) => {
+global.GlobalErrorHandling = () => {
+  libdeno.setGlobalErrorHandler((message, source, line, col, error) => {
     libdeno.print(`line ${line} col ${col}`);
     assert("ReferenceError: notdefined is not defined" === message);
     assert(source === "helloworld.js");
@@ -131,10 +131,15 @@ global.ErrorHandling = () => {
     assert(col === 1);
     assert(error instanceof Error);
     libdeno.send(new Uint8Array([42]));
-  };
+  });
   eval("\n\n notdefined()\n//# sourceURL=helloworld.js");
 };
 
+global.DoubleGlobalErrorHandlingFails = () => {
+  libdeno.setGlobalErrorHandler((message, source, line, col, error) => {});
+  libdeno.setGlobalErrorHandler((message, source, line, col, error) => {});
+};
+
 global.SendNullAllocPtr = () => {
   libdeno.recv(msg => {
     assert(msg instanceof Uint8Array);