Bug 888104 - Introduce AutoCxPusher and reimplement nsCxPusher in terms of it. r=gabor

js/xpconnect/src/nsCxPusher.cpp

@@ -17,27 +17,12 @@ using mozilla::dom::EventTarget;
 using mozilla::DebugOnly;
 
 NS_EXPORT
-nsCxPusher::nsCxPusher()
-    : mScriptIsRunning(false),
-      mPushedSomething(false)
-{
-}
-
-NS_EXPORT
-nsCxPusher::~nsCxPusher()
-{
-  Pop();
-}
+nsCxPusher::~nsCxPusher() {}
 
 bool
 nsCxPusher::Push(EventTarget *aCurrentTarget)
 {
-  if (mPushedSomething) {
-    NS_ERROR("Whaaa! No double pushing with nsCxPusher::Push()!");
-
-    return false;
-  }
-
+  MOZ_ASSERT(mPusher.empty());
   NS_ENSURE_TRUE(aCurrentTarget, false);
   nsresult rv;
   nsIScriptContext* scx =
@@ -54,7 +39,7 @@ nsCxPusher::Push(EventTarget *aCurrentTarget)
     // The target may have a special JS context for event handlers.
     JSContext* cx = aCurrentTarget->GetJSContextForEventHandlers();
     if (cx) {
-      DoPush(cx);
+      mPusher.construct(cx);
     }
 
     // Nothing to do here, I guess.  Have to return true so that event firing
@@ -62,20 +47,14 @@ nsCxPusher::Push(EventTarget *aCurrentTarget)
     return true;
   }
 
-  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.
-  Push(cx);
+  mPusher.construct(scx->GetNativeContext());
   return true;
 }
 
 bool
 nsCxPusher::RePush(EventTarget *aCurrentTarget)
 {
-  if (!mPushedSomething) {
+  if (mPusher.empty()) {
     return Push(aCurrentTarget);
   }
 
@@ -84,34 +63,49 @@ nsCxPusher::RePush(EventTarget *aCurrentTarget)
     nsIScriptContext* scx =
       aCurrentTarget->GetContextForEventHandlers(&rv);
     if (NS_FAILED(rv)) {
-      Pop();
+      mPusher.destroy();
       return false;
     }
 
     // If we have the same script context and native context is still
     // alive, no need to Pop/Push.
-    if (scx && scx == mScx &&
+    if (scx && scx == mPusher.ref().GetScriptContext() &&
         scx->GetNativeContext()) {
       return true;
     }
   }
 
-  Pop();
+  mPusher.destroy();
   return Push(aCurrentTarget);
 }
 
 NS_EXPORT_(void)
 nsCxPusher::Push(JSContext *cx)
 {
-  MOZ_ASSERT(!mPushedSomething, "No double pushing with nsCxPusher::Push()!");
-  MOZ_ASSERT(cx);
-
-  DoPush(cx);
+  mPusher.construct(cx);
 }
 
 void
-nsCxPusher::DoPush(JSContext* cx)
+nsCxPusher::PushNull()
 {
+  // Note: The Maybe<> template magic seems to need the static_cast below to
+  // work right on some older compilers.
+  mPusher.construct(static_cast<JSContext*>(nullptr), /* aAllowNull = */ true);
+}
+
+NS_EXPORT_(void)
+nsCxPusher::Pop()
+{
+  if (!mPusher.empty())
+    mPusher.destroy();
+}
+
+namespace mozilla {
+
+AutoCxPusher::AutoCxPusher(JSContext* cx, bool allowNull) : mScriptIsRunning(false)
+{
+  MOZ_ASSERT_IF(!allowNull, cx);
+
   // If we have a 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|?
@@ -139,33 +133,14 @@ nsCxPusher::DoPush(JSContext* cx)
     xpc_UnmarkGrayContext(cx);
   }
 
-  mPushedSomething = true;
 #ifdef DEBUG
   mPushedContext = cx;
-  if (cx)
-    mCompartmentDepthOnEntry = js::GetEnterCompartmentDepth(cx);
+  mCompartmentDepthOnEntry = cx ? js::GetEnterCompartmentDepth(cx) : 0;
 #endif
 }
 
-void
-nsCxPusher::PushNull()
+AutoCxPusher::~AutoCxPusher()
 {
-  DoPush(nullptr);
-}
-
-NS_EXPORT_(void)
-nsCxPusher::Pop()
-{
-  if (!mPushedSomething) {
-    mScx = nullptr;
-    mPushedSomething = false;
-
-    NS_ASSERTION(!mScriptIsRunning, "Huh, this can't be happening, "
-                 "mScriptIsRunning can't be set here!");
-
-    return;
-  }
-
   // Leave the request before popping.
   mAutoRequest.destroyIfConstructed();
 
@@ -189,11 +164,8 @@ nsCxPusher::Pop()
 
   mScx = nullptr;
   mScriptIsRunning = false;
-  mPushedSomething = false;
 }
 
-namespace mozilla {
-
 AutoJSContext::AutoJSContext(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_IN_IMPL)
   : mCx(nullptr)
 {

js/xpconnect/src/nsCxPusher.h

@@ -16,11 +16,54 @@ class EventTarget;
 
 class nsIScriptContext;
 
+namespace mozilla {
+
+/**
+ * Fundamental cx pushing class. All other cx pushing classes are implemented
+ * in terms of this class.
+ */
+class MOZ_STACK_CLASS AutoCxPusher
+{
+public:
+  AutoCxPusher(JSContext *aCx, bool aAllowNull = false);
+  ~AutoCxPusher();
+
+  nsIScriptContext* GetScriptContext() { return mScx; }
+
+private:
+  mozilla::Maybe<JSAutoRequest> mAutoRequest;
+  nsCOMPtr<nsIScriptContext> mScx;
+  bool mScriptIsRunning;
+#ifdef DEBUG
+  JSContext* mPushedContext;
+  unsigned mCompartmentDepthOnEntry;
+#endif
+};
+
+} /* namespace mozilla */
+
+/**
+ * Legacy cx pushing class.
+ *
+ * This class provides a rather wonky interface, with the following quirks:
+ *   * The constructor is a no-op, and callers must explicitly call one of
+ *     the Push() methods.
+ *   * Null must be pushed with PushNull().
+ *   * The cx pusher can be reused multiple times with RePush().
+ *
+ * This class implements this interface in terms of the much simpler
+ * AutoCxPusher class below.
+ */
 class MOZ_STACK_CLASS nsCxPusher
 {
 public:
-  NS_EXPORT nsCxPusher();
-  NS_EXPORT ~nsCxPusher(); // Calls Pop();
+  // This destructor doesn't actually do anything, but it implicitly depends on
+  // the Maybe<AutoCxPusher> destructor, which in turn depends on the
+  // ~AutoCxPusher destructor. If we stick with the default destructor, the
+  // caller needs to be able to link against the AutoCxPusher destructor, which
+  // isn't possible with externally-linked consumers like xpcshell. Hoist this
+  // work into nsCxPusher.cpp and use NS_EXPORT to make it all work right.
+  NS_EXPORT ~nsCxPusher();
 
   // Returns false if something erroneous happened.
   bool Push(mozilla::dom::EventTarget *aCurrentTarget);
@@ -36,19 +79,12 @@ public:
   // Pop() will be a no-op if Push() or PushNull() fail
   NS_EXPORT_(void) Pop();
 
-  nsIScriptContext* GetCurrentScriptContext() { return mScx; }
-private:
-  // Combined code for PushNull() and Push(JSContext*)
-  void DoPush(JSContext* cx);
+  nsIScriptContext* GetCurrentScriptContext() {
+    return mPusher.empty() ? nullptr : mPusher.ref().GetScriptContext();
+  }
 
-  mozilla::Maybe<JSAutoRequest> mAutoRequest;
-  nsCOMPtr<nsIScriptContext> mScx;
-  bool mScriptIsRunning;
-  bool mPushedSomething;
-#ifdef DEBUG
-  JSContext* mPushedContext;
-  unsigned mCompartmentDepthOnEntry;
-#endif
+private:
+  mozilla::Maybe<mozilla::AutoCxPusher> mPusher;
 };
 
 namespace mozilla {