Bug 978618. Fix error reporting for unintended XPConnect exceptions thrown by JS-implemented webidl to actually work correctly. r=bholley

dom/base/nsJSUtils.cpp

@@ -105,6 +105,17 @@ nsJSUtils::ReportPendingException(JSContext *aContext)
   if (JS_IsExceptionPending(aContext)) {
     bool saved = JS_SaveFrameChain(aContext);
     {
+      // JS_SaveFrameChain set the compartment of aContext to null, so we need
+      // to enter a compartment.  The question is, which one? We don't want to
+      // enter the original compartment of aContext (or the compartment of the
+      // current exception on aContext, for that matter) because when we
+      // JS_ReportPendingException the JS engine can try to duck-type the
+      // exception and produce a JSErrorReport.  It will then pass that
+      // JSErrorReport to the error reporter on aContext, which might expose
+      // information from it to script via onerror handlers.  So it's very
+      // important that the duck typing happen in the same compartment as the
+      // onerror handler.  In practice, that's the compartment of the window (or
+      // otherwise default global) of aContext, so use that here.
       nsIScriptContext* scx = GetScriptContextFromJSContext(aContext);
       JS::Rooted<JSObject*> scope(aContext);
       scope = scx ? scx->GetWindowProxy()

dom/bindings/CallbackObject.cpp

@@ -207,8 +207,15 @@ CallbackObject::CallSetup::ShouldRethrowException(JS::Handle<JS::Value> aExcepti
 
 CallbackObject::CallSetup::~CallSetup()
 {
-  // First things first: if we have a JSContext, report any pending
-  // errors on it, unless we were told to re-throw them.
+  // To get our nesting right we have to destroy our JSAutoCompartment first.
+  // In particular, we want to do this before we try reporting any exceptions,
+  // so we end up reporting them while in the compartment of our entry point,
+  // not whatever cross-compartment wrappper mCallback might be.
+  // Be careful: the JSAutoCompartment might not have been constructed at all!
+  mAc.destroyIfConstructed();
+
+  // Now, if we have a JSContext, report any pending errors on it, unless we
+  // were told to re-throw them.
   if (mCx) {
     bool dealtWithPendingException = false;
     if ((mCompartment && mExceptionHandling == eRethrowContentExceptions) ||
@@ -231,14 +238,34 @@ CallbackObject::CallSetup::~CallSetup()
       // Either we're supposed to report our exceptions, or we're supposed to
       // re-throw them but we failed to JS_GetPendingException.  Either way,
       // just report the pending exception, if any.
-      nsJSUtils::ReportPendingException(mCx);
+      //
+      // We don't use nsJSUtils::ReportPendingException here because all it
+      // does at this point is JS_SaveFrameChain and enter a compartment around
+      // a JS_ReportPendingException call.  But our mAutoEntryScript should
+      // already do a JS_SaveFrameChain and we are already in the compartment
+      // we want to be in, so all nsJSUtils::ReportPendingException would do is
+      // screw up our compartment, which is exactly what we do not want.
+      //
+      // XXXbz FIXME: bug 979525 means we don't always JS_SaveFrameChain here,
+      // so we need to go ahead and do that.
+      JS::Rooted<JSObject*> oldGlobal(mCx, JS::CurrentGlobalOrNull(mCx));
+      MOZ_ASSERT(oldGlobal, "How can we not have a global here??");
+      bool saved = JS_SaveFrameChain(mCx);
+      // Make sure the JSAutoCompartment goes out of scope before the
+      // JS_RestoreFrameChain call!
+      {
+        JSAutoCompartment ac(mCx, oldGlobal);
+        MOZ_ASSERT(!JS::DescribeScriptedCaller(mCx),
+                   "Our comment above about JS_SaveFrameChain having been "
+                   "called is a lie?");
+        JS_ReportPendingException(mCx);
+      }
+      if (saved) {
+        JS_RestoreFrameChain(mCx);
+      }
     }
   }
 
-  // To get our nesting right we have to destroy our JSAutoCompartment first.
-  // But be careful: it might not have been constructed at all!
-  mAc.destroyIfConstructed();
-
   mAutoIncumbentScript.destroyIfConstructed();
   mAutoEntryScript.destroyIfConstructed();
 

dom/bindings/ErrorResult.h

@@ -78,6 +78,10 @@ public:
   // is being thrown.  Code that would call ReportJSException* or
   // StealJSException as needed must first call WouldReportJSException even if
   // this ErrorResult has not failed.
+  //
+  // The exn argument to ThrowJSException can be in any compartment.  It does
+  // not have to be in the compartment of cx.  If someone later uses it, they
+  // will wrap it into whatever compartment they're working in, as needed.
   void ThrowJSException(JSContext* cx, JS::Handle<JS::Value> exn);
   void ReportJSException(JSContext* cx);
   // Used to implement throwing exceptions from the JS implementation of

js/src/jscntxt.cpp

@@ -1172,6 +1172,8 @@ JSContext::saveFrameChain()
 void
 JSContext::restoreFrameChain()
 {
+    JS_ASSERT(enterCompartmentDepth_ == 0); // We're about to clobber it, and it
+                                            // will be wrong forevermore.
     SavedFrameChain sfc = savedFrameChains_.popCopy();
     setCompartment(sfc.compartment);
     enterCompartmentDepth_ = sfc.enterCompartmentCount;