Bug 735544 - Allow exception stacks to cross compartment boundaries. r=luke
authorBobby Holley <bobbyholley@gmail.com>
Thu, 15 Mar 2012 15:19:52 -0700
changeset 91351 1d61262c243cc16ea84496126fd1a13ecb70aa41
parent 91350 84f9eb64e005f030783e99cd3a5c309238c8af84
child 91352 65605fe6063dee1daef806c5d59500741f683720
push idunknown
push userunknown
push dateunknown
reviewersluke
bugs735544
milestone14.0a1
Bug 735544 - Allow exception stacks to cross compartment boundaries. r=luke
caps/tests/mochitest/test_bug246699.html
js/src/jsexn.cpp
js/xpconnect/tests/chrome/Makefile.in
js/xpconnect/tests/chrome/test_exnstack.xul
js/xpconnect/tests/mochitest/Makefile.in
js/xpconnect/tests/mochitest/file_exnstack.html
--- a/caps/tests/mochitest/test_bug246699.html
+++ b/caps/tests/mochitest/test_bug246699.html
@@ -18,17 +18,17 @@ https://bugzilla.mozilla.org/show_bug.cg
 <script class="testbody" type="text/javascript">
 
 /**
  ** Test for Bug 246699
  ** (should produce stack information for caps errors)
  **/
 function hasStack(e)
 {
-  return e instanceof Error && /inciteCaps/.test(e.stack);
+  return e.constructor.name === "Error" && /inciteCaps/.test(e.stack);
 }
 
 function inciteCaps(f)
 {
     try {
         f();
         return "operation succeeded";
     } catch (e if hasStack(e)) {
--- a/js/src/jsexn.cpp
+++ b/js/src/jsexn.cpp
@@ -285,21 +285,52 @@ struct SuppressErrorsGuard
 
     ~SuppressErrorsGuard()
     {
         JS_RestoreExceptionState(cx, prevState);
         JS_SetErrorReporter(cx, prevReporter);
     }
 };
 
-struct AppendArg {
+struct AppendWrappedArg {
+    JSContext *cx;
     Vector<Value> &values;
-    AppendArg(Vector<Value> &values) : values(values) {}
+    AppendWrappedArg(JSContext *cx, Vector<Value> &values)
+      : cx(cx),
+        values(values)
+    {}
+
     bool operator()(unsigned, Value *vp) {
-        return values.append(*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, JSObject *exnObject, JSString *message,
@@ -310,40 +341,38 @@ InitExnPrivate(JSContext *cx, JSObject *
 
     JSCheckAccessOp checkAccess = cx->runtime->securityCallbacks->checkObjectAccess;
 
     Vector<JSStackTraceStackElem> frames(cx);
     Vector<Value> values(cx);
     {
         SuppressErrorsGuard seg(cx);
         for (FrameRegsIter i(cx); !i.done(); ++i) {
+            StackFrame *fp = i.fp();
+
             /*
-             * An exception object stores stack values from 'fp' which may be
-             * in a different compartment from 'exnObject'. Engine compartment
-             * invariants require such values to be wrapped. A simpler solution
-             * is to just cut off the backtrace at compartment boundaries.
-             * Also, avoid exposing values from different security principals.
+             * Ask the crystal CAPS ball whether we can see values across
+             * compartment boundaries.
+             *
+             * NB: 'fp' may point to cross-compartment values that require wrapping.
              */
-            StackFrame *fp = i.fp();
-            if (fp->compartment() != cx->compartment)
-                break;
             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()) {
                 frame.funName = fp->fun()->atom ? fp->fun()->atom : cx->runtime->emptyString;
                 frame.argc = fp->numActualArgs();
-                if (!fp->forEachCanonicalActualArg(AppendArg(values)))
+                if (!fp->forEachCanonicalActualArg(AppendWrappedArg(cx, values)))
                     return false;
             } else {
                 frame.funName = NULL;
                 frame.argc = 0;
             }
             if (fp->isScriptFrame()) {
                 frame.filename = fp->script()->filename;
                 frame.ulineno = PCToLineNumber(fp->script(), i.pc());
@@ -586,17 +615,17 @@ ValueToShortSource(JSContext *cx, const 
             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]", obj->getClass()->name);
+        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;
--- a/js/xpconnect/tests/chrome/Makefile.in
+++ b/js/xpconnect/tests/chrome/Makefile.in
@@ -71,16 +71,17 @@ include $(topsrcdir)/config/rules.mk
 		test_APIExposer.xul \
 		test_bug664689.xul \
 		test_precisegc.xul \
 		test_nodelists.xul \
 		test_getweakmapkeys.xul \
 		test_weakmaps.xul \
 		test_bug706301.xul \
 		test_watchpoints.xul \
+		test_exnstack.xul \
 		$(NULL)
 
 # Disabled until this test gets updated to test the new proxy based
 # wrappers.
 #		test_wrappers-2.xul \
 
 libs:: $(_CHROME_FILES)
 	$(INSTALL) $(foreach f,$^,"$f") $(DEPTH)/_tests/testing/mochitest/chrome/$(relativesrcdir)
new file mode 100644
--- /dev/null
+++ b/js/xpconnect/tests/chrome/test_exnstack.xul
@@ -0,0 +1,69 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
+<?xml-stylesheet type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"?>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=735544
+-->
+<window title="Mozilla Bug 735544"
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
+
+  <!-- test results are displayed in the html:body -->
+  <body xmlns="http://www.w3.org/1999/xhtml">
+  <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=735544"
+     target="_blank">Mozilla Bug 735544</a>
+ <iframe id='ifr0' onload="frameDone(0);" src="http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/file_exnstack.html" />
+ <iframe id='ifr1' onload="frameDone(1);" src="http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/file_exnstack.html" />
+  </body>
+
+  <!-- test code goes here -->
+  <script type="application/javascript">
+  <![CDATA[
+  /** Test for Bug 735544 - Allow exception stacks to cross compartment boundaries **/
+
+  SimpleTest.waitForExplicitFinish();
+
+  var gFramesDone = [false, false];
+  function frameDone(idx) {
+    gFramesDone[idx] = true;
+    if (gFramesDone[0] && gFramesDone[1])
+      startTest();
+  }
+
+  function throwAsChrome() {
+
+    // Grab the iframe content windows.
+    var cwin0 = document.getElementById('ifr0').contentWindow;
+    var cwin1 = document.getElementById('ifr1').contentWindow;
+
+    // Have cwin0 call a function on cwin1 that throws.
+    cwin0.wrappedJSObject.doThrow(cwin1);
+  }
+
+  function startTest() {
+
+    try {
+      throwAsChrome();
+      ok(false, "should throw");
+    } catch (e) {
+
+      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();
+  }
+
+  ]]>
+  </script>
+
+</window>
--- a/js/xpconnect/tests/mochitest/Makefile.in
+++ b/js/xpconnect/tests/mochitest/Makefile.in
@@ -91,16 +91,17 @@ include $(topsrcdir)/config/rules.mk
 		test_bug661980.html \
 		test_bug650273.html \
 		file_bug650273.html \
 		file_bug658560.html \
 		test_bug655297.html \
 		test_bug691059.html \
 		file_nodelists.html \
 		file_bug706301.html \
+		file_exnstack.html \
 		$(NULL)
 
 _CHROME_FILES	= \
 		test_bug361111.xul \
 		$(NULL)
 
 ifneq ($(OS_TARGET),Android)
 ifndef MOZ_PLATFORM_MAEMO
new file mode 100644
--- /dev/null
+++ b/js/xpconnect/tests/mochitest/file_exnstack.html
@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <script type="application/javascript">
+    window.doThrow = function(other) {
+      if (other)
+        throwAsOuter(other);
+      else
+        throwAsInner();
+    }
+
+    function throwAsInner() {
+      throw Error('look at me go!');
+    }
+
+    function throwAsOuter(other) {
+      other.doThrow(null);
+    }
+  </script>
+</head>
+<body>
+</body>
+</html>