Bug 834732 - Get rid of footgun bool param for nsCxPusher and use an explicit enum. r=mrbkap
authorBobby Holley <bobbyholley@gmail.com>
Wed, 13 Feb 2013 00:22:26 +0100
changeset 131561 cec9e89a2a9674c673349ec30c905ba191327ae9
parent 131560 6228ddc49fc263aaf1640f1cab7522be78f16da3
child 131562 0e7ab61ab410564a7d53419ce5d2a95a240f582c
push id2323
push userbbajaj@mozilla.com
push dateMon, 01 Apr 2013 19:47:02 +0000
treeherdermozilla-beta@7712be144d91 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmrbkap
bugs834732
milestone21.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 - Get rid of footgun bool param for nsCxPusher and use an explicit enum. r=mrbkap The goal here is to get rid of this crap entirely, and make nsCxPusher always push. But that's a scary change, so we do it in chunks. This patch, in particular, should have zero behavioral change. This means preserving some very wrong behavior. For instance, currently SafeAutoJSContext never pushes a damn thing, because the safe JSContext doesn't have an associated nsIScriptContext. We preserve this behavior, and in fact convert various similarly-buggy consumers to SafeAutoJSContext, so that we can hoist the behavioral change into a subsequent patch.
content/base/public/nsContentUtils.h
content/base/src/nsContentUtils.cpp
content/base/src/nsFrameMessageManager.cpp
content/events/src/nsEventListenerManager.cpp
content/xbl/src/nsXBLBinding.cpp
dom/base/nsDOMClassInfo.cpp
dom/base/nsGlobalWindow.cpp
dom/base/nsJSEnvironment.cpp
dom/base/nsStructuredCloneContainer.cpp
dom/bindings/CallbackObject.cpp
dom/indexedDB/IDBFactory.cpp
dom/sms/src/SmsRequest.cpp
dom/system/gonk/SystemWorkerManager.cpp
js/ipc/ObjectWrapperChild.cpp
js/xpconnect/src/dictionary_helper_gen.py
--- a/content/base/public/nsContentUtils.h
+++ b/content/base/public/nsContentUtils.h
@@ -2190,27 +2190,28 @@ typedef nsCharSeparatedTokenizerTemplate
 
 #define NS_DROP_JS_OBJECTS(obj, clazz)                                         \
   nsContentUtils::DropJSObjects(NS_CYCLE_COLLECTION_UPCAST(obj, clazz))
 
 
 class NS_STACK_CLASS nsCxPusher
 {
 public:
+  enum PushBehavior { ALWAYS_PUSH, REQUIRE_SCRIPT_CONTEXT, ASSERT_SCRIPT_CONTEXT };
   nsCxPusher();
   ~nsCxPusher(); // Calls Pop();
 
   // 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, bool aRequiresScriptContext = true);
+  bool Push(JSContext *cx, PushBehavior behavior);
   // Explicitly push a null JSContext on the the stack
   bool PushNull();
 
   // Pop() will be a no-op if Push() or PushNull() fail
   void Pop();
 
   nsIScriptContext* GetCurrentScriptContext() { return mScx; }
 private:
--- a/content/base/src/nsContentUtils.cpp
+++ b/content/base/src/nsContentUtils.cpp
@@ -3010,29 +3010,23 @@ nsCxPusher::Push(nsIDOMEventTarget *aCur
       DoPush(cx);
     }
 
     // Nothing to do here, I guess.  Have to return true so that event firing
     // will still work correctly even if there is no associated JSContext
     return true;
   }
 
-  JSContext* cx = nullptr;
-
-  if (scx) {
-    cx = scx->GetNativeContext();
-    // Bad, no JSContext from script context!
-    NS_ENSURE_TRUE(cx, false);
-  }
+  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);
+  return Push(cx, ASSERT_SCRIPT_CONTEXT);
 }
 
 bool
 nsCxPusher::RePush(nsIDOMEventTarget *aCurrentTarget)
 {
   if (!mPushedSomething) {
     return Push(aCurrentTarget);
   }
@@ -3054,33 +3048,34 @@ nsCxPusher::RePush(nsIDOMEventTarget *aC
     }
   }
 
   Pop();
   return Push(aCurrentTarget);
 }
 
 bool
-nsCxPusher::Push(JSContext *cx, bool aRequiresScriptContext)
+nsCxPusher::Push(JSContext *cx, PushBehavior behavior)
 {
   if (mPushedSomething) {
     NS_ERROR("Whaaa! No double pushing with nsCxPusher::Push()!");
 
     return false;
   }
 
   if (!cx) {
     return false;
   }
 
   // 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);
-  if (!mScx && aRequiresScriptContext) {
+  MOZ_ASSERT_IF(behavior == ASSERT_SCRIPT_CONTEXT, mScx);
+  if (!mScx && behavior == REQUIRE_SCRIPT_CONTEXT) {
     // Should probably return false. See bug 416916.
     return true;
   }
 
   return DoPush(cx);
 }
 
 bool
@@ -6855,17 +6850,19 @@ 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);
+    // XXXbholley - This is totally wrong, but necessary to make this patch
+    // not change any behavior. We'll fix it in an upcoming patch.
+    bool result = mPusher.Push(mCx, nsCxPusher::REQUIRE_SCRIPT_CONTEXT);
     if (!result || !mCx) {
       MOZ_CRASH();
     }
   }
 }
 
 AutoJSContext::operator JSContext*()
 {
--- 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, false));
+        NS_ENSURE_STATE(pusher.Push(ctx, nsCxPusher::ALWAYS_PUSH));
 
         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,17 +813,17 @@ nsEventListenerManager::CompileEventHand
       nsIURI *uri = doc->GetDocumentURI();
       if (uri) {
         uri->GetSpec(url);
         lineNo = 1;
       }
     }
 
     nsCxPusher pusher;
-    if (aNeedsCxPush && !pusher.Push(cx)) {
+    if (aNeedsCxPush && !pusher.Push(cx, nsCxPusher::ASSERT_SCRIPT_CONTEXT)) {
       return NS_ERROR_FAILURE;
     }
 
     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
--- a/content/xbl/src/nsXBLBinding.cpp
+++ b/content/xbl/src/nsXBLBinding.cpp
@@ -938,17 +938,17 @@ nsXBLBinding::ChangeDocument(nsIDocument
           // about fixing the prototype chain, since everything's going
           // away immediately.
 
           nsCOMPtr<nsIScriptContext> context = global->GetContext();
           if (context && scope) {
             JSContext *cx = context->GetNativeContext();
  
             nsCxPusher pusher;
-            pusher.Push(cx);
+            pusher.Push(cx, nsCxPusher::ASSERT_SCRIPT_CONTEXT);
 
             JSObject* scriptObject = mBoundElement->GetWrapper();
             if (scriptObject) {
               // XXX Stay in sync! What if a layered binding has an
               // <interface>?!
               // XXXbz what does that comment mean, really?  It seems to date
               // back to when there was such a thing as an <interface>, whever
               // that was...
--- a/dom/base/nsDOMClassInfo.cpp
+++ b/dom/base/nsDOMClassInfo.cpp
@@ -4760,17 +4760,17 @@ BaseStubConstructor(nsIWeakReference* aW
 
       JSObject* object = nullptr;
       wrappedJS->GetJSObject(&object);
       if (!object) {
         return NS_ERROR_UNEXPECTED;
       }
 
       nsCxPusher pusher;
-      NS_ENSURE_STATE(pusher.Push(cx, false));
+      NS_ENSURE_STATE(pusher.Push(cx, nsCxPusher::ALWAYS_PUSH));
 
       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;
@@ -8541,17 +8541,17 @@ nsresult
 nsHTMLPluginObjElementSH::SetupProtoChain(nsIXPConnectWrappedNative *wrapper,
                                           JSContext *cx,
                                           JSObject *obj)
 {
   NS_ASSERTION(nsContentUtils::IsSafeToRunScript(),
                "Shouldn't have gotten in here");
 
   nsCxPusher cxPusher;
-  if (!cxPusher.Push(cx)) {
+  if (!cxPusher.Push(cx, nsCxPusher::REQUIRE_SCRIPT_CONTEXT)) {
     return NS_OK;
   }
 
   JSAutoRequest ar(cx);
   JSAutoCompartment ac(cx, obj);
 
   nsRefPtr<nsNPAPIPluginInstance> pi;
   nsresult rv = GetPluginInstanceIfSafe(wrapper, obj, cx, getter_AddRefs(pi));
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
@@ -2080,17 +2080,17 @@ nsGlobalWindow::SetNewDocument(nsIDocume
   }
 
   nsRefPtr<nsGlobalWindow> newInnerWindow;
   bool createdInnerWindow = false;
 
   bool thisChrome = IsChromeWindow();
 
   nsCxPusher cxPusher;
-  if (!cxPusher.Push(cx)) {
+  if (!cxPusher.Push(cx, nsCxPusher::ASSERT_SCRIPT_CONTEXT)) {
     return NS_ERROR_FAILURE;
   }
 
   XPCAutoRequest ar(cx);
 
   nsCOMPtr<WindowStateHolder> wsh = do_QueryInterface(aState);
   NS_ASSERTION(!aState || wsh, "What kind of weird state are you giving me here?");
 
--- a/dom/base/nsJSEnvironment.cpp
+++ b/dom/base/nsJSEnvironment.cpp
@@ -1255,17 +1255,17 @@ nsJSContext::EvaluateString(const nsAStr
     *aRetValue = JSVAL_VOID;
   }
 
   if (!mScriptsEnabled) {
     return NS_OK;
   }
 
   nsCxPusher pusher;
-  if (!pusher.Push(mContext))
+  if (!pusher.Push(mContext, nsCxPusher::ASSERT_SCRIPT_CONTEXT))
     return NS_ERROR_FAILURE;
 
   xpc_UnmarkGrayObject(&aScopeObject);
   nsAutoMicroTask mt;
 
   JSPrincipals* p = JS_GetCompartmentPrincipals(js::GetObjectCompartment(&aScopeObject));
   aOptions.setPrincipals(p);
 
@@ -1510,17 +1510,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, true))
+  if (!pusher.Push(mContext, nsCxPusher::ASSERT_SCRIPT_CONTEXT))
     return NS_ERROR_FAILURE;
 
   // 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)) {
--- a/dom/base/nsStructuredCloneContainer.cpp
+++ b/dom/base/nsStructuredCloneContainer.cpp
@@ -50,17 +50,17 @@ nsStructuredCloneContainer::InitFromVari
   NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
 
   // Make sure that we serialize in the right context.
   JSAutoRequest ar(aCx);
   JSAutoCompartment ac(aCx, JS_GetGlobalObject(aCx));
   JS_WrapValue(aCx, &jsData);
 
   nsCxPusher cxPusher;
-  cxPusher.Push(aCx);
+  cxPusher.Push(aCx, nsCxPusher::REQUIRE_SCRIPT_CONTEXT);
 
   uint64_t* jsBytes = nullptr;
   bool success = JS_WriteStructuredClone(aCx, jsData, &jsBytes, &mSize,
                                            nullptr, nullptr, JSVAL_VOID);
   NS_ENSURE_STATE(success);
   NS_ENSURE_STATE(jsBytes);
 
   // Copy jsBytes into our own buffer.
--- a/dom/bindings/CallbackObject.cpp
+++ b/dom/bindings/CallbackObject.cpp
@@ -82,17 +82,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, false)) {
+  if (!mCxPusher.Push(cx, nsCxPusher::ALWAYS_PUSH)) {
     return;
   }
 
   // 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.
--- a/dom/indexedDB/IDBFactory.cpp
+++ b/dom/indexedDB/IDBFactory.cpp
@@ -209,25 +209,17 @@ IDBFactory::Create(ContentParent* aConte
     }
   }
 #endif
 
   nsCOMPtr<nsIPrincipal> principal =
     do_CreateInstance("@mozilla.org/nullprincipal;1");
   NS_ENSURE_TRUE(principal, NS_ERROR_FAILURE);
 
-  JSContext* cx = nsContentUtils::GetSafeJSContext();
-  NS_ENSURE_TRUE(cx, NS_ERROR_FAILURE);
-
-  nsCxPusher pusher;
-  if (!pusher.Push(cx)) {
-    NS_WARNING("Failed to push safe JS context!");
-    return NS_ERROR_FAILURE;
-  }
-
+  SafeAutoJSContext cx;
   JSAutoRequest ar(cx);
 
   nsIXPConnect* xpc = nsContentUtils::XPConnect();
   NS_ASSERTION(xpc, "This should never be null!");
 
   nsCOMPtr<nsIXPConnectJSObjectHolder> globalHolder;
   nsresult rv = xpc->CreateSandbox(cx, principal, getter_AddRefs(globalHolder));
   NS_ENSURE_SUCCESS(rv, rv);
--- 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, false));
+  NS_ENSURE_TRUE_VOID(pusher.Push(cx, nsCxPusher::ALWAYS_PUSH));
 
   JSAutoRequest ar(cx);
   JSAutoCompartment ac(cx, ownerObj);
 
   JSObject* array = JS_NewArrayObject(cx, aItems.Length(), nullptr);
   NS_ENSURE_TRUE_VOID(array);
 
   bool ok;
--- a/dom/system/gonk/SystemWorkerManager.cpp
+++ b/dom/system/gonk/SystemWorkerManager.cpp
@@ -337,23 +337,17 @@ SystemWorkerManager::Init()
 {
   if (XRE_GetProcessType() != GeckoProcessType_Default) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   NS_ASSERTION(NS_IsMainThread(), "We can only initialize on the main thread");
   NS_ASSERTION(!mShutdown, "Already shutdown!");
 
-  JSContext* cx = nsContentUtils::ThreadJSContextStack()->GetSafeJSContext();
-  NS_ENSURE_TRUE(cx, NS_ERROR_FAILURE);
-
-  nsCxPusher pusher;
-  if (!pusher.Push(cx, false)) {
-    return NS_ERROR_FAILURE;
-  }
+  AutoSafeJSContext cx;
 
   nsresult rv = InitWifi(cx);
   if (NS_FAILED(rv)) {
     NS_WARNING("Failed to initialize WiFi Networking!");
     return rv;
   }
 
 #ifdef MOZ_WIDGET_GONK
--- a/js/ipc/ObjectWrapperChild.cpp
+++ b/js/ipc/ObjectWrapperChild.cpp
@@ -37,17 +37,17 @@ namespace {
         AutoContextPusher(JSContext* cx
                           MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
             : mRequest(cx)
             , mContext(cx)
             , mSavedOptions(JS_SetOptions(cx, (JS_GetOptions(cx) |
                                                JSOPTION_DONT_REPORT_UNCAUGHT)))
         {
             MOZ_GUARD_OBJECT_NOTIFIER_INIT;
-            mStack.Push(cx, false);
+            mStack.Push(cx, nsCxPusher::ALWAYS_PUSH);
         }
 
         ~AutoContextPusher() {
             mStack.Pop();
             JS_SetOptions(mContext, mSavedOptions);
         }
 
     };
--- a/js/xpconnect/src/dictionary_helper_gen.py
+++ b/js/xpconnect/src/dictionary_helper_gen.py
@@ -423,17 +423,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, false));\n"
+             "  NS_ENSURE_STATE(pusher.Push(aCx, nsCxPusher::ALWAYS_PUSH));\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__':