Bug 889911 - Switch xpcshell to SystemErrorReporter with a little bit of special magic. r=mrbkap
authorBobby Holley <bobbyholley@gmail.com>
Wed, 31 Jul 2013 10:59:24 -0700
changeset 148164 772aac97a20ccae01d6a50f88d43ebae8877db29
parent 148163 b935f76df94a1792ee56587fbfdd9db32f159578
child 148165 ad0360cc0bcdb9335621e7ca2492ced9e46e1264
push id2697
push userbbajaj@mozilla.com
push dateMon, 05 Aug 2013 18:49:53 +0000
treeherdermozilla-beta@dfec938c7b63 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs889911, 889714
milestone24.0a2
Bug 889911 - Switch xpcshell to SystemErrorReporter with a little bit of special magic. r=mrbkap XPCShell currently overrides all the JSContexts whose creation it observes with its own custom error reporter. This reporter does all sorts of funny things which we try to clean up for the most part. But there are a few very intricate considerations at play. First, the old xpcshell error reporter does some mumbo jumbo with the XPCCallContext stack to try to guess whether some other code might catch the exception. This is total garbage on a number of fronts, particularly because the XPCCallContext stack has no concept of saved frame chains, nested event loops, sandbox boundaries, origin boundaries, or any of the myriad of complicating factors that determine whether or not an exception will propagate. So we get rid of it. But this causes some crazy debugger tests to fail, because they rely on an exception from uriloader/exthandler/nsHandlerService.js getting squelched, and can't handle anybody reporting errors to the console service at the particular moment of contortionism when the exception is raised. So we need to introduce an explicit mechanism to disable the error reporter here to keep things running. Second, we have to be very careful about tracking the return status of the xpcshell binary. The old code would simply flag an error code if the error handler was invoked, and we can mostly continue to do that. But there are some complications. See the comments. Finally, we don't anything analogous in XPCShellEnvironment, because I have patches in bug 889714 to remove its state-dependence on the error reporter. I'll switch it to SystemErrorReporter in that bug.
js/xpconnect/shell/xpcshell.cpp
toolkit/devtools/server/tests/unit/test_sourcemaps-04.js
toolkit/devtools/server/tests/unit/test_sourcemaps-05.js
toolkit/mozapps/installer/precompile_cache.js
--- a/js/xpconnect/shell/xpcshell.cpp
+++ b/js/xpconnect/shell/xpcshell.cpp
@@ -134,16 +134,17 @@ static const char kXPConnectServiceContr
 #define EXITCODE_RUNTIME_ERROR 3
 #define EXITCODE_FILE_NOT_FOUND 4
 
 FILE *gOutFile = NULL;
 FILE *gErrFile = NULL;
 FILE *gInFile = NULL;
 
 int gExitCode = 0;
+bool gIgnoreReportedErrors = false;
 JSBool gQuitting = false;
 static JSBool reportWarnings = true;
 static JSBool compileOnly = false;
 
 JSPrincipals *gJSPrincipals = nullptr;
 nsAutoString *gWorkingDirectory = nullptr;
 
 static JSBool
@@ -261,107 +262,16 @@ GetLine(JSContext *cx, char *bufp, FILE 
         fflush(gOutFile);
         if ((!fgets(line, sizeof line, file) && errno != EINTR) || feof(file))
             return false;
         strcpy(bufp, line);
     }
     return true;
 }
 
-static void
-my_ErrorReporter(JSContext *cx, const char *message, JSErrorReport *report)
-{
-    int i, j, k, n;
-    char *prefix = NULL, *tmp;
-    const char *ctmp;
-    nsCOMPtr<nsIXPConnect> xpc;
-
-    // Don't report an exception from inner JS frames as the callers may intend
-    // to handle it.
-    if (JS_DescribeScriptedCaller(cx, nullptr, nullptr)) {
-        return;
-    }
-
-    // In some cases cx->fp is null here so use XPConnect to tell us about inner
-    // frames.
-    if ((xpc = do_GetService(nsIXPConnect::GetCID()))) {
-        nsAXPCNativeCallContext *cc = nullptr;
-        xpc->GetCurrentNativeCallContext(&cc);
-        if (cc) {
-            nsAXPCNativeCallContext *prev = cc;
-            while (NS_SUCCEEDED(prev->GetPreviousCallContext(&prev)) && prev) {
-                uint16_t lang;
-                if (NS_SUCCEEDED(prev->GetLanguage(&lang)) &&
-                    lang == nsAXPCNativeCallContext::LANG_JS) {
-                    return;
-                }
-            }
-        }
-    }
-
-    if (!report) {
-        fprintf(gErrFile, "%s\n", message);
-        return;
-    }
-
-    /* Conditionally ignore reported warnings. */
-    if (JSREPORT_IS_WARNING(report->flags) && !reportWarnings)
-        return;
-
-    if (report->filename)
-        prefix = JS_smprintf("%s:", report->filename);
-    if (report->lineno) {
-        tmp = prefix;
-        prefix = JS_smprintf("%s%u: ", tmp ? tmp : "", report->lineno);
-        JS_free(cx, tmp);
-    }
-    if (JSREPORT_IS_WARNING(report->flags)) {
-        tmp = prefix;
-        prefix = JS_smprintf("%s%swarning: ",
-                             tmp ? tmp : "",
-                             JSREPORT_IS_STRICT(report->flags) ? "strict " : "");
-        JS_free(cx, tmp);
-    }
-
-    /* embedded newlines -- argh! */
-    while ((ctmp = strchr(message, '\n')) != 0) {
-        ctmp++;
-        if (prefix) fputs(prefix, gErrFile);
-        fwrite(message, 1, ctmp - message, gErrFile);
-        message = ctmp;
-    }
-    /* If there were no filename or lineno, the prefix might be empty */
-    if (prefix)
-        fputs(prefix, gErrFile);
-    fputs(message, gErrFile);
-
-    if (!report->linebuf) {
-        fputc('\n', gErrFile);
-        goto out;
-    }
-
-    fprintf(gErrFile, ":\n%s%s\n%s", prefix, report->linebuf, prefix);
-    n = report->tokenptr - report->linebuf;
-    for (i = j = 0; i < n; i++) {
-        if (report->linebuf[i] == '\t') {
-            for (k = (j + 8) & ~7; j < k; j++) {
-                fputc('.', gErrFile);
-            }
-            continue;
-        }
-        fputc('.', gErrFile);
-        j++;
-    }
-    fputs("^\n", gErrFile);
- out:
-    if (!JSREPORT_IS_WARNING(report->flags))
-        gExitCode = EXITCODE_RUNTIME_ERROR;
-    JS_free(cx, prefix);
-}
-
 static JSBool
 ReadLine(JSContext *cx, unsigned argc, jsval *vp)
 {
     // While 4096 might be quite arbitrary, this is something to be fixed in
     // bug 105707. It is also the same limit as in ProcessFile.
     char buf[4096];
     JSString *str;
 
@@ -513,16 +423,31 @@ Quit(JSContext *cx, unsigned argc, jsval
     gExitCode = 0;
     JS_ConvertArguments(cx, argc, JS_ARGV(cx, vp),"/ i", &gExitCode);
 
     gQuitting = true;
 //    exit(0);
     return false;
 }
 
+// Provide script a way to disable the xpcshell error reporter, preventing
+// reported errors from being logged to the console and also from affecting the
+// exit code returned by the xpcshell binary.
+static JSBool
+IgnoreReportedErrors(JSContext *cx, unsigned argc, jsval *vp)
+{
+    CallArgs args = CallArgsFromVp(argc, vp);
+    if (argc != 1 || !args[0].isBoolean()) {
+        JS_ReportError(cx, "Bad arguments");
+        return false;
+    }
+    gIgnoreReportedErrors = args[0].toBoolean();
+    return true;
+}
+
 static JSBool
 DumpXPC(JSContext *cx, unsigned argc, jsval *vp)
 {
     int32_t depth = 2;
 
     if (argc > 0) {
         if (!JS_ValueToInt32(cx, JS_ARGV(cx, vp)[0], &depth))
             return false;
@@ -887,16 +812,17 @@ File(JSContext *cx, unsigned argc, jsval
   return true;
 }
 
 static const JSFunctionSpec glob_functions[] = {
     JS_FS("print",           Print,          0,0),
     JS_FS("readline",        ReadLine,       1,0),
     JS_FS("load",            Load,           1,0),
     JS_FS("quit",            Quit,           0,0),
+    JS_FS("ignoreReportedErrors", IgnoreReportedErrors, 1,0),
     JS_FS("version",         Version,        1,0),
     JS_FS("build",           BuildDate,      0,0),
     JS_FS("dumpXPC",         DumpXPC,        1,0),
     JS_FS("dump",            Dump,           1,0),
     JS_FS("gc",              GC,             0,0),
 #ifdef JS_GC_ZEAL
     JS_FS("gczeal",          GCZeal,         1,0),
 #endif
@@ -1373,16 +1299,17 @@ ProcessArgs(JSContext *cx, JS::Handle<JS
         }
         default:
             return usage();
         }
     }
 
     if (filename || isInteractive)
         Process(cx, obj, filename, forceTTY);
+
     return gExitCode;
 }
 
 /***************************************************************************/
 
 // #define TEST_InitClassesWithNewWrappedGlobal
 
 #ifdef TEST_InitClassesWithNewWrappedGlobal
@@ -1457,24 +1384,37 @@ nsXPCFunctionThisTranslator::TranslateTh
     return NS_OK;
 }
 
 #endif
 
 // ContextCallback calls are chained
 static JSContextCallback gOldJSContextCallback;
 
+void
+XPCShellErrorReporter(JSContext *cx, const char *message, JSErrorReport *rep)
+{
+    if (gIgnoreReportedErrors)
+        return;
+
+    if (!JSREPORT_IS_WARNING(rep->flags))
+        gExitCode = EXITCODE_RUNTIME_ERROR;
+
+    // Delegate to the system error reporter for heavy lifting.
+    xpc::SystemErrorReporterExternal(cx, message, rep);
+}
+
 static JSBool
 ContextCallback(JSContext *cx, unsigned contextOp)
 {
     if (gOldJSContextCallback && !gOldJSContextCallback(cx, contextOp))
         return false;
 
     if (contextOp == JSCONTEXT_NEW) {
-        JS_SetErrorReporter(cx, my_ErrorReporter);
+        JS_SetErrorReporter(cx, XPCShellErrorReporter);
     }
     return true;
 }
 
 static bool
 GetCurrentWorkingDirectory(nsAString& workingDirectory)
 {
 #if !defined(XP_WIN) && !defined(XP_UNIX)
--- a/toolkit/devtools/server/tests/unit/test_sourcemaps-04.js
+++ b/toolkit/devtools/server/tests/unit/test_sourcemaps-04.js
@@ -6,16 +6,25 @@
  */
 
 var gDebuggee;
 var gClient;
 var gThreadClient;
 
 Components.utils.import('resource:///modules/devtools/SourceMap.jsm');
 
+// Deep in the complicated labyrinth of code that this test invokes, beneath
+// debugger callbacks, sandboxes and nested event loops, lies an exception.
+// This exception lay sleeping since the dawn of time, held captive in a
+// delicate balance of custom xpcshell error reporters and garbage data about
+// the XPCCallContext stack. But bholley dug too greedily, and too deep, and
+// awoke shadow and flame in the darkness of nsExternalHelperAppService.cpp.
+// We must now trust in deep magic to ensure that it does not awaken again.
+ignoreReportedErrors(true);
+
 function run_test()
 {
   initTestDebuggerServer();
   gDebuggee = addTestGlobal("test-source-map");
   gClient = new DebuggerClient(DebuggerServer.connectPipe());
   gClient.connect(function() {
     attachTestTabAndResume(gClient, "test-source-map", function(aResponse, aTabClient, aThreadClient) {
       gThreadClient = aThreadClient;
--- a/toolkit/devtools/server/tests/unit/test_sourcemaps-05.js
+++ b/toolkit/devtools/server/tests/unit/test_sourcemaps-05.js
@@ -6,16 +6,25 @@
  */
 
 var gDebuggee;
 var gClient;
 var gThreadClient;
 
 Components.utils.import('resource:///modules/devtools/SourceMap.jsm');
 
+// Deep in the complicated labyrinth of code that this test invokes, beneath
+// debugger callbacks, sandboxes and nested event loops, lies an exception.
+// This exception lay sleeping since the dawn of time, held captive in a
+// delicate balance of custom xpcshell error reporters and garbage data about
+// the XPCCallContext stack. But bholley dug too greedily, and too deep, and
+// awoke shadow and flame in the darkness of nsExternalHelperAppService.cpp.
+// We must now trust in deep magic to ensure that it does not awaken again.
+ignoreReportedErrors(true);
+
 function run_test()
 {
   initTestDebuggerServer();
   gDebuggee = addTestGlobal("test-source-map");
   gClient = new DebuggerClient(DebuggerServer.connectPipe());
   gClient.connect(function() {
     attachTestTabAndResume(gClient, "test-source-map", function(aResponse, aTabClient, aThreadClient) {
       gThreadClient = aThreadClient;
--- a/toolkit/mozapps/installer/precompile_cache.js
+++ b/toolkit/mozapps/installer/precompile_cache.js
@@ -61,22 +61,32 @@ function get_modules_under(uri) {
            .concat(dir_entries(file.file, "modules", ".jsm"));
   } else {
     throw "Expected a nsIJARURI or nsIFileURL";
   }
 }
 
 function load_modules_under(spec, uri) {
   var entries = get_modules_under(uri);
+  // The precompilation of JS here sometimes reports errors, which we don't
+  // really care about. But if the errors are ever reported to xpcshell's
+  // error reporter, it will cause it to return an error code, which will break
+  // automation. Currently they won't be, because the component loader spins up
+  // its JSContext before xpcshell has time to set its context callback (which
+  // overrides the error reporter on all newly-created JSContexts). But as we
+  // move towards a singled-cxed browser, we'll run into this. So let's be
+  // forward-thinking and deal with it now.
+  ignoreReportedErrors(true);
   for each (let entry in entries) {
     try {
       dump(spec + entry + "\n");
       Cu.import(spec + entry, null);
     } catch(e) {}
   }
+  ignoreReportedErrors(false);
 }
 
 function resolveResource(spec) {
   var uri = Services.io.newURI(spec, null, null);
   return Services.io.newURI(rph.resolveURI(uri), null, null);
 }
 
 function precompile_startupcache(uri) {