Bug 744842 - don't include actual args in error.stack.toString (r=dmandelin)
authorLuke Wagner <luke@mozilla.com>
Wed, 11 Apr 2012 18:07:44 -0700
changeset 91548 c7d6c4305f9397296eb0e97e99b6ea9aa8c9dc6b
parent 91547 dd28759eba7b1d75772c2ea69326347b5be4ec4c
child 91549 1ae36c75ffb30796c21d4a3b822bbba55653681a
push id8271
push userlwagner@mozilla.com
push dateThu, 12 Apr 2012 23:28:15 +0000
treeherdermozilla-inbound@c7d6c4305f93 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdmandelin
bugs744842
milestone14.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 744842 - don't include actual args in error.stack.toString (r=dmandelin)
js/src/jsexn.cpp
js/src/tests/js1_5/Exceptions/errstack-001.js
js/xpconnect/tests/chrome/test_exnstack.xul
js/xpconnect/tests/mochitest/test_bug390488.html
services/common/tests/unit/test_utils_stackTrace.js
--- a/js/src/jsexn.cpp
+++ b/js/src/jsexn.cpp
@@ -107,36 +107,37 @@ Class js::ErrorClass = {
     NULL,                 /* checkAccess */
     NULL,                 /* call        */
     NULL,                 /* construct   */
     NULL,                 /* hasInstance */
     exn_trace
 };
 
 template <typename T>
-struct JSStackTraceElemImpl {
+struct JSStackTraceElemImpl
+{
     T                   funName;
-    size_t              argc;
     const char          *filename;
     unsigned            ulineno;
 };
 
 typedef JSStackTraceElemImpl<HeapPtrString> JSStackTraceElem;
 typedef JSStackTraceElemImpl<JSString *>    JSStackTraceStackElem;
 
-typedef struct JSExnPrivate {
+struct JSExnPrivate
+{
     /* A copy of the JSErrorReport originally generated. */
     JSErrorReport       *errorReport;
     js::HeapPtrString   message;
     js::HeapPtrString   filename;
     unsigned            lineno;
     size_t              stackDepth;
     int                 exnType;
     JSStackTraceElem    stackElems[1];
-} JSExnPrivate;
+};
 
 static JSString *
 StackTraceToString(JSContext *cx, JSExnPrivate *priv);
 
 static JSErrorReport *
 CopyErrorReport(JSContext *cx, JSErrorReport *report)
 {
     /*
@@ -252,30 +253,16 @@ CopyErrorReport(JSContext *cx, JSErrorRe
 
     /* Note that this is before it gets flagged with JSREPORT_EXCEPTION */
     copy->flags = report->flags;
 
 #undef JS_CHARS_SIZE
     return copy;
 }
 
-static HeapValue *
-GetStackTraceValueBuffer(JSExnPrivate *priv)
-{
-    /*
-     * We use extra memory after JSExnPrivateInfo.stackElems to store jsvals
-     * that helps to produce more informative stack traces. The following
-     * assert allows us to assume that no gap after stackElems is necessary to
-     * align the buffer properly.
-     */
-    JS_STATIC_ASSERT(sizeof(JSStackTraceElem) % sizeof(jsval) == 0);
-
-    return reinterpret_cast<HeapValue *>(priv->stackElems + priv->stackDepth);
-}
-
 struct SuppressErrorsGuard
 {
     JSContext *cx;
     JSErrorReporter prevReporter;
     JSExceptionState *prevState;
 
     SuppressErrorsGuard(JSContext *cx)
       : cx(cx),
@@ -285,117 +272,69 @@ struct SuppressErrorsGuard
 
     ~SuppressErrorsGuard()
     {
         JS_RestoreExceptionState(cx, prevState);
         JS_SetErrorReporter(cx, prevReporter);
     }
 };
 
-struct AppendWrappedArg {
-    JSContext *cx;
-    AutoValueVector &values;
-    AppendWrappedArg(JSContext *cx, AutoValueVector &values)
-      : cx(cx),
-        values(values)
-    {}
-
-    bool operator()(unsigned, Value *vp) {
-        Value v = *vp;
-
-        /*
-         * Try to wrap.
-         *
-         * If wrap() fails, there's a good chance that it's because we're
-         * already in the process of throwing a native stack limit exception.
-         *
-         * This causes wrap() to throw, but it can't actually create an exception
-         * because we're already making one here, and cx->generatingError is true.
-         * So it returns false without an exception set on the stack. If we propagate
-         * that, it constitutes an uncatchable exception.
-         *
-         * So we just ignore exceptions. If wrap actually does set a pending
-         * exception, or if the caller sloppily left an exception on cx (which the
-         * e4x parser does), it doesn't matter - it will be overwritten shortly.
-         *
-         * NB: In the sloppy e4x case, one might thing we should clear the
-         * exception before calling wrap(). But wrap() has to be ok with pending
-         * exceptions, since it wraps exception objects during cross-compartment
-         * unwinding.
-         */
-        if (!cx->compartment->wrap(cx, &v))
-            v = JSVAL_VOID;
-
-        /* Append the value. */
-        return values.append(v);
-    }
-};
-
 static void
 SetExnPrivate(JSContext *cx, JSObject *exnObject, JSExnPrivate *priv);
 
 static bool
 InitExnPrivate(JSContext *cx, HandleObject exnObject, HandleString message,
                HandleString filename, unsigned lineno, JSErrorReport *report, int exnType)
 {
     JS_ASSERT(exnObject->isError());
     JS_ASSERT(!exnObject->getPrivate());
 
     JSCheckAccessOp checkAccess = cx->runtime->securityCallbacks->checkObjectAccess;
 
     Vector<JSStackTraceStackElem> frames(cx);
-    AutoValueVector values(cx);
     {
         SuppressErrorsGuard seg(cx);
         for (FrameRegsIter i(cx); !i.done(); ++i) {
             StackFrame *fp = i.fp();
 
             /*
-             * Ask the crystal CAPS ball whether we can see values across
-             * compartment boundaries.
-             *
-             * NB: 'fp' may point to cross-compartment values that require wrapping.
+             * Ask the crystal CAPS ball whether we can see across compartments.
+             * NB: this means 'fp' may point to cross-compartment frames.
              */
             if (checkAccess && fp->isNonEvalFunctionFrame()) {
                 Value v = NullValue();
                 jsid callerid = ATOM_TO_JSID(cx->runtime->atomState.callerAtom);
                 if (!checkAccess(cx, &fp->callee(), callerid, JSACC_READ, &v))
                     break;
             }
 
             if (!frames.growBy(1))
                 return false;
             JSStackTraceStackElem &frame = frames.back();
-            if (fp->isNonEvalFunctionFrame()) {
+            if (fp->isNonEvalFunctionFrame())
                 frame.funName = fp->fun()->atom ? fp->fun()->atom : cx->runtime->emptyString;
-                frame.argc = fp->numActualArgs();
-                if (!fp->forEachCanonicalActualArg(AppendWrappedArg(cx, values)))
-                    return false;
-            } else {
+            else
                 frame.funName = NULL;
-                frame.argc = 0;
-            }
             if (fp->isScriptFrame()) {
                 frame.filename = SaveScriptFilename(cx, fp->script()->filename);
                 if (!frame.filename)
                     return false;
                 frame.ulineno = PCToLineNumber(fp->script(), i.pc());
             } else {
                 frame.ulineno = 0;
                 frame.filename = NULL;
             }
         }
     }
 
     /* Do not need overflow check: the vm stack is already bigger. */
     JS_STATIC_ASSERT(sizeof(JSStackTraceElem) <= sizeof(StackFrame));
 
     size_t nbytes = offsetof(JSExnPrivate, stackElems) +
-                    frames.length() * sizeof(JSStackTraceElem) +
-                    values.length() * sizeof(HeapValue);
+                    frames.length() * sizeof(JSStackTraceElem);
 
     JSExnPrivate *priv = (JSExnPrivate *)cx->malloc_(nbytes);
     if (!priv)
         return false;
 
     /* Initialize to zero so that write barriers don't witness undefined values. */
     memset(priv, 0, nbytes);
 
@@ -415,67 +354,49 @@ InitExnPrivate(JSContext *cx, HandleObje
         priv->errorReport = NULL;
     }
 
     priv->message.init(message);
     priv->filename.init(filename);
     priv->lineno = lineno;
     priv->stackDepth = frames.length();
     priv->exnType = exnType;
-
-    JSStackTraceElem *framesDest = priv->stackElems;
-    HeapValue *valuesDest = reinterpret_cast<HeapValue *>(framesDest + frames.length());
-    JS_ASSERT(valuesDest == GetStackTraceValueBuffer(priv));
-
     for (size_t i = 0; i < frames.length(); ++i) {
-        framesDest[i].funName.init(frames[i].funName);
-        framesDest[i].argc = frames[i].argc;
-        framesDest[i].filename = frames[i].filename;
-        framesDest[i].ulineno = frames[i].ulineno;
+        priv->stackElems[i].funName.init(frames[i].funName);
+        priv->stackElems[i].filename = frames[i].filename;
+        priv->stackElems[i].ulineno = frames[i].ulineno;
     }
-    for (size_t i = 0; i < values.length(); ++i)
-        valuesDest[i].init(cx->compartment, values[i]);
 
     SetExnPrivate(cx, exnObject, priv);
     return true;
 }
 
 static inline JSExnPrivate *
 GetExnPrivate(JSObject *obj)
 {
     JS_ASSERT(obj->isError());
     return (JSExnPrivate *) obj->getPrivate();
 }
 
 static void
 exn_trace(JSTracer *trc, JSObject *obj)
 {
-    JSExnPrivate *priv;
-    JSStackTraceElem *elem;
-    size_t vcount, i;
-    HeapValue *vp;
-
-    priv = GetExnPrivate(obj);
-    if (priv) {
+    if (JSExnPrivate *priv = GetExnPrivate(obj)) {
         if (priv->message)
             MarkString(trc, &priv->message, "exception message");
         if (priv->filename)
             MarkString(trc, &priv->filename, "exception filename");
 
-        elem = priv->stackElems;
-        for (vcount = i = 0; i != priv->stackDepth; ++i, ++elem) {
-            if (elem->funName)
-                MarkString(trc, &elem->funName, "stack trace function name");
-            if (IS_GC_MARKING_TRACER(trc) && elem->filename)
-                MarkScriptFilename(elem->filename);
-            vcount += elem->argc;
+        for (size_t i = 0; i != priv->stackDepth; ++i) {
+            JSStackTraceElem &elem = priv->stackElems[i];
+            if (elem.funName)
+                MarkString(trc, &elem.funName, "stack trace function name");
+            if (IS_GC_MARKING_TRACER(trc) && elem.filename)
+                MarkScriptFilename(elem.filename);
         }
-        vp = GetStackTraceValueBuffer(priv);
-        for (i = 0; i != vcount; ++i, ++vp)
-            MarkValue(trc, vp, "stack trace argument");
     }
 }
 
 /* NB: An error object's private must be set through this function. */
 static void
 SetExnPrivate(JSContext *cx, JSObject *exnObject, JSExnPrivate *priv)
 {
     JS_ASSERT(!exnObject->getPrivate());
@@ -587,67 +508,21 @@ js_ErrorFromException(JSContext *cx, jsv
         return NULL;
     priv = GetExnPrivate(obj);
     if (!priv)
         return NULL;
     return priv->errorReport;
 }
 
 static JSString *
-ValueToShortSource(JSContext *cx, const Value &v)
-{
-    JSString *str;
-
-    /* Avoid toSource bloat and fallibility for object types. */
-    if (!v.isObject())
-        return js_ValueToSource(cx, v);
-
-    JSObject *obj = &v.toObject();
-    AutoCompartment ac(cx, obj);
-    if (!ac.enter())
-        return NULL;
-
-    if (obj->isFunction()) {
-        /*
-         * XXX Avoid function decompilation bloat for now.
-         */
-        str = JS_GetFunctionId(obj->toFunction());
-        if (!str && !(str = js_ValueToSource(cx, v))) {
-            /*
-             * Continue to soldier on if the function couldn't be
-             * converted into a string.
-             */
-            JS_ClearPendingException(cx);
-            str = JS_NewStringCopyZ(cx, "[unknown function]");
-        }
-    } else {
-        /*
-         * XXX Avoid toString on objects, it takes too long and uses too much
-         * memory, for too many classes (see Mozilla bug 166743).
-         */
-        char buf[100];
-        JS_snprintf(buf, sizeof buf, "[object %s]", js::UnwrapObject(obj, false)->getClass()->name);
-        str = JS_NewStringCopyZ(cx, buf);
-    }
-
-    ac.leave();
-
-    if (!str || !cx->compartment->wrap(cx, &str))
-        return NULL;
-    return str;
-}
-
-static JSString *
 StackTraceToString(JSContext *cx, JSExnPrivate *priv)
 {
     jschar *stackbuf;
     size_t stacklen, stackmax;
     JSStackTraceElem *elem, *endElem;
-    HeapValue *values;
-    size_t i;
     JSString *str;
     const char *cp;
     char ulnbuf[11];
 
     /* After this point, failing control flow must goto bad. */
     stackbuf = NULL;
     stacklen = stackmax = 0;
 
@@ -688,32 +563,20 @@ StackTraceToString(JSContext *cx, JSExnP
             if (!ptr_)                                                        \
                 goto bad;                                                     \
             stackbuf = (jschar *) ptr_;                                       \
         }                                                                     \
         js_strncpy(stackbuf + stacklen, chars_, length_);                     \
         stacklen += length_;                                                  \
     JS_END_MACRO
 
-    values = GetStackTraceValueBuffer(priv);
     elem = priv->stackElems;
     for (endElem = elem + priv->stackDepth; elem != endElem; elem++) {
-        if (elem->funName) {
+        if (elem->funName)
             APPEND_STRING_TO_STACK(elem->funName);
-            APPEND_CHAR_TO_STACK('(');
-            for (i = 0; i != elem->argc; i++, values++) {
-                if (i > 0)
-                    APPEND_CHAR_TO_STACK(',');
-                str = ValueToShortSource(cx, *values);
-                if (!str)
-                    goto bad;
-                APPEND_STRING_TO_STACK(str);
-            }
-            APPEND_CHAR_TO_STACK(')');
-        }
         APPEND_CHAR_TO_STACK('@');
         if (elem->filename) {
             for (cp = elem->filename; *cp; cp++)
                 APPEND_CHAR_TO_STACK(*cp);
         }
         APPEND_CHAR_TO_STACK(':');
         JS_snprintf(ulnbuf, sizeof ulnbuf, "%u", elem->ulineno);
         for (cp = ulnbuf; *cp; cp++)
@@ -1378,24 +1241,18 @@ js_ReportUncaughtException(JSContext *cx
 }
 
 extern JSObject *
 js_CopyErrorObject(JSContext *cx, JSObject *errobj, JSObject *scope)
 {
     assertSameCompartment(cx, scope);
     JSExnPrivate *priv = GetExnPrivate(errobj);
 
-    uint32_t stackDepth = priv->stackDepth;
-    size_t valueCount = 0;
-    for (uint32_t i = 0; i < stackDepth; i++)
-        valueCount += priv->stackElems[i].argc;
-
     size_t size = offsetof(JSExnPrivate, stackElems) +
-                  stackDepth * sizeof(JSStackTraceElem) +
-                  valueCount * sizeof(jsval);
+                  priv->stackDepth * sizeof(JSStackTraceElem);
 
     JSExnPrivate *copy = (JSExnPrivate *)cx->malloc_(size);
     if (!copy)
         return NULL;
 
     struct AutoFree {
         JSContext *cx;
         JSExnPrivate *p;
--- a/js/src/tests/js1_5/Exceptions/errstack-001.js
+++ b/js/src/tests/js1_5/Exceptions/errstack-001.js
@@ -103,84 +103,84 @@ function D(x,z)
 myErr = A(44,13);
 stackFrames = getStackFrames(myErr);
 status = inSection(1);
 actual = stackFrames[0].substring(0,1);
 expect = '@';
 addThis();
 
 status = inSection(2);
-actual = stackFrames[1].substring(0,9);
-expect = 'A(44,13)@';
+actual = stackFrames[1].substring(0,2);
+expect = 'A@';
 addThis();
 
 status = inSection(3);
-actual = stackFrames[2].substring(0,9);
-expect = 'B(45,14)@';
+actual = stackFrames[2].substring(0,2);
+expect = 'B@';
 addThis();
 
 status = inSection(4);
-actual = stackFrames[3].substring(0,9);
-expect = 'C(46,15)@';
+actual = stackFrames[3].substring(0,2);
+expect = 'C@';
 addThis();
 
 status = inSection(5);
-actual = stackFrames[4].substring(0,9);
-expect = 'D(47,16)@';
+actual = stackFrames[4].substring(0,2);
+expect = 'D@';
 addThis();
 
 
 
 myErr = A('44:foo','13:bar');
 stackFrames = getStackFrames(myErr);
 status = inSection(6);
 actual = stackFrames[0].substring(0,1);
 expect = '@';
 addThis();
 
 status = inSection(7);
-actual = stackFrames[1].substring(0,21);
-expect = 'A("44:foo","13:bar")@';
+actual = stackFrames[1].substring(0,2);
+expect = 'A@';
 addThis();
 
 status = inSection(8);
-actual = stackFrames[2].substring(0,23);
-expect = 'B("44:foo1","13:bar1")@';
+actual = stackFrames[2].substring(0,2);
+expect = 'B@';
 addThis();
 
 status = inSection(9);
-actual = stackFrames[3].substring(0,25);
-expect = 'C("44:foo11","13:bar11")@';
+actual = stackFrames[3].substring(0,2);
+expect = 'C@';
 addThis();
 
 status = inSection(10);
-actual = stackFrames[4].substring(0,27);
-expect = 'D("44:foo111","13:bar111")@';;
+actual = stackFrames[4].substring(0,2);
+expect = 'D@';;
 addThis();
 
 
 
 /*
  * Make the first frame occur in a function with an empty name -
  */
 myErr = function() { return A(44,13); } ();
 stackFrames = getStackFrames(myErr);
 status = inSection(11);
 actual = stackFrames[0].substring(0,1);
 expect = '@';
 addThis();
 
 status = inSection(12);
-actual = stackFrames[1].substring(0,3);
-expect = '()@';
+actual = stackFrames[1].substring(0,1);
+expect = '@';
 addThis();
 
 status = inSection(13);
-actual = stackFrames[2].substring(0,9);
-expect = 'A(44,13)@';
+actual = stackFrames[2].substring(0,2);
+expect = 'A@';
 addThis();
 
 // etc. for the rest of the frames as above
 
 
 
 /*
  * Make the first frame occur in a function with name 'anonymous' -
@@ -189,23 +189,23 @@ var f = Function('return A(44,13);');
 myErr = f();
 stackFrames = getStackFrames(myErr);
 status = inSection(14);
 actual = stackFrames[0].substring(0,1);
 expect = '@';
 addThis();
 
 status = inSection(15);
-actual = stackFrames[1].substring(0,12);
-expect = 'anonymous()@';
+actual = stackFrames[1].substring(0,10);
+expect = 'anonymous@';
 addThis();
 
 status = inSection(16);
-actual = stackFrames[2].substring(0,9);
-expect = 'A(44,13)@';
+actual = stackFrames[2].substring(0,2);
+expect = 'A@';
 addThis();
 
 // etc. for the rest of the frames as above
 
 
 
 /*
  * Make a user-defined error via the Error() function -
--- a/js/xpconnect/tests/chrome/test_exnstack.xul
+++ b/js/xpconnect/tests/chrome/test_exnstack.xul
@@ -49,17 +49,16 @@ https://bugzilla.mozilla.org/show_bug.cg
 
       stackFrames = e.stack.split("\n");
 
       ok(/throwAsInner/.exec(stackFrames[0]),
          "The bottom frame should be thrown by the inner");
 
       ok(/throwAsOuter/.exec(stackFrames[2]),
          "The 3rd-from-bottom frame should be thrown by the other");
-      ok(/Window/.exec(stackFrames[2]), "Should have a |Window| argument");
 
       ok(!/throwAsChrome/.exec(e.stack),
          "The entire stack should not cross into chrome.");
     }
 
     SimpleTest.finish();
   }
 
--- a/js/xpconnect/tests/mochitest/test_bug390488.html
+++ b/js/xpconnect/tests/mochitest/test_bug390488.html
@@ -47,17 +47,17 @@ https://bugzilla.mozilla.org/show_bug.cg
   function matches(s, p, name) {
     ok(s.match(p) != null, name,
        "got " + uneval(s) + ", expected a string matching " + uneval(p));
   }
 
   function checkForStacks() {
     matches(getStack1(), /checkForStacks .* onclick .* simulateClick/,
             "Stack from walking caller chain should be correct");
-    isnot(getStack2().indexOf("simulateClick()@"),  -1,
+    isnot(getStack2().indexOf("simulateClick@"),  -1,
           "Stack from |new Error().stack| should include simulateClick");
   }
 
   simulateClick();
 </script>
 </pre>
 </body>
 </html>
--- a/services/common/tests/unit/test_utils_stackTrace.js
+++ b/services/common/tests/unit/test_utils_stackTrace.js
@@ -16,18 +16,18 @@ function run_test() {
     foo(0);
   }
   catch(ex) {
     trace = CommonUtils.stackTrace(ex);
   }
   _("Got trace:", trace);
   do_check_neq(trace, "");
 
-  let bazPos = trace.indexOf("baz(2)@test_utils_stackTrace.js:7");
-  let barPos = trace.indexOf("bar(1)@test_utils_stackTrace.js:6");
-  let fooPos = trace.indexOf("foo(0)@test_utils_stackTrace.js:5");
+  let bazPos = trace.indexOf("baz@test_utils_stackTrace.js:7");
+  let barPos = trace.indexOf("bar@test_utils_stackTrace.js:6");
+  let fooPos = trace.indexOf("foo@test_utils_stackTrace.js:5");
   _("String positions:", bazPos, barPos, fooPos);
 
   _("Make sure the desired messages show up");
   do_check_true(bazPos >= 0);
   do_check_true(barPos > bazPos);
   do_check_true(fooPos > barPos);
 }