Bug 737365 - stop using the cx during finalization, part 2.
authorIgor Bukanov <igor@mir2.org>
Mon, 19 Mar 2012 15:34:58 +0100
changeset 94310 74053b148a3c8883ad1a375107c94359606f6e1e
parent 94309 d5057ff02ffb9786f62ca69ebc22cf11aa86f612
child 94311 d8c5316f513afee907aabd5a04873bdf41fdebb7
push id886
push userlsblakk@mozilla.com
push dateMon, 04 Jun 2012 19:57:52 +0000
treeherdermozilla-beta@bbd8d5efd6d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs737365
milestone14.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 737365 - stop using the cx during finalization, part 2. This parts removes the usage of JSContext* during the finalization in Web Workers and JS shells implementations. With this chnages JSContext * is only accessed during the finalization in the SpiderMonkey implementation.
dom/workers/WorkerPrivate.cpp
dom/workers/WorkerPrivate.h
dom/workers/XMLHttpRequest.cpp
js/src/jsapi.cpp
js/src/jsapi.h
js/src/jscntxt.h
js/src/jsgc.cpp
js/src/shell/js.cpp
--- a/dom/workers/WorkerPrivate.cpp
+++ b/dom/workers/WorkerPrivate.cpp
@@ -1915,20 +1915,20 @@ WorkerPrivateParent<Derived>::Start()
       mParentStatus = Running;
       return true;
     }
   }
 
   return false;
 }
 
+// aCx is null when called from the finalizer
 template <class Derived>
 bool
-WorkerPrivateParent<Derived>::NotifyPrivate(JSContext* aCx, Status aStatus,
-                                            bool aFromJSObjectFinalizer)
+WorkerPrivateParent<Derived>::NotifyPrivate(JSContext* aCx, Status aStatus)
 {
   AssertIsOnParentThread();
 
   bool pending;
   {
     MutexAutoLock lock(mMutex);
 
     if (mParentStatus >= aStatus) {
@@ -1957,19 +1957,18 @@ WorkerPrivateParent<Derived>::NotifyPriv
 
   NS_ASSERTION(aStatus != Terminating || mQueuedRunnables.IsEmpty(),
                "Shouldn't have anything queued!");
 
   // Anything queued will be discarded.
   mQueuedRunnables.Clear();
 
   nsRefPtr<NotifyRunnable> runnable =
-    new NotifyRunnable(ParentAsWorkerPrivate(), aFromJSObjectFinalizer,
-                       aStatus);
-  return runnable->Dispatch(aFromJSObjectFinalizer ? nsnull : aCx);
+    new NotifyRunnable(ParentAsWorkerPrivate(), !aCx, aStatus);
+  return runnable->Dispatch(aCx);
 }
 
 template <class Derived>
 bool
 WorkerPrivateParent<Derived>::Suspend(JSContext* aCx)
 {
   AssertIsOnParentThread();
   NS_ASSERTION(!mParentSuspended, "Suspended more than once!");
@@ -2048,17 +2047,17 @@ WorkerPrivateParent<Derived>::_Finalize(
   AssertIsOnParentThread();
 
   MOZ_ASSERT(mJSObject);
   MOZ_ASSERT(!mJSObjectRooted);
 
   // Clear the JS object.
   mJSObject = nsnull;
 
-  if (!TerminatePrivate(aFop->context, true)) {
+  if (!TerminatePrivate(nsnull)) {
     NS_WARNING("Failed to terminate!");
   }
 
   // Before calling through to the base class we need to grab another reference
   // if we're on the main thread. Otherwise the base class' _Finalize method
   // will call Release, and some of our members cannot be released during
   // finalization. Of course, if those members are already gone then we can skip
   // this mess...
@@ -2142,19 +2141,18 @@ WorkerPrivateParent<Derived>::RootJSObje
 
   if (aRoot != mJSObjectRooted) {
     if (aRoot) {
       if (!JS_AddNamedObjectRoot(aCx, &mJSObject, "Worker root")) {
         NS_WARNING("JS_AddNamedObjectRoot failed!");
         return false;
       }
     }
-    else if (!JS_RemoveObjectRoot(aCx, &mJSObject)) {
-      NS_WARNING("JS_RemoveObjectRoot failed!");
-      return false;
+    else {
+      JS_RemoveObjectRoot(aCx, &mJSObject);
     }
 
     mJSObjectRooted = aRoot;
   }
 
   return true;
 }
 
--- a/dom/workers/WorkerPrivate.h
+++ b/dom/workers/WorkerPrivate.h
@@ -239,35 +239,37 @@ protected:
 
 private:
   Derived*
   ParentAsWorkerPrivate() const
   {
     return static_cast<Derived*>(const_cast<WorkerPrivateParent*>(this));
   }
 
-  bool
-  NotifyPrivate(JSContext* aCx, Status aStatus, bool aFromJSFinalizer);
-
+  // aCx is null when called from the finalizer
   bool
-  TerminatePrivate(JSContext* aCx, bool aFromJSFinalizer)
+  NotifyPrivate(JSContext* aCx, Status aStatus);
+
+  // aCx is null when called from the finalizer
+  bool
+  TerminatePrivate(JSContext* aCx)
   {
-    return NotifyPrivate(aCx, Terminating, aFromJSFinalizer);
+    return NotifyPrivate(aCx, Terminating);
   }
 
 public:
   // May be called on any thread...
   bool
   Start();
 
   // Called on the parent thread.
   bool
   Notify(JSContext* aCx, Status aStatus)
   {
-    return NotifyPrivate(aCx, aStatus, false);
+    return NotifyPrivate(aCx, aStatus);
   }
 
   bool
   Cancel(JSContext* aCx)
   {
     return Notify(aCx, Canceling);
   }
 
@@ -295,17 +297,17 @@ public:
     RootJSObject(aCx, false);
   }
 
   bool
   Terminate(JSContext* aCx)
   {
     AssertIsOnParentThread();
     RootJSObject(aCx, false);
-    return TerminatePrivate(aCx, false);
+    return TerminatePrivate(aCx);
   }
 
   bool
   Close(JSContext* aCx);
 
   bool
   ModifyBusyCount(JSContext* aCx, bool aIncrease);
 
--- a/dom/workers/XMLHttpRequest.cpp
+++ b/dom/workers/XMLHttpRequest.cpp
@@ -1543,19 +1543,17 @@ XMLHttpRequest::MaybePin(nsresult& aRv)
   JSContext* cx = GetJSContext();
 
   if (!JS_AddNamedObjectRoot(cx, &mJSObject, "XMLHttpRequest mJSObject")) {
     aRv = NS_ERROR_FAILURE;
     return;
   }
 
   if (!mWorkerPrivate->AddFeature(cx, this)) {
-    if (!JS_RemoveObjectRoot(cx, &mJSObject)) {
-      NS_ERROR("JS_RemoveObjectRoot failed!");
-    }
+    JS_RemoveObjectRoot(cx, &mJSObject);
     aRv = NS_ERROR_FAILURE;
     return;
   }
 
   mJSObjectRooted = true;
 }
 
 void
@@ -1659,19 +1657,17 @@ void
 XMLHttpRequest::Unpin()
 {
   mWorkerPrivate->AssertIsOnWorkerThread();
 
   NS_ASSERTION(mJSObjectRooted, "Mismatched calls to Unpin!");
 
   JSContext* cx = GetJSContext();
 
-  if (!JS_RemoveObjectRoot(cx, &mJSObject)) {
-    NS_ERROR("JS_RemoveObjectRoot failed!");
-  }
+  JS_RemoveObjectRoot(cx, &mJSObject);
 
   mWorkerPrivate->RemoveFeature(cx, this);
 
   mJSObjectRooted = false;
 }
 
 void
 XMLHttpRequest::SendInternal(const nsAString& aStringBody,
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -771,16 +771,17 @@ JSRuntime::JSRuntime()
     debugMode(false),
     profilingScripts(false),
     hadOutOfMemory(false),
     data(NULL),
 #ifdef JS_THREADSAFE
     gcLock(NULL),
     gcHelperThread(thisFromCtor()),
 #endif
+    defaultFreeOp_(thisFromCtor(), false, false, NULL),
     debuggerMutations(0),
     securityCallbacks(const_cast<JSSecurityCallbacks *>(&NullSecurityCallbacks)),
     destroyPrincipals(NULL),
     structuredCloneCallbacks(NULL),
     telemetryCallback(NULL),
     propertyRemovals(0),
     thousandsSeparator(0),
     decimalSeparator(0),
@@ -2230,16 +2231,22 @@ JS_free(JSContext *cx, void *p)
 }
 
 JS_PUBLIC_API(void)
 JS_freeop(JSFreeOp *fop, void *p)
 {
     return FreeOp::get(fop)->free_(p);
 }
 
+JS_PUBLIC_API(JSFreeOp *)
+JS_GetDefaultFreeOp(JSRuntime *rt)
+{
+    return rt->defaultFreeOp();
+}
+
 JS_PUBLIC_API(void)
 JS_updateMallocCounter(JSContext *cx, size_t nbytes)
 {
     return cx->runtime->updateMallocCounter(cx, nbytes);
 }
 
 JS_PUBLIC_API(char *)
 JS_strdup(JSContext *cx, const char *s)
@@ -2332,49 +2339,73 @@ JS_AddNamedGCThingRoot(JSContext *cx, vo
 {
     AssertNoGC(cx);
     CHECK_REQUEST(cx);
     return js_AddGCThingRoot(cx, (void **)rp, name);
 }
 
 /* We allow unrooting from finalizers within the GC */
 
-JS_PUBLIC_API(JSBool)
+JS_PUBLIC_API(void)
 JS_RemoveValueRoot(JSContext *cx, jsval *vp)
 {
     CHECK_REQUEST(cx);
-    return js_RemoveRoot(cx->runtime, (void *)vp);
-}
-
-JS_PUBLIC_API(JSBool)
+    js_RemoveRoot(cx->runtime, (void *)vp);
+}
+
+JS_PUBLIC_API(void)
 JS_RemoveStringRoot(JSContext *cx, JSString **rp)
 {
     CHECK_REQUEST(cx);
-    return js_RemoveRoot(cx->runtime, (void *)rp);
-}
-
-JS_PUBLIC_API(JSBool)
+    js_RemoveRoot(cx->runtime, (void *)rp);
+}
+
+JS_PUBLIC_API(void)
 JS_RemoveObjectRoot(JSContext *cx, JSObject **rp)
 {
     CHECK_REQUEST(cx);
-    return js_RemoveRoot(cx->runtime, (void *)rp);
-}
-
-JS_PUBLIC_API(JSBool)
+    js_RemoveRoot(cx->runtime, (void *)rp);
+}
+
+JS_PUBLIC_API(void)
 JS_RemoveScriptRoot(JSContext *cx, JSScript **rp)
 {
     CHECK_REQUEST(cx);
-    return js_RemoveRoot(cx->runtime, (void *)rp);
-}
-
-JS_PUBLIC_API(JSBool)
+    js_RemoveRoot(cx->runtime, (void *)rp);
+}
+
+JS_PUBLIC_API(void)
 JS_RemoveGCThingRoot(JSContext *cx, void **rp)
 {
     CHECK_REQUEST(cx);
-    return js_RemoveRoot(cx->runtime, (void *)rp);
+    js_RemoveRoot(cx->runtime, (void *)rp);
+}
+
+JS_PUBLIC_API(void)
+JS_RemoveValueRootRT(JSRuntime *rt, jsval *vp)
+{
+    js_RemoveRoot(rt, (void *)vp);
+}
+
+JS_PUBLIC_API(void)
+JS_RemoveStringRootRT(JSRuntime *rt, JSString **rp)
+{
+    js_RemoveRoot(rt, (void *)rp);
+}
+
+JS_PUBLIC_API(void)
+JS_RemoveObjectRootRT(JSRuntime *rt, JSObject **rp)
+{
+    js_RemoveRoot(rt, (void *)rp);
+}
+
+JS_PUBLIC_API(void)
+JS_RemoveScriptRoot(JSRuntime *rt, JSScript **rp)
+{
+    js_RemoveRoot(rt, (void *)rp);
 }
 
 JS_NEVER_INLINE JS_PUBLIC_API(void)
 JS_AnchorPtr(void *p)
 {
 }
 
 #ifdef DEBUG
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -1342,26 +1342,25 @@ typedef JSBool
  * Delegate typeof to an object so it can cloak a primitive or another object.
  */
 typedef JSType
 (* JSTypeOfOp)(JSContext *cx, JSObject *obj);
 
 typedef struct JSFreeOp JSFreeOp;
 
 struct JSFreeOp {
-    JSContext   *context;
 #ifndef __cplusplus
     JSRuntime   *runtime;
 #else
   private:
     JSRuntime   *runtime_;
 
   protected:
-    JSFreeOp(JSRuntime *rt, JSContext *cx)
-      : context(cx), runtime_(rt) { }
+    JSFreeOp(JSRuntime *rt)
+      : runtime_(rt) { }
 
   public:
     JSRuntime *runtime() const {
         return runtime_;
     }
 #endif
 };
 
@@ -2984,16 +2983,19 @@ JS_free(JSContext *cx, void *p);
 
 /*
  * A wrapper for js_free(p) that may delay js_free(p) invocation as a
  * performance optimization as specified by the given JSFreeOp instance.
  */
 extern JS_PUBLIC_API(void)
 JS_freeop(JSFreeOp *fop, void *p);
 
+extern JS_PUBLIC_API(JSFreeOp *)
+JS_GetDefaultFreeOp(JSRuntime *rt);    
+
 extern JS_PUBLIC_API(void)
 JS_updateMallocCounter(JSContext *cx, size_t nbytes);
 
 extern JS_PUBLIC_API(char *)
 JS_strdup(JSContext *cx, const char *s);
 
 extern JS_PUBLIC_API(JSBool)
 JS_NewNumberValue(JSContext *cx, double d, jsval *rval);
@@ -3054,40 +3056,52 @@ extern JS_PUBLIC_API(JSBool)
 JS_AddNamedObjectRoot(JSContext *cx, JSObject **rp, const char *name);
 
 extern JS_PUBLIC_API(JSBool)
 JS_AddNamedScriptRoot(JSContext *cx, JSScript **rp, const char *name);
 
 extern JS_PUBLIC_API(JSBool)
 JS_AddNamedGCThingRoot(JSContext *cx, void **rp, const char *name);
 
-extern JS_PUBLIC_API(JSBool)
+extern JS_PUBLIC_API(void)
 JS_RemoveValueRoot(JSContext *cx, jsval *vp);
 
-extern JS_PUBLIC_API(JSBool)
+extern JS_PUBLIC_API(void)
 JS_RemoveStringRoot(JSContext *cx, JSString **rp);
 
-extern JS_PUBLIC_API(JSBool)
+extern JS_PUBLIC_API(void)
 JS_RemoveObjectRoot(JSContext *cx, JSObject **rp);
 
-extern JS_PUBLIC_API(JSBool)
+extern JS_PUBLIC_API(void)
 JS_RemoveScriptRoot(JSContext *cx, JSScript **rp);
 
-extern JS_PUBLIC_API(JSBool)
+extern JS_PUBLIC_API(void)
 JS_RemoveGCThingRoot(JSContext *cx, void **rp);
 
+extern JS_PUBLIC_API(void)
+JS_RemoveValueRootRT(JSRuntime *rt, jsval *vp);
+
+extern JS_PUBLIC_API(void)
+JS_RemoveStringRootRT(JSRuntime *rt, JSString **rp);
+
+extern JS_PUBLIC_API(void)
+JS_RemoveObjectRootRT(JSRuntime *rt, JSObject **rp);
+
+extern JS_PUBLIC_API(void)
+JS_RemoveScriptRootRT(JSRuntime *rt, JSScript **rp);
+
 /* TODO: remove these APIs */
 
 extern JS_FRIEND_API(JSBool)
 js_AddRootRT(JSRuntime *rt, jsval *vp, const char *name);
 
 extern JS_FRIEND_API(JSBool)
 js_AddGCThingRootRT(JSRuntime *rt, void **rp, const char *name);
 
-extern JS_FRIEND_API(JSBool)
+extern JS_FRIEND_API(void)
 js_RemoveRoot(JSRuntime *rt, void *rp);
 
 /*
  * C-compatible version of the Anchor class. It should be called after the last
  * use of the variable it protects.
  */
 extern JS_NEVER_INLINE JS_PUBLIC_API(void)
 JS_AnchorPtr(void *p);
--- a/js/src/jscntxt.h
+++ b/js/src/jscntxt.h
@@ -178,16 +178,57 @@ struct ConservativeGCData
     }
 #endif
 
     bool hasStackToScan() const {
         return !!nativeStackTop;
     }
 };
 
+class FreeOp : public JSFreeOp {
+    bool        shouldFreeLater_;
+    bool        onBackgroundThread_;
+  public:
+    JSContext   *context;
+
+    static FreeOp *get(JSFreeOp *fop) {
+        return static_cast<FreeOp *>(fop);
+    }
+
+    FreeOp(JSRuntime *rt, bool shouldFreeLater, bool onBackgroundThread, JSContext *cx)
+      : JSFreeOp(rt),
+        shouldFreeLater_(shouldFreeLater),
+        onBackgroundThread_(onBackgroundThread),
+        context(cx)
+    {
+    }
+
+    bool shouldFreeLater() const {
+        return shouldFreeLater_;
+    }
+
+    bool onBackgroundThread() const {
+        return onBackgroundThread_;
+    }
+
+    inline void free_(void* p);
+
+    JS_DECLARE_DELETE_METHODS(free_, inline)
+
+    static void staticAsserts() {
+        /*
+         * Check that JSFreeOp is the first base class for FreeOp and we can
+         * reinterpret a pointer to JSFreeOp as a pointer to FreeOp without
+         * any offset adjustments. JSClass::freeOp <-> Class::freeOp depends
+         * on this.
+         */
+        JS_STATIC_ASSERT(offsetof(FreeOp, shouldFreeLater_) == sizeof(JSFreeOp));
+    }
+};
+
 } /* namespace js */
 
 struct JSRuntime : js::RuntimeFriendFields
 {
     /* Default compartment. */
     JSCompartment       *atomsCompartment;
 
     /* List of compartments (protected by the GC lock). */
@@ -500,16 +541,24 @@ struct JSRuntime : js::RuntimeFriendFiel
 
 #ifdef JS_THREADSAFE
     /* These combine to interlock the GC and new requests. */
     PRLock              *gcLock;
 
     js::GCHelperThread  gcHelperThread;
 #endif /* JS_THREADSAFE */
 
+  private:
+    js::FreeOp          defaultFreeOp_;
+
+  public:
+    js::FreeOp *defaultFreeOp() {
+        return &defaultFreeOp_;
+    }
+
     uint32_t            debuggerMutations;
 
     const JSSecurityCallbacks *securityCallbacks;
     JSDestroyPrincipalsOp destroyPrincipals;
 
     /* Structured data callbacks are runtime-wide. */
     const JSStructuredCloneCallbacks *structuredCloneCallbacks;
 
@@ -802,62 +851,26 @@ VersionIsKnown(JSVersion version)
 {
     return VersionNumber(version) != JSVERSION_UNKNOWN;
 }
 
 typedef HashSet<JSObject *,
                 DefaultHasher<JSObject *>,
                 SystemAllocPolicy> BusyArraysSet;
 
-class FreeOp : public JSFreeOp {
-    bool        shouldFreeLater_;
-    bool        onBackgroundThread_;
-  public:
-
-    static FreeOp *get(JSFreeOp *fop) {
-        return static_cast<FreeOp *>(fop);
-    }
-
-    FreeOp(JSRuntime *rt, bool shouldFreeLater, bool onBackgroundThread, JSContext *cx)
-      : JSFreeOp(rt, cx),
-        shouldFreeLater_(shouldFreeLater),
-        onBackgroundThread_(onBackgroundThread)
-    {
-    }
-
-    bool shouldFreeLater() const {
-        return shouldFreeLater_;
-    }
-
-    bool onBackgroundThread() const {
-        return onBackgroundThread_;
+inline void
+FreeOp::free_(void* p) {
+#ifdef JS_THREADSAFE
+    if (shouldFreeLater()) {
+        runtime()->gcHelperThread.freeLater(p);
+        return;
     }
-
-    void free_(void* p) {
-#ifdef JS_THREADSAFE
-        if (shouldFreeLater()) {
-            runtime()->gcHelperThread.freeLater(p);
-            return;
-        }
 #endif
-        runtime()->free_(p);
-    }
-
-    JS_DECLARE_DELETE_METHODS(free_, inline)
-
-    static void staticAsserts() {
-        /*
-         * Check that JSFreeOp is the first base class for FreeOp and we can
-         * reinterpret a pointer to JSFreeOp as a pointer to FreeOp without
-         * any offset adjustments. JSClass::freeOp <-> Class::freeOp depends
-         * on this.
-         */
-        JS_STATIC_ASSERT(offsetof(FreeOp, shouldFreeLater_) == sizeof(JSFreeOp));
-    }
-};
+    runtime()->free_(p);
+}
 
 } /* namespace js */
 
 struct JSContext : js::ContextFriendFields
 {
     explicit JSContext(JSRuntime *rt);
     JSContext *thisDuringConstruction() { return this; }
     ~JSContext();
--- a/js/src/jsgc.cpp
+++ b/js/src/jsgc.cpp
@@ -1265,22 +1265,21 @@ js_AddRootRT(JSRuntime *rt, jsval *vp, c
 
 JS_FRIEND_API(JSBool)
 js_AddGCThingRootRT(JSRuntime *rt, void **rp, const char *name)
 {
     return !!rt->gcRootsHash.put((void *)rp,
                                  RootInfo(name, JS_GC_ROOT_GCTHING_PTR));
 }
 
-JS_FRIEND_API(JSBool)
+JS_FRIEND_API(void)
 js_RemoveRoot(JSRuntime *rt, void *rp)
 {
     rt->gcRootsHash.remove(rp);
-    rt->gcPoke = JS_TRUE;
-    return JS_TRUE;
+    rt->gcPoke = true;
 }
 
 typedef RootedValueMap::Range RootRange;
 typedef RootedValueMap::Entry RootEntry;
 typedef RootedValueMap::Enum RootEnum;
 
 #ifdef DEBUG
 
--- a/js/src/shell/js.cpp
+++ b/js/src/shell/js.cpp
@@ -4089,19 +4089,19 @@ its_convert(JSContext *cx, JSObject *obj
 static void
 its_finalize(JSFreeOp *fop, JSObject *obj)
 {
     jsval *rootedVal;
     if (its_noisy)
         fprintf(gOutFile, "finalizing it\n");
     rootedVal = (jsval *) JS_GetPrivate(obj);
     if (rootedVal) {
-      JS_RemoveValueRoot(fop->context, rootedVal);
-      JS_SetPrivate(obj, NULL);
-      delete rootedVal;
+        JS_RemoveValueRootRT(fop->runtime(), rootedVal);
+        JS_SetPrivate(obj, NULL);
+        delete rootedVal;
     }
 }
 
 static JSClass its_class = {
     "It", JSCLASS_NEW_RESOLVE | JSCLASS_NEW_ENUMERATE | JSCLASS_HAS_PRIVATE,
     its_addProperty,  its_delProperty,  its_getProperty,  its_setProperty,
     (JSEnumerateOp)its_enumerate, (JSResolveOp)its_resolve,
     its_convert,      its_finalize