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 89539 1d61262c243cc16ea84496126fd1a13ecb70aa41
parent 89538 84f9eb64e005f030783e99cd3a5c309238c8af84
child 89540 65605fe6063dee1daef806c5d59500741f683720
push id22258
push usermak77@bonardo.net
push dateFri, 16 Mar 2012 12:43:42 +0000
treeherdermozilla-central@7f540f758671 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersluke
bugs735544
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 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>