Bug 37195

Summary: JSC's currentThreadStackBase is not reentrant on some platforms
Product: WebKit Reporter: Simon Hausmann <hausmann>
Component: JavaScriptCoreAssignee: Simon Hausmann <hausmann>
Status: CLOSED FIXED    
Severity: Major CC: abarth, ap, eric, kent.hansen, koshuin, laszlo.gombos, norbert.leser, ogoffart, s.mathur, webkit.review.bot
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for Symbian (untested)
none
Patch darin: review+

Simon Hausmann
Reported 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.
Attachments
Patch (2.48 KB, patch)
2010-04-07 01:55 PDT, Simon Hausmann 		no flags
DetailsFormatted DiffDiff
Patch (2.58 KB, patch)
2010-04-07 06:29 PDT, Simon Hausmann 		no flags
DetailsFormatted DiffDiff
Patch for Symbian (untested) (874 bytes, patch)
2010-04-21 08:49 PDT, Kent Hansen 		no flags
DetailsFormatted DiffDiff
Patch (2.92 KB, patch)
2010-04-27 02:07 PDT, Simon Hausmann 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Simon Hausmann
Comment 1 2010-04-07 01:55:14 PDT
Created attachment 52715 [details] Patch
Kent Hansen
Comment 2 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.
Eric Seidel (no email)
Comment 3 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
Kent Hansen
Comment 4 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).
Olivier Goffart
Comment 5 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?)
Kent Hansen
Comment 6 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
Simon Hausmann
Comment 7 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.
Kent Hansen
Comment 8 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?
Simon Hausmann
Comment 9 2010-04-07 06:29:03 PDT
Created attachment 52733 [details] Patch
WebKit Review Bot
Comment 10 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.
Olivier Goffart
Comment 11 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.
Kent Hansen
Comment 12 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.
Kent Hansen
Comment 13 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.
Kent Hansen
Comment 14 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.
Kent Hansen
Comment 15 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.
Simon Hausmann
Comment 16 2010-04-27 02:07:39 PDT
Created attachment 54401 [details] Patch
Simon Hausmann
Comment 17 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.
WebKit Review Bot
Comment 18 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.
Siddharth Mathur
Comment 19 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.
Darin Adler
Comment 20 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.
Simon Hausmann
Comment 21 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.
Simon Hausmann
Comment 22 2010-04-28 03:05:57 PDT
Committed r58393: <http://trac.webkit.org/changeset/58393>
WebKit Review Bot
Comment 23 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
Simon Hausmann
Comment 24 2010-04-29 00:15:33 PDT
Revision r58393 cherry-picked into qtwebkit-2.0 with commit 76fc13db1b9a2116d6bee35c0e2c8fbfd940cd34
Note You need to log in before you can comment on or make changes to this bug.