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.
Created attachment 52715 [details] Patch
(In reply to comment #1) > Created an attachment (id=52715) [details] > Patch Yep, this fixes the crash on Linux.
Attachment 52715 [details] did not build on mac: Build output: http://webkit-commit-queue.appspot.com/results/1552337
(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).
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?)
(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
(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.
(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?
Created attachment 52733 [details] Patch
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.
(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.
(In reply to comment #9) > Created an attachment (id=52733) [details] > Patch Builds on Mac now, at least.
(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.
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.
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.
Created attachment 54401 [details] Patch
(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.
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.
(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 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.
(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.
Committed r58393: <http://trac.webkit.org/changeset/58393>
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
Revision r58393 cherry-picked into qtwebkit-2.0 with commit 76fc13db1b9a2116d6bee35c0e2c8fbfd940cd34