Bug 834732 - Make nsCxPusher.Push(JSContext*) infallible. r=mrbkap
authorBobby Holley <bobbyholley@gmail.com>
Tue, 26 Feb 2013 11:04:11 -0800
changeset 123057 d6766dee457e2b4d59ca65c4da87911caf450b33
parent 123056 471fe31fc325774e0aa26b39ec442e23b5cb1e2e
child 123058 19593a7c78f1a86a829503c1e5665661f54c4d04
push id24372
push useremorley@mozilla.com
push dateWed, 27 Feb 2013 13:22:59 +0000
treeherdermozilla-central@0a91da5f5eab [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs834732
milestone22.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 834732 - Make nsCxPusher.Push(JSContext*) infallible. r=mrbkap We leave the nsIDOMEventTarget* versions fallible for now, but this makes the common case a lot simpler. Note that this means that pushing a null JSContext, a bug, is no longer handled at runtime. But I think we should just assert against it, since there are already callers that don't check the return value.
content/base/public/nsContentUtils.h
content/base/src/nsContentUtils.cpp
content/base/src/nsFrameMessageManager.cpp
content/events/src/nsEventListenerManager.cpp
dom/base/nsDOMClassInfo.cpp
dom/base/nsGlobalWindow.cpp
dom/base/nsJSEnvironment.cpp
dom/bindings/CallbackObject.cpp
dom/sms/src/SmsRequest.cpp
js/xpconnect/src/dictionary_helper_gen.py
layout/base/nsRefreshDriver.cpp
xpfe/appshell/src/nsXULWindow.cpp
--- a/content/base/public/nsContentUtils.h
+++ b/content/base/public/nsContentUtils.h
@@ -2200,27 +2200,27 @@ public:
 
   // Returns false if something erroneous happened.
   bool Push(nsIDOMEventTarget *aCurrentTarget);
   // If nothing has been pushed to stack, this works like Push.
   // Otherwise if context will change, Pop and Push will be called.
   bool RePush(nsIDOMEventTarget *aCurrentTarget);
   // If a null JSContext is passed to Push(), that will cause no
   // push to happen and false to be returned.
-  bool Push(JSContext *cx);
+  void Push(JSContext *cx);
   // Explicitly push a null JSContext on the the stack
-  bool PushNull();
+  void PushNull();
 
   // Pop() will be a no-op if Push() or PushNull() fail
   void Pop();
 
   nsIScriptContext* GetCurrentScriptContext() { return mScx; }
 private:
   // Combined code for PushNull() and Push(JSContext*)
-  bool DoPush(JSContext* cx);
+  void DoPush(JSContext* cx);
 
   nsCOMPtr<nsIScriptContext> mScx;
   bool mScriptIsRunning;
   bool mPushedSomething;
 #ifdef DEBUG
   JSContext* mPushedContext;
   unsigned mCompartmentDepthOnEntry;
 #endif
--- a/content/base/src/nsContentUtils.cpp
+++ b/content/base/src/nsContentUtils.cpp
@@ -3016,17 +3016,18 @@ nsCxPusher::Push(nsIDOMEventTarget *aCur
   }
 
   JSContext* cx = scx ? scx->GetNativeContext() : nullptr;
 
   // If there's no native context in the script context it must be
   // in the process or being torn down. We don't want to notify the
   // script context about scripts having been evaluated in such a
   // case, calling with a null cx is fine in that case.
-  return Push(cx);
+  Push(cx);
+  return true;
 }
 
 bool
 nsCxPusher::RePush(nsIDOMEventTarget *aCurrentTarget)
 {
   if (!mPushedSomething) {
     return Push(aCurrentTarget);
   }
@@ -3047,70 +3048,60 @@ nsCxPusher::RePush(nsIDOMEventTarget *aC
       return true;
     }
   }
 
   Pop();
   return Push(aCurrentTarget);
 }
 
-bool
+void
 nsCxPusher::Push(JSContext *cx)
 {
-  if (mPushedSomething) {
-    NS_ERROR("Whaaa! No double pushing with nsCxPusher::Push()!");
-
-    return false;
-  }
-
-  if (!cx) {
-    return false;
-  }
+  MOZ_ASSERT(!mPushedSomething, "No double pushing with nsCxPusher::Push()!");
+  MOZ_ASSERT(cx);
 
   // Hold a strong ref to the nsIScriptContext, just in case
   // XXXbz do we really need to?  If we don't get one of these in Pop(), is
   // that really a problem?  Or do we need to do this to effectively root |cx|?
   mScx = GetScriptContextFromJSContext(cx);
 
-  return DoPush(cx);
-}
-
-bool
+  DoPush(cx);
+}
+
+void
 nsCxPusher::DoPush(JSContext* cx)
 {
   nsIThreadJSContextStack* stack = nsContentUtils::ThreadJSContextStack();
   if (!stack) {
-    return true;
+    return;
   }
 
   if (cx && IsContextOnStack(stack, cx)) {
     // If the context is on the stack, that means that a script
     // is running at the moment in the context.
     mScriptIsRunning = true;
   }
 
   if (NS_FAILED(stack->Push(cx))) {
-    mScriptIsRunning = false;
-    mScx = nullptr;
-    return false;
+    MOZ_CRASH();
   }
 
   mPushedSomething = true;
 #ifdef DEBUG
   mPushedContext = cx;
   if (cx)
     mCompartmentDepthOnEntry = js::GetEnterCompartmentDepth(cx);
 #endif
-  return true;
-}
-
-bool
+}
+
+void
 nsCxPusher::PushNull()
 {
-  return DoPush(nullptr);
+  DoPush(nullptr);
 }
 
 void
 nsCxPusher::Pop()
 {
   nsIThreadJSContextStack* stack = nsContentUtils::ThreadJSContextStack();
   if (!mPushedSomething || !stack) {
     mScx = nullptr;
@@ -6823,20 +6814,17 @@ AutoJSContext::Init(bool aSafe MOZ_GUARD
   MOZ_GUARD_OBJECT_NOTIFIER_INIT;
 
   if (!aSafe) {
     mCx = nsContentUtils::GetCurrentJSContext();
   }
 
   if (!mCx) {
     mCx = nsContentUtils::GetSafeJSContext();
-    bool result = mPusher.Push(mCx);
-    if (!result || !mCx) {
-      MOZ_CRASH();
-    }
+    mPusher.Push(mCx);
   }
 }
 
 AutoJSContext::operator JSContext*()
 {
   return mCx;
 }
 
--- a/content/base/src/nsFrameMessageManager.cpp
+++ b/content/base/src/nsFrameMessageManager.cpp
@@ -646,17 +646,17 @@ nsFrameMessageManager::ReceiveMessage(ns
           continue;
         }
         JSObject* object = nullptr;
         wrappedJS->GetJSObject(&object);
         if (!object) {
           continue;
         }
         nsCxPusher pusher;
-        NS_ENSURE_STATE(pusher.Push(ctx));
+        pusher.Push(ctx);
 
         JSAutoRequest ar(ctx);
         JSAutoCompartment ac(ctx, object);
 
         // The parameter for the listener function.
         JSObject* param = JS_NewObject(ctx, NULL, NULL, NULL);
         NS_ENSURE_TRUE(param, NS_ERROR_OUT_OF_MEMORY);
 
--- a/content/events/src/nsEventListenerManager.cpp
+++ b/content/events/src/nsEventListenerManager.cpp
@@ -813,18 +813,18 @@ nsEventListenerManager::CompileEventHand
       nsIURI *uri = doc->GetDocumentURI();
       if (uri) {
         uri->GetSpec(url);
         lineNo = 1;
       }
     }
 
     nsCxPusher pusher;
-    if (aNeedsCxPush && !pusher.Push(cx)) {
-      return NS_ERROR_FAILURE;
+    if (aNeedsCxPush) {
+      pusher.Push(cx);
     }
 
     uint32_t argCount;
     const char **argNames;
     // If no content, then just use kNameSpaceID_None for the
     // namespace ID.  In practice, it doesn't matter since SVG is
     // the only thing with weird arg names and SVG doesn't map event
     // listeners to the window.
--- a/dom/base/nsDOMClassInfo.cpp
+++ b/dom/base/nsDOMClassInfo.cpp
@@ -4077,17 +4077,17 @@ BaseStubConstructor(nsIWeakReference* aW
 
       JSObject* object = nullptr;
       wrappedJS->GetJSObject(&object);
       if (!object) {
         return NS_ERROR_UNEXPECTED;
       }
 
       nsCxPusher pusher;
-      NS_ENSURE_STATE(pusher.Push(cx));
+      pusher.Push(cx);
 
       JSAutoRequest ar(cx);
       JSAutoCompartment ac(cx, object);
 
       JS::Value thisValue = JSVAL_VOID;
       JS::Value funval;
       if (!JS_GetProperty(cx, object, "constructor", &funval) || !funval.isObject()) {
         return NS_ERROR_UNEXPECTED;
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -2072,19 +2072,17 @@ nsGlobalWindow::SetNewDocument(nsIDocume
   }
 
   nsRefPtr<nsGlobalWindow> newInnerWindow;
   bool createdInnerWindow = false;
 
   bool thisChrome = IsChromeWindow();
 
   nsCxPusher cxPusher;
-  if (!cxPusher.Push(cx)) {
-    return NS_ERROR_FAILURE;
-  }
+  cxPusher.Push(cx);
 
   XPCAutoRequest ar(cx);
 
   nsCOMPtr<WindowStateHolder> wsh = do_QueryInterface(aState);
   NS_ASSERTION(!aState || wsh, "What kind of weird state are you giving me here?");
 
   if (reUseInnerWindow) {
     // We're reusing the current inner window.
--- a/dom/base/nsJSEnvironment.cpp
+++ b/dom/base/nsJSEnvironment.cpp
@@ -1255,18 +1255,17 @@ nsJSContext::EvaluateString(const nsAStr
     *aRetValue = JSVAL_VOID;
   }
 
   if (!mScriptsEnabled) {
     return NS_OK;
   }
 
   nsCxPusher pusher;
-  if (!pusher.Push(mContext))
-    return NS_ERROR_FAILURE;
+  pusher.Push(mContext);
 
   xpc_UnmarkGrayObject(&aScopeObject);
   nsAutoMicroTask mt;
 
   JSPrincipals* p = JS_GetCompartmentPrincipals(js::GetObjectCompartment(&aScopeObject));
   aOptions.setPrincipals(p);
 
   bool ok = false;
@@ -1510,18 +1509,17 @@ nsJSContext::CallEventHandler(nsISupport
   jsval rval = JSVAL_VOID;
 
   // This one's a lot easier than EvaluateString because we don't have to
   // hassle with principals: they're already compiled into the JS function.
   // xxxmarkh - this comment is no longer true - principals are not used at
   // all now, and never were in some cases.
 
   nsCxPusher pusher;
-  if (!pusher.Push(mContext))
-    return NS_ERROR_FAILURE;
+  pusher.Push(mContext);
 
   // check if the event handler can be run on the object in question
   rv = sSecurityManager->CheckFunctionAccess(mContext, aHandler, target);
 
   nsJSContext::TerminationFuncHolder holder(this);
 
   if (NS_SUCCEEDED(rv)) {
     // Convert args to jsvals.
--- a/dom/bindings/CallbackObject.cpp
+++ b/dom/bindings/CallbackObject.cpp
@@ -86,19 +86,17 @@ CallbackObject::CallSetup::CallSetup(JSO
     // back on using the safe context.
     cx = nsContentUtils::GetSafeJSContext();
   }
 
   // Victory!  We have a JSContext.  Now do the things we need a JSContext for.
   mAr.construct(cx);
 
   // Make sure our JSContext is pushed on the stack.
-  if (!mCxPusher.Push(cx)) {
-    return;
-  }
+  mCxPusher.Push(cx);
 
   // After this point we guarantee calling ScriptEvaluated() if we
   // have an nsIScriptContext.
   // XXXbz Why, if, say CheckFunctionAccess fails?  I know that's how
   // nsJSContext::CallEventHandler works, but is it required?
   // FIXME: Bug 807369.
   mCtx = ctx;
 
--- a/dom/sms/src/SmsRequest.cpp
+++ b/dom/sms/src/SmsRequest.cpp
@@ -518,17 +518,17 @@ SmsRequest::NotifyThreadList(const Infal
   MOZ_ASSERT(cx);
 
   nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(GetOwner());
 
   JSObject* ownerObj = sgo->GetGlobalJSObject();
   NS_ENSURE_TRUE_VOID(ownerObj);
 
   nsCxPusher pusher;
-  NS_ENSURE_TRUE_VOID(pusher.Push(cx));
+  pusher.Push(cx);
 
   JSAutoRequest ar(cx);
   JSAutoCompartment ac(cx, ownerObj);
 
   JSObject* array = JS_NewArrayObject(cx, aItems.Length(), nullptr);
   NS_ENSURE_TRUE_VOID(array);
 
   bool ok;
--- a/js/xpconnect/src/dictionary_helper_gen.py
+++ b/js/xpconnect/src/dictionary_helper_gen.py
@@ -401,17 +401,17 @@ def write_cpp(iface, fd):
              "  if (!aCx || !aVal) {\n"
              "    return NS_OK;\n"
              "  }\n"
              "  if (!aVal->isObject()) {\n"
              "    return aVal->isNullOrUndefined() ? NS_OK : NS_ERROR_TYPE_ERR;\n"
              "  }\n\n"
              "  JSObject* obj = &aVal->toObject();\n"
              "  nsCxPusher pusher;\n"
-             "  NS_ENSURE_STATE(pusher.Push(aCx));\n"
+             "  pusher.Push(aCx);\n"
              "  JSAutoRequest ar(aCx);\n"
              "  JSAutoCompartment ac(aCx, obj);\n")
 
     fd.write("  return %s_InitInternal(*this, aCx, obj);\n}\n\n" %
                  iface.name)
 
 
 if __name__ == '__main__':
--- a/layout/base/nsRefreshDriver.cpp
+++ b/layout/base/nsRefreshDriver.cpp
@@ -557,20 +557,18 @@ nsRefreshDriver::AdvanceTimeAndRefresh(i
 
     mTestControllingRefreshes = true;
   }
 
   mMostRecentRefreshEpochTime += aMilliseconds * 1000;
   mMostRecentRefresh += TimeDuration::FromMilliseconds((double) aMilliseconds);
 
   nsCxPusher pusher;
-  if (pusher.PushNull()) {
-    DoTick();
-    pusher.Pop();
-  }
+  pusher.PushNull();
+  DoTick();
 }
 
 void
 nsRefreshDriver::RestoreNormalRefresh()
 {
   mTestControllingRefreshes = false;
   EnsureTimerStarted(false);
 }
--- a/xpfe/appshell/src/nsXULWindow.cpp
+++ b/xpfe/appshell/src/nsXULWindow.cpp
@@ -1795,18 +1795,17 @@ NS_IMETHODIMP nsXULWindow::CreateNewCont
   }
   NS_ENSURE_TRUE(uri, NS_ERROR_FAILURE);
 
   // We need to create a chrome window to contain the content window we're about
   // to pass back. The subject principal needs to be system while we're creating
   // it to make things work right, so push a null cx. See bug 799348 comment 13
   // for a description of what happens when we don't.
   nsCxPusher pusher;
-  if (!pusher.PushNull())
-    return NS_ERROR_FAILURE;
+  pusher.PushNull();
   nsCOMPtr<nsIXULWindow> newWindow;
   appShell->CreateTopLevelWindow(this, uri,
                                  aChromeFlags, 615, 480,
                                  getter_AddRefs(newWindow));
   NS_ENSURE_TRUE(newWindow, NS_ERROR_FAILURE);
   pusher.Pop();
 
   // Specify that we want the window to remain locked until the chrome has loaded.