Bug 1255817 part 1. Make AutoJSAPI always take ownership of error reporting. r=bholley
authorBoris Zbarsky <bzbarsky@mit.edu>
Wed, 23 Mar 2016 11:44:54 -0400
changeset 290073 96449936b41a3bca0b748836269f3baafa155aa1
parent 290072 c3c85d59ec72519765722caf648c4abfe30179ed
child 290074 a45a6cde6558c0ed0797a29818eec13c4e0e2196
push id30114
push usercbook@mozilla.com
push dateThu, 24 Mar 2016 15:15:54 +0000
treeherdermozilla-central@24c5fbde4488 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1255817
milestone48.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 1255817 part 1. Make AutoJSAPI always take ownership of error reporting. r=bholley
dom/base/ScriptSettings.cpp
dom/base/ScriptSettings.h
--- a/dom/base/ScriptSettings.cpp
+++ b/dom/base/ScriptSettings.cpp
@@ -300,41 +300,49 @@ FindJSContext(nsIGlobalObject* aGlobalOb
   if (!cx) {
     cx = nsContentUtils::GetSafeJSContext();
   }
   return cx;
 }
 
 AutoJSAPI::AutoJSAPI()
   : mCx(nullptr)
-  , mOwnErrorReporting(false)
   , mOldAutoJSAPIOwnsErrorReporting(false)
   , mIsMainThread(false) // For lack of anything better
 {
 }
 
 AutoJSAPI::~AutoJSAPI()
 {
-  if (mOwnErrorReporting) {
-    ReportException();
+  if (!mCx) {
+    // No need to do anything here: we never managed to Init, so can't have an
+    // exception on our (nonexistent) JSContext.  We also don't need to restore
+    // any state on it.
+    return;
+  }
 
-    // We need to do this _after_ processing the existing exception, because the
-    // JS engine can throw while doing that, and uses this bit to determine what
-    // to do in that case: squelch the exception if the bit is set, otherwise
-    // call the error reporter. Calling WarningOnlyErrorReporter with a
-    // non-warning will assert, so we need to make sure we do the former.
-    JS::ContextOptionsRef(cx()).setAutoJSAPIOwnsErrorReporting(mOldAutoJSAPIOwnsErrorReporting);
-  }
+  ReportException();
+
+  // We need to do this _after_ processing the existing exception, because the
+  // JS engine can throw while doing that, and uses this bit to determine what
+  // to do in that case: squelch the exception if the bit is set, otherwise
+  // call the error reporter. Calling WarningOnlyErrorReporter with a
+  // non-warning will assert, so we need to make sure we do the former.
+  JS::ContextOptionsRef(cx()).setAutoJSAPIOwnsErrorReporting(mOldAutoJSAPIOwnsErrorReporting);
 
   if (mOldErrorReporter.isSome()) {
     JS_SetErrorReporter(JS_GetRuntime(cx()), mOldErrorReporter.value());
   }
 }
 
 void
+WarningOnlyErrorReporter(JSContext* aCx, const char* aMessage,
+                         JSErrorReport* aRep);
+
+void
 AutoJSAPI::InitInternal(JSObject* aGlobal, JSContext* aCx, bool aIsMainThread)
 {
   MOZ_ASSERT(aCx);
   MOZ_ASSERT(aIsMainThread == NS_IsMainThread());
 #ifdef DEBUG
   bool haveException = JS_IsExceptionPending(aCx);
 #endif // DEBUG
 
@@ -350,19 +358,19 @@ AutoJSAPI::InitInternal(JSObject* aGloba
     mAutoNullableCompartment.emplace(mCx, global);
   } else {
     mAutoNullableCompartment.emplace(mCx, aGlobal);
   }
 
   JSRuntime* rt = JS_GetRuntime(aCx);
   mOldErrorReporter.emplace(JS_GetErrorReporter(rt));
 
-  if (aIsMainThread) {
-    JS_SetErrorReporter(rt, xpc::SystemErrorReporter);
-  }
+  mOldAutoJSAPIOwnsErrorReporting = JS::ContextOptionsRef(aCx).autoJSAPIOwnsErrorReporting();
+  JS::ContextOptionsRef(aCx).setAutoJSAPIOwnsErrorReporting(true);
+  JS_SetErrorReporter(rt, WarningOnlyErrorReporter);
 
 #ifdef DEBUG
   if (haveException) {
     JS::Rooted<JS::Value> exn(aCx);
     JS_GetPendingException(aCx, &exn);
 
     JS_ClearPendingException(aCx);
     if (exn.isObject()) {
@@ -424,18 +432,17 @@ AutoJSAPI::InitInternal(JSObject* aGloba
     MOZ_ASSERT(false, "We had an exception; we should not have");
   }
 #endif // DEBUG
 }
 
 AutoJSAPI::AutoJSAPI(nsIGlobalObject* aGlobalObject,
                      bool aIsMainThread,
                      JSContext* aCx)
-  : mOwnErrorReporting(false)
-  , mOldAutoJSAPIOwnsErrorReporting(false)
+  : mOldAutoJSAPIOwnsErrorReporting(false)
   , mIsMainThread(aIsMainThread)
 {
   MOZ_ASSERT(aGlobalObject);
   MOZ_ASSERT(aGlobalObject->GetGlobalJSObject(), "Must have a JS global");
   MOZ_ASSERT(aCx);
   MOZ_ASSERT(aIsMainThread == NS_IsMainThread());
 
   InitInternal(aGlobalObject->GetGlobalJSObject(), aCx, aIsMainThread);
@@ -542,29 +549,21 @@ WarningOnlyErrorReporter(JSContext* aCx,
   xpcReport->Init(aRep, aMessage, nsContentUtils::IsCallerChrome(),
                   win ? win->AsInner()->WindowID() : 0);
   xpcReport->LogToConsole();
 }
 
 void
 AutoJSAPI::TakeOwnershipOfErrorReporting()
 {
-  MOZ_ASSERT(!mOwnErrorReporting);
-  mOwnErrorReporting = true;
-
-  JSRuntime *rt = JS_GetRuntime(cx());
-  mOldAutoJSAPIOwnsErrorReporting = JS::ContextOptionsRef(cx()).autoJSAPIOwnsErrorReporting();
-  JS::ContextOptionsRef(cx()).setAutoJSAPIOwnsErrorReporting(true);
-  JS_SetErrorReporter(rt, WarningOnlyErrorReporter);
 }
 
 void
 AutoJSAPI::ReportException()
 {
-  MOZ_ASSERT(OwnsErrorReporting(), "This is not our exception to report!");
   if (!HasException()) {
     return;
   }
 
   // AutoJSAPI uses a JSAutoNullableCompartment, and may be in a null
   // compartment when the destructor is called. However, the JS engine
   // requires us to be in a compartment when we fetch the pending exception.
   // In this case, we enter the privileged junk scope and don't dispatch any
--- a/dom/base/ScriptSettings.h
+++ b/dom/base/ScriptSettings.h
@@ -267,17 +267,17 @@ public:
 
   bool CxPusherIsStackTop() const { return mCxPusher->IsStackTop(); }
 
   // We're moving towards a world where the AutoJSAPI always handles
   // exceptions that bubble up from the JS engine. In order to make this
   // process incremental, we allow consumers to opt-in to the new behavior
   // while keeping the old behavior as the default.
   void TakeOwnershipOfErrorReporting();
-  bool OwnsErrorReporting() { return mOwnErrorReporting; }
+  bool OwnsErrorReporting() { return true; }
   // If HasException, report it.  Otherwise, a no-op.  This must be
   // called only if OwnsErrorReporting().
   void ReportException();
 
   bool HasException() const {
     MOZ_ASSERT_IF(NS_IsMainThread(), CxPusherIsStackTop());
     return JS_IsExceptionPending(cx());
   };
@@ -312,17 +312,16 @@ protected:
   AutoJSAPI(nsIGlobalObject* aGlobalObject, bool aIsMainThread, JSContext* aCx);
 
 private:
   mozilla::Maybe<danger::AutoCxPusher> mCxPusher;
   mozilla::Maybe<JSAutoNullableCompartment> mAutoNullableCompartment;
   JSContext *mCx;
 
   // Track state between the old and new error reporting modes.
-  bool mOwnErrorReporting;
   bool mOldAutoJSAPIOwnsErrorReporting;
   // Whether we're mainthread or not; set when we're initialized.
   bool mIsMainThread;
   Maybe<JSErrorReporter> mOldErrorReporter;
 
   void InitInternal(JSObject* aGlobal, JSContext* aCx, bool aIsMainThread);
 
   AutoJSAPI(const AutoJSAPI&) = delete;