bug 590533 - InvokeOperationCallback should yield when the is cancelled. r=gal

js/src/jsapi-tests/Makefile.in

@@ -49,6 +49,7 @@ PROGRAM         = jsapi-tests$(BIN_SUFFIX)
 CPPSRCS = \
   tests.cpp \
   selfTest.cpp \
+  testBug604087.cpp \
   testClassGetter.cpp \
   testCloneScript.cpp \
   testConservativeGC.cpp \
@@ -69,7 +70,7 @@ CPPSRCS = \
   testSameValue.cpp \
   testScriptObject.cpp \
   testSetPropertyWithNativeGetterStubSetter.cpp \
-  testBug604087.cpp \
+  testThreadGC.cpp \
   testThreads.cpp \
   testTrap.cpp \
   testUTF8.cpp \

js/src/jsapi-tests/testThreadGC.cpp

@@ -0,0 +1,195 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
+ * vim: set ts=8 sw=4 et tw=99:
+ */
+
+#ifdef JS_THREADSAFE
+
+#include "tests.h"
+#include "prthread.h"
+
+#include "jscntxt.h"
+
+/*
+ * We test that if a GC callback cancels the GC on a child thread the GC can
+ * still proceed on the main thread even if the child thread continue to
+ * run uninterrupted.
+ */
+
+struct SharedData {
+    enum ChildState {
+        CHILD_STARTING,
+        CHILD_RUNNING,
+        CHILD_DONE,
+        CHILD_ERROR
+    };
+
+    JSRuntime   *const runtime;
+    PRThread    *const mainThread;
+    PRLock      *const lock;
+    PRCondVar   *const signal;
+    ChildState  childState;
+    bool        childShouldStop;
+    JSContext   *childContext;
+
+    SharedData(JSRuntime *rt, bool *ok)
+      : runtime(rt),
+        mainThread(PR_GetCurrentThread()),
+        lock(PR_NewLock()),
+        signal(lock ? PR_NewCondVar(lock) : NULL),
+        childState(CHILD_STARTING),
+        childShouldStop(false),
+        childContext(NULL)
+    {
+        JS_ASSERT(!*ok);
+        *ok = !!signal;
+    }
+
+    ~SharedData() {
+        if (signal)
+            PR_DestroyCondVar(signal);
+        if (lock)
+            PR_DestroyLock(lock);
+    }
+};
+
+static SharedData *shared;
+
+static JSBool
+CancelNonMainThreadGCCallback(JSContext *cx, JSGCStatus status)
+{
+    return status != JSGC_BEGIN || PR_GetCurrentThread() == shared->mainThread;
+}
+
+static JSBool
+StopChildOperationCallback(JSContext *cx)
+{
+    bool shouldStop;
+    PR_Lock(shared->lock);
+    shouldStop = shared->childShouldStop;
+    PR_Unlock(shared->lock);
+    return !shouldStop;
+}
+
+static JSBool
+NotifyMainThreadAboutBusyLoop(JSContext *cx, uintN argc, jsval *vp)
+{
+    PR_Lock(shared->lock);
+    JS_ASSERT(shared->childState == SharedData::CHILD_STARTING);
+    shared->childState = SharedData::CHILD_RUNNING;
+    shared->childContext = cx;
+    PR_NotifyCondVar(shared->signal);
+    PR_Unlock(shared->lock);
+
+    return true;
+}
+
+static void
+ChildThreadMain(void *arg)
+{
+    JS_ASSERT(!arg);
+    bool error = true;
+    JSContext *cx = JS_NewContext(shared->runtime, 8192);
+    if (cx) {
+        JS_SetOperationCallback(cx, StopChildOperationCallback);
+        JSAutoRequest ar(cx);
+        JSObject *global = JS_NewCompartmentAndGlobalObject(cx, JSAPITest::basicGlobalClass(),
+                                                            NULL);
+        if (global) {
+            JS_SetGlobalObject(cx, global);
+            if (JS_InitStandardClasses(cx, global) &&
+                JS_DefineFunction(cx, global, "notify", NotifyMainThreadAboutBusyLoop, 0, 0)) {
+
+                jsval rval;
+                static const char code[] = "var i = 0; notify(); for (var i = 0; ; ++i);";
+                JSBool ok = JS_EvaluateScript(cx, global, code, strlen(code),
+                                              __FILE__, __LINE__, &rval);
+                if (!ok && !JS_IsExceptionPending(cx)) {
+                    /* Evaluate should only return via the callback cancellation. */
+                    error = false;
+                }
+            }
+        }
+    }
+
+    PR_Lock(shared->lock);
+    shared->childState = error ? SharedData::CHILD_DONE : SharedData::CHILD_ERROR;
+    shared->childContext = NULL;
+    PR_NotifyCondVar(shared->signal);
+    PR_Unlock(shared->lock);
+
+    if (cx)
+        JS_DestroyContextNoGC(cx);
+}
+
+BEGIN_TEST(testThreadGC_bug590533)
+    {
+        /*
+         * Test the child thread busy running while the current thread calls
+         * the GC both with JSRuntime->gcIsNeeded set and unset.
+         */
+        bool ok = TestChildThread(true);
+        CHECK(ok);
+        ok = TestChildThread(false);
+        CHECK(ok);
+        return ok;
+    }
+
+    bool TestChildThread(bool setGCIsNeeded)
+    {
+        bool ok = false;
+        shared = new SharedData(rt, &ok);
+        CHECK(ok);
+
+        JSGCCallback oldGCCallback = JS_SetGCCallback(cx, CancelNonMainThreadGCCallback);
+
+        PRThread *thread =
+            PR_CreateThread(PR_USER_THREAD, ChildThreadMain, NULL,
+                            PR_PRIORITY_NORMAL, PR_LOCAL_THREAD, PR_JOINABLE_THREAD, 0);
+        if (!thread)
+            return false;
+
+        PR_Lock(shared->lock);
+        while (shared->childState == SharedData::CHILD_STARTING)
+            PR_WaitCondVar(shared->signal, PR_INTERVAL_NO_TIMEOUT);
+        JS_ASSERT(shared->childState != SharedData::CHILD_DONE);
+        ok = (shared->childState == SharedData::CHILD_RUNNING);
+        PR_Unlock(shared->lock);
+
+        CHECK(ok);
+
+        if (setGCIsNeeded) {
+            /*
+             * Use JS internal API to set the GC trigger flag after we know
+             * that the child is in a request and is about to run an infinite
+             * loop. Then run the GC with JSRuntime->gcIsNeeded flag set.
+             */
+            js::AutoLockGC lock(rt);
+            js::TriggerGC(rt);
+        }
+
+        JS_GC(cx);
+
+        PR_Lock(shared->lock);
+        shared->childShouldStop = true;
+        while (shared->childState == SharedData::CHILD_RUNNING) {
+            JS_TriggerOperationCallback(shared->childContext);
+            PR_WaitCondVar(shared->signal, PR_INTERVAL_NO_TIMEOUT);
+        }
+        JS_ASSERT(shared->childState != SharedData::CHILD_STARTING);
+        ok = (shared->childState == SharedData::CHILD_DONE);
+        PR_Unlock(shared->lock);
+
+        JS_SetGCCallback(cx, oldGCCallback);
+
+        PR_JoinThread(thread);
+
+        delete shared;
+        shared = NULL;
+
+        return true;
+    }
+
+
+END_TEST(testThreadGC_bug590533)
+
+#endif

js/src/jscntxt.cpp

@@ -1845,9 +1845,9 @@ js_InvokeOperationCallback(JSContext *cx)
     JS_ASSERT(td->interruptFlags != 0);
 
     /*
-     * Reset the callback counter first, then yield. If another thread is racing
-     * us here we will accumulate another callback request which will be
-     * serviced at the next opportunity.
+     * Reset the callback counter first, then run GC and yield. If another
+     * thread is racing us here we will accumulate another callback request
+     * which will be serviced at the next opportunity.
      */
     JS_LOCK_GC(rt);
     td->interruptFlags = 0;
@@ -1856,13 +1856,6 @@ js_InvokeOperationCallback(JSContext *cx)
 #endif
     JS_UNLOCK_GC(rt);
 
-    /*
-     * Unless we are going to run the GC, we automatically yield the current
-     * context every time the operation callback is hit since we might be
-     * called as a result of an impending GC, which would deadlock if we do
-     * not yield. Operation callbacks are supposed to happen rarely (seconds,
-     * not milliseconds) so it is acceptable to yield at every callback.
-     */
     if (rt->gcIsNeeded) {
         js_GC(cx, GC_NORMAL);
 
@@ -1879,10 +1872,19 @@ js_InvokeOperationCallback(JSContext *cx)
             return false;
         }
     }
+    
 #ifdef JS_THREADSAFE
-    else {
-        JS_YieldRequest(cx);
-    }
+    /*
+     * We automatically yield the current context every time the operation
+     * callback is hit since we might be called as a result of an impending
+     * GC on another thread, which would deadlock if we do not yield.
+     * Operation callbacks are supposed to happen rarely (seconds, not
+     * milliseconds) so it is acceptable to yield at every callback.
+     *
+     * As the GC can be canceled before it does any request checks we yield
+     * even if rt->gcIsNeeded was true above. See bug 590533.
+     */
+    JS_YieldRequest(cx);
 #endif
 
     JSOperationCallback cb = cx->operationCallback;