Bug 37195 - JSC's currentThreadStackBase is not reentrant on some platforms
Summary: JSC's currentThreadStackBase is not reentrant on some platforms
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Simon Hausmann
URL:
Keywords: Qt
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-04-07 01:53 PDT by Simon Hausmann
Modified: 2010-04-29 00:15 PDT (History)
10 users (show)

See Also:


Attachments
Patch (2.48 KB, patch)
2010-04-07 01:55 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff
Patch (2.58 KB, patch)
2010-04-07 06:29 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff
Patch for Symbian (untested) (874 bytes, patch)
2010-04-21 08:49 PDT, Kent Hansen
no flags Details | Formatted Diff | Diff
Patch (2.92 KB, patch)
2010-04-27 02:07 PDT, Simon Hausmann
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2010-04-07 01:53:07 PDT
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.
Comment 1 Simon Hausmann 2010-04-07 01:55:14 PDT
Created attachment 52715 [details]
Patch
Comment 2 Kent Hansen 2010-04-07 02:12:32 PDT
(In reply to comment #1)
> Created an attachment (id=52715) [details]
> Patch

Yep, this fixes the crash on Linux.
Comment 3 Eric Seidel (no email) 2010-04-07 03:02:41 PDT
Attachment 52715 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1552337
Comment 4 Kent Hansen 2010-04-07 05:44:09 PDT
(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).
Comment 5 Olivier Goffart 2010-04-07 05:55:10 PDT
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?)
Comment 6 Kent Hansen 2010-04-07 06:10:40 PDT
(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
Comment 7 Simon Hausmann 2010-04-07 06:11:34 PDT
(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.
Comment 8 Kent Hansen 2010-04-07 06:16:49 PDT
(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?
Comment 9 Simon Hausmann 2010-04-07 06:29:03 PDT
Created attachment 52733 [details]
Patch
Comment 10 WebKit Review Bot 2010-04-07 06:30:05 PDT
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.
Comment 11 Olivier Goffart 2010-04-07 09:19:45 PDT
(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.
Comment 12 Kent Hansen 2010-04-21 07:18:33 PDT
(In reply to comment #9)
> Created an attachment (id=52733) [details]
> Patch

Builds on Mac now, at least.
Comment 13 Kent Hansen 2010-04-21 07:46:28 PDT
(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.
Comment 14 Kent Hansen 2010-04-21 08:44:27 PDT
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.
Comment 15 Kent Hansen 2010-04-21 08:49:21 PDT
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.
Comment 16 Simon Hausmann 2010-04-27 02:07:39 PDT
Created attachment 54401 [details]
Patch
Comment 17 Simon Hausmann 2010-04-27 02:09:13 PDT
(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.
Comment 18 WebKit Review Bot 2010-04-27 02:11:02 PDT
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.
Comment 19 Siddharth Mathur 2010-04-27 07:26:19 PDT
(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.
Comment 20 Darin Adler 2010-04-27 12:16:33 PDT
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.
Comment 21 Simon Hausmann 2010-04-27 13:50:34 PDT
(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.
Comment 22 Simon Hausmann 2010-04-28 03:05:57 PDT
Committed r58393: <http://trac.webkit.org/changeset/58393>
Comment 23 WebKit Review Bot 2010-04-28 03:42:46 PDT
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
Comment 24 Simon Hausmann 2010-04-29 00:15:33 PDT
Revision r58393 cherry-picked into qtwebkit-2.0 with commit 76fc13db1b9a2116d6bee35c0e2c8fbfd940cd34