538702 - Thread synchronization for conservative stack scanning

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Gregor Wagner [:gwagner]]

Since the the debugging process for bug 516832 became too complicated because of stack-scan crashes and synchronization crashes, I removed all the stack-scan code. This bug is just for the thread synchronization.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: WIP

Work in progress.
Works on Mac, random xpcshell crashes on Win.

Comment 2

[Gregor Wagner [:gwagner]]

Attachment: patch

Passes tryserver for all platforms.
Thread synchronization is enabled in DEBUG mode.

Comment 3

[Andreas Gal :gal]

Comment on attachment 420900 [details] [diff] [review]
patch


>+#if defined(XP_UNIX) && !defined(__APPLE__)
>+static struct timespec onemillisec = {0, 1000000L};
>+#endif

OneMilliSec. Also maybe move closer to where do you need it.

>+#if defined(XP_UNIX) && !defined(__APPLE__)
>+
>+static pthread_key_t key;
>+static pthread_once_t key_once = PTHREAD_ONCE_INIT;
>+

SuspendSignalHandler

>+static void suspendSignalHandler(int signo, siginfo_t *info, void *extra)
>+{
>+    JSThread *thread = (JSThread*)pthread_getspecific(key);
>+    JS_ASSERT(thread != 0);
>+    JS_ASSERT((thread->suspended & JSTHREAD_SUSPENDED) == 0);
>+
>+    thread->suspended |= JSTHREAD_SUSPENDED;
>+    pthread_cond_signal(&thread->suspendResumeCV);
>+    while (thread->suspended & JSTHREAD_SUSPENDED)
>+    {
>+        pthread_yield();
>+    }

Don't over-brace single line consequence.

>+    thread->suspended |= JSTHREAD_RESUMED;
>+    pthread_cond_signal(&thread->suspendResumeCV);
>+}
>+
>+static void 
>+initGCSupport()

InitGCSupport, but thats a cheesy name too. InstallSuspendSignalHandler?

>+{
>+    struct sigaction sigactUsr2;
>+    
>+    sigactUsr2.sa_flags = SA_SIGINFO;
>+    sigactUsr2.sa_sigaction = suspendSignalHandler;
>+    sigaction (SIGUSR2, &sigactUsr2, NULL);
>+}

Use short names inside functions, sigactUsr2 is too long, just sa or something.

>+
>+static void
>+makeKey()
>+{
>+    (void) pthread_key_create(&key, NULL);
>+}

Better name? And MakeKey. 

>+
>+#endif
>+
> static JSThread *
> NewThread(jsword id)
> {
>     JS_ASSERT(js_CurrentThreadId() == id);
>     JSThread *thread = (JSThread *) js_calloc(sizeof(JSThread));
>     if (!thread)
>         return NULL;
>     JS_INIT_CLIST(&thread->contextList);
>     thread->id = id;
>+#ifdef DEBUG
>+# ifdef __APPLE__
>+    thread->handle = pthread_mach_thread_np(pthread_self());
>+# elif WIN32
>+    HANDLE realHandle = 0;

"handle" ?

>+    DuplicateHandle(GetCurrentProcess(), GetCurrentThread(), 
>+        GetCurrentProcess(), &realHandle, 0, TRUE, DUPLICATE_SAME_ACCESS);

Align GetCurrentProcesses here, indent further.

>+    thread->handle = HANDLE2WORD(realHandle);
>+# elif defined(XP_UNIX) && !defined(__APPLE__)
>+    thread->handle = pthread_self();
>+    (void) pthread_once(&key_once, makeKey);
>+    (void) pthread_setspecific(key, thread);
>+    initGCSupport();
>+    pthread_mutex_init(&thread->suspendResumeMutex,NULL);
>+    pthread_cond_init(&thread->suspendResumeCV,NULL);
>+# else
>+#  error Unsupported platform.
>+# endif
>+#endif
>     thread->data.init();
>     return thread;
> }
> 
> static void
> DestroyThread(JSThread *thread)
> {
>     /* The thread must have zero contexts. */
>     JS_ASSERT(JS_CLIST_IS_EMPTY(&thread->contextList));
>     JS_ASSERT(!thread->titleToShare);
>     thread->data.finish();
>+#ifdef DEBUG
>+# ifdef WIN32
>+    CloseHandle(WORD2HANDLE(thread->handle));
>+# endif
>+#endif
>     js_free(thread);
> }
> 
>+#if defined(XP_UNIX) && !defined(__APPLE__)
>+typedef struct user_regs_struct user_regs_struct;
>+#endif
>+
>+void
>+JSThread::suspend()
>+{
>+#ifdef __APPLE__
>+    thread_suspend(mach_port_t(handle));
>+#elif WIN32
>+    SuspendThread(WORD2HANDLE(handle));
>+#elif defined(XP_UNIX) && !defined(__APPLE__)
>+    JS_ASSERT((suspended & JSTHREAD_SUSPENDED) == 0);
>+    pthread_kill(handle, SIGUSR2);
>+    suspendTest();

Test?

>+#else
>+ #error Unsupported platform.
>+#endif
>+}
>+
>+void
>+JSThread::resume()
>+{
>+#ifdef __APPLE__
>+    thread_resume(mach_port_t(handle));
>+#elif WIN32
>+    ResumeThread(WORD2HANDLE(handle));
>+#elif defined(XP_UNIX) && !defined(__APPLE__)
>+    JS_ASSERT(suspended & JSTHREAD_SUSPENDED);
>+    suspended &= ~JSTHREAD_SUSPENDED;
>+    resumeTest();

Test?

>+#else
>+#error Unsupported platform.
>+#endif
>+}
>+
>+#if defined(XP_UNIX) && !defined(__APPLE__)
>+void 
>+JSThread::suspendTest()
>+{
>+    pthread_mutex_lock(&suspendResumeMutex);
>+    while ((suspended & JSTHREAD_SUSPENDED) == 0)
>+    {

Don't overbrace. Even if, it would be in the line one up.

>+        pthread_cond_timedwait(&suspendResumeCV, &suspendResumeMutex, &onemillisec);
>+    }
>+    pthread_mutex_unlock(&suspendResumeMutex);
>+}
>+
>+void 
>+JSThread::resumeTest()

Do you mean resumeWait?

>+{
>+    pthread_mutex_lock(&suspendResumeMutex);
>+    while ((suspended & JSTHREAD_RESUMED) == 0)
>+    {
>+        pthread_cond_timedwait(&suspendResumeCV, &suspendResumeMutex, &onemillisec);
>+    }
>+    pthread_mutex_unlock(&suspendResumeMutex);
>+    suspended &= ~JSTHREAD_RESUMED;
>+}
>+#endif
>+
> JSThread *
> js_CurrentThread(JSRuntime *rt)
> {
>     jsword id = js_CurrentThreadId();
>     JS_LOCK_GC(rt);
> 
>     /*
>      * We must not race with a GC that accesses cx->thread for JSContext
>@@ -303,17 +467,29 @@ thread_purger(JSDHashTable *table, JSDHa
>     return JS_DHASH_NEXT;
> }
> 
> static JSDHashOperator
> thread_marker(JSDHashTable *table, JSDHashEntryHdr *hdr, uint32 /* index */,
>               void *arg)
> {
>     JSThread *thread = ((JSThreadsHashEntry *) hdr)->thread;
>-    thread->data.mark((JSTracer *) arg);
>+    JSTracer *trc = (JSTracer *)arg; 
>+    if (!trc->context->runtime->gcReleaseRoots) {
>+#ifdef DEBUG
>+        if (trc->context->thread != thread)
>+            thread->suspend();
>+#endif
>+        thread->data.mark(trc);
>+#ifdef DEBUG
>+        if (trc->context->thread != thread)
>+            thread->resume();
>+#endif
>+    }
>+    
>     return JS_DHASH_NEXT;
> }
> 
> #endif /* JS_THREADSAFE */
> 
> JSThreadData *
> js_CurrentThreadData(JSRuntime *rt)
> {
>diff -r d0ad088ca59f js/src/jscntxt.h
>--- a/js/src/jscntxt.h	Fri Jan 08 18:16:40 2010 -0800
>+++ b/js/src/jscntxt.h	Sat Jan 09 09:43:00 2010 -0800
>@@ -430,35 +430,51 @@ struct JSThreadData {
>     void finish();
>     void mark(JSTracer *trc);
>     void purge(JSContext *cx);
>     void purgeGCFreeLists();
> };
> 
> #ifdef JS_THREADSAFE
> 
>+#define JSTHREAD_SUSPENDED 0x40    /* thread has been suspended */
>+#define JSTHREAD_RESUMED 0x80    /* thread has been resumed */
>+

I tink we can move the defines into the CPP file and make const int?

> /*
>  * Structure uniquely representing a thread.  It holds thread-private data
>  * that can be accessed without a global lock.
>  */
> struct JSThread {
>     /* Linked list of all contexts in use on this thread. */
>     JSCList             contextList;
> 
>     /* Opaque thread-id, from NSPR's PR_GetCurrentThread(). */
>     jsword              id;
>+    jsword              handle;
>+    volatile jsuword    suspended;
>+#if defined(XP_UNIX) && !defined(__APPLE__) 
>+    pthread_mutex_t suspendResumeMutex;
>+    pthread_cond_t suspendResumeCV;

Seriously, indentation =)

>+#endif
> 
>     /* Indicates that the thread is waiting in ClaimTitle from jslock.cpp. */
>     JSTitle             *titleToShare;
> 
>     /*
>      * Thread-local version of JSRuntime.gcMallocBytes to avoid taking
>      * locks on each JS_malloc.
>      */
>     ptrdiff_t           gcThreadMallocBytes;
>+    
>+    void suspend();
>+    void resume();
>+#if defined(XP_UNIX) && !defined(__APPLE__)
>+    void resumeTest();
>+    void suspendTest();
>+#endif
> 
>     /*
>      * Deallocator task for this thread.
>      */
>     JSFreePointerListTask *deallocatorTask;
> 
>     /* Factored out of JSThread for !JS_THREADSAFE embedding in JSRuntime. */
>     JSThreadData        data;
>@@ -567,16 +583,17 @@ struct JSRuntime {
>     uint32              gcEmptyArenaPoolLifespan;
>     uint32              gcLevel;
>     uint32              gcNumber;
>     JSTracer            *gcMarkingTracer;
>     uint32              gcTriggerFactor;
>     size_t              gcTriggerBytes;
>     volatile JSBool     gcIsNeeded;
>     volatile JSBool     gcFlushCodeCaches;
>+    volatile JSBool     gcReleaseRoots;
> 
>     /*
>      * NB: do not pack another flag here by claiming gcPadding unless the new
>      * flag is written only by the GC thread.  Atomic updates to packed bytes
>      * are not guaranteed, so stores issued by one thread may be lost due to
>      * unsynchronized read-modify-write cycles on other threads.
>      */
>     JSPackedBool        gcPoke;
>diff -r d0ad088ca59f js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp	Fri Jan 08 18:16:40 2010 -0800
>+++ b/js/src/jsgc.cpp	Sat Jan 09 09:43:00 2010 -0800
>@@ -3100,16 +3100,19 @@ js_GC(JSContext *cx, JSGCInvocationKind 
> #endif
> 
>     /*
>      * Mark phase.
>      */
>     JS_TRACER_INIT(&trc, cx, NULL);
>     rt->gcMarkingTracer = &trc;
>     JS_ASSERT(IS_GC_MARKING_TRACER(&trc));
>+    
>+    if (gckind == GC_LAST_CONTEXT)
>+        rt->gcReleaseRoots = true;
> 
> #ifdef DEBUG
>     for (a = rt->gcDoubleArenaList.head; a; a = a->prev)
>         JS_ASSERT(!a->hasMarkedDoubles);
> #endif
> 
>     js_TraceRuntime(&trc, keepAtoms);
>     js_MarkScriptFilenames(rt, keepAtoms);

Comment 4

[Andreas Gal :gal]

Looks pretty cool otherwise. Can you do the changes and then we should probably consult with igor.

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 6

[Igor Bukanov]

My concern with the current patch is that it may suspend a thread that is inside the GC lock. Then, if any marking code would directly or indirectly call code that calls JS_LOCK_GC (which is allowed by the current GC model), we would get a deadlock. A similar problem may happen with XPConnect which have a few locks on its own that IIRC is also taken during the marking phase of the GC.

Also, the problem with the original approach (that just checked threads inside JS requests) was due to JS_SuspendRequest. The stack for such threads has to be scanned yet they are not suspended. But such threads must not produce more GC roots. Moreover, an all relevant platforms AFAIK OS never releases CPU pages allocated for the stack until the thread finishes. Thus taking the stack pointer at an arbitrary moment and scanning for roots the space between the thread base and that pointer is safe.

Comment 7

[Andreas Gal :gal]

Igor, any ideas how to address the locking/deadlock issue?

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> Igor, any ideas how to address the locking/deadlock issue?

The question is why we need to suspend a thread to mark its stack? If the thread is running during the GC, then it is either outside the request, when it should not have any roots, or it called JS_SuspendRequest. In the latter case the thread cannot generate new GC roots to scan or it would violate the JS request model. 

Thus, for such thread, its SP can be snapshot at an arbitrary moment, rounded to the nearest CPU page boundary and the space between the stack base and that SP is scanned. This is safe since OS never release native stack CPU pages and only that space may contain any GC roots. Thus at worst the root scanner would need to check unused pages of the native stack. It would not miss any roots.

Comment 9

[Igor Bukanov]

If the above idea of not suspending threads is unsound, the the deadlock can be avoided if the GC would not mark GC things discovered on the native stack. Rather it would set them aside so the marking can be done later after the GC resume all the native threads. That "set aside" can be trivially implemented using the same mechanism that the GC uses to avoid using too much native stack during recursive scanning of the  GC thing graph.

Comment 10

[Gregor Wagner [:gwagner]]

I was also thinking of a "marking queue" or stack as an optimization for conservative stack scanning but I guess we need it earlier. We should discuss this in the  stack scanning bug or even in a new bug. Are there any other problems with just the thread synchronization part?

Comment 11

[Gregor Wagner [:gwagner]]

Attachment: patch

fix mutex and condvar destruction.

Comment 12

[Gregor Wagner [:gwagner]]

Attachment: patch

refresh

Comment 13

[Gregor Wagner [:gwagner]]

Attachment: update

Comment 14

[Gregor Wagner [:gwagner]]

Passes tryserver with unrelated? orange on Win unit test.
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1266977080.1266989310.32680.gz

Comment 15

[Igor Bukanov]

Comment on attachment 428596 [details] [diff] [review]
update

>diff -r 1b5ca6cc5ce8 js/src/jscntxt.cpp
>+static void 
>+SuspendSignalHandler(int signo, siginfo_t *info, void *extra)
>+{
...
>+    while (thread->suspended & JSTHREAD_SUSPENDED)
>+        pthread_yield();


This does not put truly suspend the thread. According to the manual for pthread_yield "the thread is placed at the end of the run queue". So there is chance that, when the stack is scanned, the thread will wake up. I suppose a better alternative here would be to use a true waiting mutex.  


> static JSThread *
> NewThread(jsword id)
> {
...
>+# elif defined(XP_UNIX) && !defined(__APPLE__)
>+    thread->handle = pthread_self();
>+    (void) pthread_once(&key_once, CreateThreadLookupKey);
>+    (void) pthread_setspecific(key, thread);
>+    InstallSuspendSignalHandler();

The signal handler is per process. So InstallSuspendSignalHandler should not be called for each thread. Also, using a user signal is a nice hack, but it may interfere with other code that use the same signal. Granted, now we have out-of-process plugins. That removes a lot of potential other users of user signals. Still non-browser embeddings or addons using some native libraries may hit it. As a workaround for that I suggest to install the signal only temporary when we about to suspend non-GC threads and then restore the handler back.

>+# else
>+#  error Unsupported platform.

That should be #error "Unsupported platform." Fix that here and everywhere.

>+void 
>+JSThread::suspendWait()
>+{
>+    while ((suspended & JSTHREAD_SUSPENDED) == 0)
>+        pthread_cond_timedwait(&suspendResumeCV, &suspendResumeMutex, &onemillisec);

Comment here why it is necessary to use a loop with 1ms wait instead of just unconditional wait.

> static JSDHashOperator
> thread_marker(JSDHashTable *table, JSDHashEntryHdr *hdr, uint32 /* index */,
>               void *arg)
> {
>     JSThread *thread = ((JSThreadsHashEntry *) hdr)->thread;
>-    thread->data.mark((JSTracer *) arg);

thread->data.mark must not be called when the thread is suspended. The suspend is only necessary for the conservative stack scanning. Thus keep thread->data.mark as is and do only conservative scanning inside suspend/resume.

Also, we do not need to suspend threads in requests. They will be waiting in any case. Only threads outside requests that call JSSuspendRequest should be suspended. This will help to mitigate that user-signal problem above. 

But in general the signal hack on Linux feels like just a hack. It is fine to use it temporary but we should try to avoid it since Linux dos not support suspending threads naively. A simple way to do that would be for any thread that calls JSSuspendRequest to scan its stack from base until the end of the CPU page that contains JSSuspendRequest call. A better way would be to add an alternative to JSSuspendRequest that takes a callback that will be called outside the JS request. This way we can accurately record the stack position.

Comment 16

[Andreas Gal :gal]

> The signal handler is per process. So InstallSuspendSignalHandler should not be
> called for each thread. Also, using a user signal is a nice hack, but it may
> interfere with other code that use the same signal. Granted, now we have
> out-of-process plugins. That removes a lot of potential other users of user
> signals. Still non-browser embeddings or addons using some native libraries may
> hit it. As a workaround for that I suggest to install the signal only temporary
> when we about to suspend non-GC threads and then restore the handler back.

We are the platform. We set the environment. If we decide we use SIGUSR1, then we use SIGUSR1. If an embedding tries to interfere, we die. I think that's acceptable. Same goes for plugins. We make a lot of other changes to the environment as well. We set the FPU into a specific mode w.r.t. to rounding and NaN reporting. If a plugin changes that we used to crash, now we just flip those bits back. We redirect SIGFPE for that.

Comment 17

[Brendan Eich [:brendan]]

Signals are bad medicine, the SIGFPU story proves this. We could try to claim SIGUSR1 but better if we don't use signals at all, or if we arm and handle one only temporarily.

Why are we not using any js/src/threads (bug 507718) or NSPR veneer at all? At the least we should minimize #ifdefs where there's no platform-specific advantage and the APIs are essentially cross-platform.

/be

Comment 18

[Andreas Gal :gal]

You mean PR_Interrupt? It has to be doing the same thing internally. Linux does not support asynchronous thread suspending.

Comment 19

[Gregor Wagner [:gwagner]]

Brendan what do you mean with "NSPR veneer"? I don't see how PR_Interrupt could help here. Is there a way to register our own interrupt handler?

The linux implementation is based on the nspr thread synchronization. Fixing the suggestions with the signal handler activation for each GC and clean up the signal handler code shouldn't be too hard.

Comment 20

[Brendan Eich [:brendan]]

Summoning Wan-Teh for better NSPR advice than I can render from old memories.

/be

Comment 21

[Wan-Teh Chang]

Brendan: what is the question?

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> Brendan: what is the question?

Sorry, the question is how to suspend and scan (in the conservative garbage collection sense) a thread's stack, short of hacking platform-dependent code (e.g., sending SIGUSR1 to the thread after arming a handler for that signal).

/be

Comment 23

[Wan-Teh Chang]

NSPR used to support that when Netscape ran its port of Sun JVM over
NSPR.  The functions are PR_SuspendAll and PR_ResumeAll.  They are
implemented using SIGUSR2 (and possibly SIGUSR1 also).  I'm not sure
if the code still works.

Pthreads doesn't have a function that suspends a thread.  Some
implementations of pthreads do have such a function as an
extension.

Comment 24

[Gregor Wagner [:gwagner]]

Attachment: WiP

Inspired by bug 507718 I modified this patch (still not done and not tested on Windows)
I also tried to address previous comments with this patch:
- install signal handler on linux only for GC
- send signal to suspended thread instead of pthread_yield

I am already using the approach Wan Teh is mentioning.

Comment 25

[Gregor Wagner [:gwagner]]

Attachment: update

Comment 26

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 27

[Igor Bukanov]

(In reply to comment #26)
> Created an attachment (id=429914) [details]
> patch

I am not sure that proliferation of files with platform-specific implementation is a right thing here. IMO it makes it harder to maintain code since updating several files requires more efforts than updating one containing platform-specific code. Plus such layers create a false sense of having a working thread suspend. I would prefer to see the Linux deficiency clear and loud in the code so other parts would not even think about calling thread->suspend(). Other opinions?

That aside the patch should not suspend threads outside the requests since they cannot contain JS values on the stack. They also can be about to be destroyed at the moment patch tries to suspend them. This could be problematic especially on Linux. Plus the threads inside the request are already suspended waiting for the GC to finish. So we only need to suspend threads that called JS_SuspendRequest from the inside the GC request.

Comment 28

[Gregor Wagner [:gwagner]]

(In reply to comment #27)
> (In reply to comment #26)
> > Created an attachment (id=429914) [details] [details]
> > patch
> 
> I am not sure that proliferation of files with platform-specific implementation
> is a right thing here. IMO it makes it harder to maintain code since updating
> several files requires more efforts than updating one containing
> platform-specific code. Plus such layers create a false sense of having a
> working thread suspend. I would prefer to see the Linux deficiency clear and
> loud in the code so other parts would not even think about calling
> thread->suspend(). Other opinions?

I don't think the code gets cleaner if we add a few hundred LOC to an already huge jsgc.cpp file. To quote Brendan: "It would be great to avoid ifdef hell."
But that can be changed...

> 
> That aside the patch should not suspend threads outside the requests since they
> cannot contain JS values on the stack. They also can be about to be destroyed
> at the moment patch tries to suspend them. This could be problematic especially
> on Linux. Plus the threads inside the request are already suspended waiting for
> the GC to finish. So we only need to suspend threads that called
> JS_SuspendRequest from the inside the GC request.

Is it safe to use one of the outstandingRequests or requestDepth variables to determine if a thread has to be suspended or do I have to add a new one for this?

Comment 29

[Brendan Eich [:brendan]]

(In reply to comment #28)
> > I am not sure that proliferation of files with platform-specific implementation
> > is a right thing here. IMO it makes it harder to maintain code since updating
> > several files requires more efforts than updating one containing
> > platform-specific code.

Note for later this point.

> > Plus such layers create a false sense of having a
> > working thread suspend. I would prefer to see the Linux deficiency clear and
> > loud in the code so other parts would not even think about calling
> > thread->suspend(). Other opinions?
> 
> I don't think the code gets cleaner if we add a few hundred LOC to an already
> huge jsgc.cpp file. To quote Brendan: "It would be great to avoid ifdef hell."
> But that can be changed...

I agree with Igor we do not want a facade of an internal API that is just going to fall over if used more widely. Attractive nuisance.

#ifdef hell is no fun either, but these are evils competing for "lesser" status.

Why not put the platform-specific code in a separate file but label/name it so as to warn kids away from the nuisance. You could even do it via inline methods in a .h file that is not exported.

> Is it safe to use one of the outstandingRequests or requestDepth variables to
> determine if a thread has to be suspended or do I have to add a new one for
> this?

JS_SuspendRequest clearly uses outstandingRequests precisely for this purpose, no more counters needed. It was added as a bug fix, see bug 401687.

/be

Comment 30

[Igor Bukanov]

I suggest not to fix this bug. Rather for a thread that have a suspended request the stack scanner should just scan until the SP on the moment of JS_SuspendRequest call. This cannot miss any possible roots. 

The only problem is that if JS_SuspendRequest would be called through a series of helpers so the scanner may end up scanning more memory than necessary. But it would just increase the chance of false positives, it would not add memory-safety worries since the OSes that we target never release the CPU pages that the stack uses. And with the bug 551680 and its followups fixed the extra false positives would not exist for the Firefox where they would inevitably exist when using the SP of the suspended thread.

Gregor, I can update your patch 516832 with the necessary changes.

Comment 31

[Gregor Wagner [:gwagner]]

That's a deja vu. We removed the thread synchronization code and realized that we might miss roots from threads that don't get suspended. So I put it in again. I am happy if the stack scanning works without the suspend-resume code. I couldn't follow the discussion about the new SuspendRequest during the last week(s). What changed that it works now for stack scanning?

Igor, feel free to update the patch.

Comment 32

[Igor Bukanov]

(In reply to comment #31)
> That's a deja vu. We removed the thread synchronization code and realized that
> we might miss roots from threads that don't get suspended. So I put it in
> again. I am happy if the stack scanning works without the suspend-resume code.
> I couldn't follow the discussion about the new SuspendRequest during the last
> week(s). What changed that it works now for stack scanning?

IIRC we have never added the native stack pointer capturing code to JS_SuspendRequest.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

(In reply to Igor Bukanov from comment #30)
> I suggest not to fix this bug.

WONTFIX then? :-)

Comment 41

[Igor Bukanov]

(In reply to Ryan VanderMeulen from comment #40)
> (In reply to Igor Bukanov from comment #30)
> > I suggest not to fix this bug.
> 
> WONTFIX then? :-)

This

Comment 42

[Igor Bukanov]

This was addressed in another bug