Bug 37195 – JSC's currentThreadStackBase is not reentrant on some platforms

• Status: CLOSED
• Resolution: FIXED
• Component: JavaScriptCore

[Simon Hausmann]

On some platforms the implementation of currentThreadStackBase() in runtime/Collector.cpp uses global variables, either to store the stack base itself and/or the last thread handle. The latter case is used as an optimization to avoid expensive operating system calls to determine the stack base. However if the function is called simultaenously from multiple threads, access to the global variables needs to be protected.

There are two consequences of the current code:

    1) Either it crashes because of concurrent access to the global variables, causing memory corruption.

    2) Or the thread stack base of a thread different than the current thread is returned, causing the garbage collector to miss out objects to free.

[Simon Hausmann]

Created attachment 52715 [details]
Patch

[Kent Hansen]

(In reply to comment #1)
> Created an attachment (id=52715) [details]
> Patch

Yep, this fixes the crash on Linux.

[Eric Seidel (no email)]

Attachment 52715 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1552337

[Kent Hansen]

(In reply to comment #3)
> Attachment 52715 [details] did not build on mac:
> Build output: http://webkit-commit-queue.appspot.com/results/1552337

Patch needs a guard for the currentThreadStackBaseMutex() definition, so it's not defined on platforms that don't use it (the Mac buildbot treats warnings as errors, and really, we don't want this warning).
As we discussed already, we just need to be careful with the #if OS(UNIX) part... :) Something like

#if (OS(UNIX) && !(OS(DARWIN) || OS(QNX) || OS(SOLARIS) || OS(OPENBSD) || OS(HAIKU) || OS(SYMBIAN))

For QNX, I'd also consider moving the MutexLocker into currentThreadStackBaseQNX() (even though the function is not called from anywhere else currently).

[Olivier Goffart]

The patch is not correct on symbian. As the stackBase will be assigned the value of the first thread, and will not be changed again if this function is called from a different thread.

(I am wondering if it is worth to cache those value at all.  And if it is, maybe a thread local storage would work better?)

[Kent Hansen]

(In reply to comment #4)
> #if (OS(UNIX) && !(OS(DARWIN) || OS(QNX) || OS(SOLARIS) || OS(OPENBSD) ||
> OS(HAIKU) || OS(SYMBIAN))

Hehe, we don't want the OS(QNX) and OS(SYMBIAN) above of course...
This builds on Mac:

#if (OS(UNIX) && !(OS(DARWIN) || OS(HAIKU) || OS(OPENBSD) || OS(SOLARIS))) || OS(WINCE)

static Mutex& currentThreadStackBaseMutex()
{
    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
    return mutex;
}

#endif

[Simon Hausmann]

(In reply to comment #5)
> The patch is not correct on symbian. As the stackBase will be assigned the
> value of the first thread, and will not be changed again if this function is
> called from a different thread.
> 
> (I am wondering if it is worth to cache those value at all.  And if it is,
> maybe a thread local storage would work better?)

Right, the patch only addresses the concurrency issue 1). With that patch there's a chance of getting less crashes, but we'll continue to collect less objects than we could.

[Kent Hansen]

(In reply to comment #5)
> The patch is not correct on symbian. As the stackBase will be assigned the
> value of the first thread, and will not be changed again if this function is
> called from a different thread.

That's a problem with the original patch for Symbian, not this patch.
 
> (I am wondering if it is worth to cache those value at all.  And if it is,
> maybe a thread local storage would work better?)

Yeah, Simon and I were also wondering whether the caching is needed on Symbian. Is it just a case of the Symbian implementation following the pattern of the Unix one (caching the value), or is the system call really that slow on Symbian too? If it is slow, then at least there needs to be a check to see if the thread handle is the same to validate the cached value, like in the Unix branch.
Janne, do you have any insights?

[Simon Hausmann]

Created attachment 52733 [details]
Patch

[WebKit Review Bot]

Attachment 52733 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/runtime/Collector.cpp:535:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Olivier Goffart]

(In reply to comment #7)
> Right, the patch only addresses the concurrency issue 1). With that patch
> there's a chance of getting less crashes, but we'll continue to collect less
> objects than we could.

We would still crash in case 2). Since we will look the memory between the base of the stack of one thread, and the top of the stack of the current thread. That cannot work.

[Kent Hansen]

(In reply to comment #9)
> Created an attachment (id=52733) [details]
> Patch

Builds on Mac now, at least.

[Kent Hansen]

(In reply to comment #11)
> (In reply to comment #7)
> > Right, the patch only addresses the concurrency issue 1). With that patch
> > there's a chance of getting less crashes, but we'll continue to collect less
> > objects than we could.
> 
> We would still crash in case 2). Since we will look the memory between the base
> of the stack of one thread, and the top of the stack of the current thread.
> That cannot work.

Nobody from S60 have responded (Norbert, are you there? :) ), and I can't even find an API reference for RThread and friends, so I don't know what the appropriate fix is.

[Kent Hansen]

Alright, I got some great information from two Symbian guys on IRC.
They believed that the construction of RThread + calling stackInfo() every time is not that expensive. To make the current implementation reentrant, we would have to mutex-lock _and_ call RThread::Id(); whereas not locking and calling stackInfo() would solve the stale cache + reentrancy issue, simplifies the code, and might actually be faster.

[Kent Hansen]

Created attachment 53961 [details]
Patch for Symbian (untested)

Does anyone on Symbian have a chance to test it?
If it works, I'll add the ChangeLog so we can get this committed, then Simon can rebase his patch.

[Simon Hausmann]

Created attachment 54401 [details]
Patch

[Simon Hausmann]

(In reply to comment #16)
> Created an attachment (id=54401) [details]
> Patch

Combined Kent and my patch, so that the return value on Symbian is always thread specific and reentrant.

[WebKit Review Bot]

Attachment 54401 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
JavaScriptCore/runtime/Collector.cpp:535:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Siddharth Mathur]

(In reply to comment #14)
> Alright, I got some great information from two Symbian guys on IRC.
> They believed that the construction of RThread + calling stackInfo() every time
> is not that expensive. To make the current implementation reentrant, we would
> have to mutex-lock _and_ call RThread::Id(); whereas not locking and calling
> stackInfo() would solve the stale cache + reentrancy issue, simplifies the
> code, and might actually be faster.

While the proposed patch is OK for Symbian (simple is good), we should at least add a comment there saying that RThread::StackInfo() involves a call to kernel mode code: 
http://developer.symbian.org/xref/oss/xref/MCL/sf/os/kernelhwsrv/kernel/eka/euser/us_exec.cpp

So, there is a possibility of slowdown because currentThreadStackBase() is called frequently, IIRC.

[Darin Adler]

Comment on attachment 54401 [details]
Patch

Looks OK. Seems to me it would be fine to repeat the single line three times:

    AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);

Rather than having a function for it, though.

[Simon Hausmann]

(In reply to comment #20)
> (From update of attachment 54401 [details])
> Looks OK. Seems to me it would be fine to repeat the single line three times:
> 
>     AtomicallyInitializedStatic(Mutex&, mutex = *new Mutex);
> 
> Rather than having a function for it, though.

Good idea! I'll change that before landing. Thanks for the review.

[Simon Hausmann]

Committed r58393: <http://trac.webkit.org/changeset/58393>

[WebKit Review Bot]

http://trac.webkit.org/changeset/58393 might have broken Tiger Intel Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/58393
http://trac.webkit.org/changeset/58394
http://trac.webkit.org/changeset/58395

[Simon Hausmann]

Revision r58393 cherry-picked into qtwebkit-2.0 with commit 76fc13db1b9a2116d6bee35c0e2c8fbfd940cd34