Summary: | WTF::Thread should have the threads stack bounds. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||
Component: | Web Template Framework | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, cdumez, cmarcelo, commit-queue, dbates, keith_miller, mark.lam, msaboff, saam, thorton, ysuzuki | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 174108, 174436 | ||||||||||||||||
Bug Blocks: | 174081, 174291 | ||||||||||||||||
Attachments: |
|
Description
Keith Miller
2017-06-29 10:28:48 PDT
(In reply to Keith Miller from comment #0) > There are a number of places in JSC that try to walk another threads stack. > Currently, the stack bounds are stored on ThreadData but that's in TLS so > other threads cannot see it. Right now we get the stack bounds by iterating > the MachineThreads looking for our target thread then getting it's stack > bounds. We should just put the stack bounds on the Thread. Sounds nice. I think we put StackBounds in WTFThreadData because of https://bugs.webkit.org/show_bug.cgi?id=102411. However, right now, we use Fiber Local Storage (FLS) to store TLS data in ThreadSpeific. Thus, I think the above bug is already fixed: Each fiber can use their own FLS. Fibers do not use TLS in Windows right now instead of FLS. Created attachment 314377 [details]
Patch
Comment on attachment 314377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314377&action=review > Source/WTF/wtf/WTFThreadData.h:-63 > - // We need to always get a fresh StackBounds from the OS due to how fibers work. > - // See https://bugs.webkit.org/show_bug.cgi?id=102411 This is no longer a problem. Right now, our ThreadSpecific (and ThreadHolder) always uses FLS instead of TLS. Comment on attachment 314377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314377&action=review > Source/WTF/wtf/Threading.cpp:97 > + // Ack initialization to the creator. > + context->initializationMutex->unlock(); Passing ownership of a mutex like this (locking on Thread A and unlocking on Thread B) is shady. It might happen to work out because out parking lot implementation may happen to just work (I haven't looked into the implementation to know if this is true). I think the classic way to do this correctly is: 1. Thread A creates mutex and a condvar (guarded by the mutex). 2. Thread A locks mutex and waits on condvar which releases the mutex. 3. Thread B locks mutex and notifies the condvar when ready. 4. Thread B unlocks the mutex. 5. Thread A now wakes from the wait and reacquires the mutex, and may unlock it thereafter. Can we do this instead? I think it is clearer about intent (with the wait and notify) and works without depending on any special behavior of the locking system. (In reply to Mark Lam from comment #4) > Comment on attachment 314377 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314377&action=review > > > Source/WTF/wtf/Threading.cpp:97 > > + // Ack initialization to the creator. > > + context->initializationMutex->unlock(); > > Passing ownership of a mutex like this (locking on Thread A and unlocking on > Thread B) is shady. It might happen to work out because out parking lot > implementation may happen to just work (I haven't looked into the > implementation to know if this is true). I think the classic way to do this > correctly is: > 1. Thread A creates mutex and a condvar (guarded by the mutex). > 2. Thread A locks mutex and waits on condvar which releases the mutex. > 3. Thread B locks mutex and notifies the condvar when ready. > 4. Thread B unlocks the mutex. > 5. Thread A now wakes from the wait and reacquires the mutex, and may unlock > it thereafter. > > Can we do this instead? I think it is clearer about intent (with the wait > and notify) and works without depending on any special behavior of the > locking system. OK, I'll upload the revised patch. Created attachment 314409 [details]
Patch
Created attachment 314410 [details]
Patch
Created attachment 314411 [details]
Patch
Comment on attachment 314411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314411&action=review Nice work. r=me. > Source/JavaScriptCore/ChangeLog:8 > + There is a site in JSC that try to walk another threads stack. typo: /threads/thread's/ Comment on attachment 314411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314411&action=review > Source/WTF/wtf/Threading.cpp:50 > + Start, Initialize "Initialize" sounds like the thread is still initializing. Let's changed it to "Initialized" to indicate that it's already done initializing. (In reply to Mark Lam from comment #10) > Comment on attachment 314411 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314411&action=review > > > Source/WTF/wtf/Threading.cpp:50 > > + Start, Initialize > > "Initialize" sounds like the thread is still initializing. Let's changed it > to "Initialized" to indicate that it's already done initializing. OK, fixed. Thank you! Committed r219060: <http://trac.webkit.org/changeset/219060> Re-opened since this is blocked by bug 174108 Yusuke, I think you'll have to work with Keith/Mark to get this re-landed. You *should* be able to reproduce the crashes by building for iOS Simulator and writing a test app that makes a UIWebView (or even just calls [UIWebView class], since the crash is under +initialize). (In reply to Tim Horton from comment #14) > Yusuke, I think you'll have to work with Keith/Mark to get this re-landed. > You *should* be able to reproduce the crashes by building for iOS Simulator > and writing a test app that makes a UIWebView (or even just calls [UIWebView > class], since the crash is under +initialize). Discussed with Mark, Keith, Wenson, and Tim. When starting Web thread, WTFThreadData is initialized before calling initializeThreading(). Before that patch, it was OK. But now, WTFThreadData touches Thread::current(). Thus, it causes crashes. And I think this is why only iOS UIWebView crashes: it is caused in Web thread initialization. I'll upload the patch with fix. Created attachment 314555 [details]
Patch
Comment on attachment 314555 [details]
Patch
r=me.
Comment on attachment 314555 [details]
Patch
r=me too.
Comment on attachment 314555 [details]
Patch
Thanks!
Comment on attachment 314555 [details] Patch Clearing flags on attachment: 314555 Committed r219176: <http://trac.webkit.org/changeset/219176> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 174436 (In reply to WebKit Commit Bot from comment #22) > Re-opened since this is blocked by bug 174436 Rolled out in r219427: <http://trac.webkit.org/changeset/219427>. Comment on attachment 314555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314555&action=review > Source/WTF/wtf/ThreadingPthreads.cpp:193 > +void Thread::initializeCurrentThreadEvenIfNonWTFCreated() > { > + Thread::current().initialize(); This call to Thread.current() can cause an infinite loop because Thread::current() can call initializeCurrentThreadEvenIfNonWTFCreated(). (In reply to Mark Lam from comment #24) > Comment on attachment 314555 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314555&action=review > > > Source/WTF/wtf/ThreadingPthreads.cpp:193 > > +void Thread::initializeCurrentThreadEvenIfNonWTFCreated() > > { > > + Thread::current().initialize(); > > This call to Thread.current() can cause an infinite loop because > Thread::current() can call initializeCurrentThreadEvenIfNonWTFCreated(). By "infinite loop", I actually meant infinite recursion. (In reply to Mark Lam from comment #24) > Comment on attachment 314555 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=314555&action=review > > > Source/WTF/wtf/ThreadingPthreads.cpp:193 > > +void Thread::initializeCurrentThreadEvenIfNonWTFCreated() > > { > > + Thread::current().initialize(); > > This call to Thread.current() can cause an infinite loop because > Thread::current() can call initializeCurrentThreadEvenIfNonWTFCreated(). What makes infinite loop? I think Thread::current() call will stop because we call ThreadHolder::initialize(thread.get()) before calling initializeCurrentThreadEvenIfNonWTFCreated(). Comment on attachment 314555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314555&action=review >>>> Source/WTF/wtf/ThreadingPthreads.cpp:193 >>>> + Thread::current().initialize(); >>> >>> This call to Thread.current() can cause an infinite loop because Thread::current() can call initializeCurrentThreadEvenIfNonWTFCreated(). >> >> By "infinite loop", I actually meant infinite recursion. > > What makes infinite loop? I think Thread::current() call will stop because we call ThreadHolder::initialize(thread.get()) before calling initializeCurrentThreadEvenIfNonWTFCreated(). Here's the sequence of events I was thinking of: 1. someone calls Thread::current(), which calls currentMayBeNull(), which returns null. 2. Thread::current() calls Thread::initializeCurrentThreadEvenIfNonWTFCreated(), which calls 3. Thread::current(), which calls 4. Thread::initializeCurrentThreadEvenIfNonWTFCreated(), ... ... but I see now that the chain should have stopped at (3) because at line 311, we should have already planted the current Thread into the ThreadHolder. I'll do more debugging. Comment on attachment 314555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314555&action=review >>>>> Source/WTF/wtf/ThreadingPthreads.cpp:193 >>>>> + Thread::current().initialize(); >>>> >>>> This call to Thread.current() can cause an infinite loop because Thread::current() can call initializeCurrentThreadEvenIfNonWTFCreated(). >>> >>> By "infinite loop", I actually meant infinite recursion. >> >> What makes infinite loop? I think Thread::current() call will stop because we call ThreadHolder::initialize(thread.get()) before calling initializeCurrentThreadEvenIfNonWTFCreated(). > > Here's the sequence of events I was thinking of: > 1. someone calls Thread::current(), which calls currentMayBeNull(), which returns null. > 2. Thread::current() calls Thread::initializeCurrentThreadEvenIfNonWTFCreated(), which calls > 3. Thread::current(), which calls > 4. Thread::initializeCurrentThreadEvenIfNonWTFCreated(), ... > > ... but I see now that the chain should have stopped at (3) because at line 311, we should have already planted the current Thread into the ThreadHolder. > > I'll do more debugging. Now I'm considering about very edge cases. Can you have a stacktrace for this infinite recursion? When is Thread::current() called in this case? I guess this is related to TLS deallocation things... (In reply to Yusuke Suzuki from comment #28) > Now I'm considering about very edge cases. Can you have a stacktrace for > this infinite recursion? When is Thread::current() called in this case? > I guess this is related to TLS deallocation things... Here's the stack trace: Thread 8 Crashed: 0 libsystem_malloc.dylib 0x0000000182c58cb0 _nano_malloc_check_clear + 516 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/nano_malloc.c:0) 1 libsystem_malloc.dylib 0x0000000182c58b44 _nano_malloc_check_clear + 152 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/nano_malloc.c:681) 2 libsystem_malloc.dylib 0x0000000182c57b8c nano_malloc + 44 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/nano_malloc.c:871) 3 libsystem_malloc.dylib 0x0000000182c46a70 malloc_zone_malloc + 172 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/malloc.c:1333) 4 libsystem_malloc.dylib 0x0000000182c49428 malloc + 32 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/malloc.c:1622) 5 libc++abi.dylib 0x00000001825df708 operator new(unsigned long) + 44 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libcxxabi/libcxxabi-400.7/src/stdlib_new_delete.cpp:33) 6 JavaScriptCore 0x000000018ab84fbc WTF::ThreadHolder::initialize(WTF::Thread&) + 60 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ThreadHolderPthreads.cpp:57) 7 JavaScriptCore 0x000000018ab853a8 WTF::Thread::current() + 164 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ThreadingPthreads.cpp:311) 8 JavaScriptCore 0x000000018ab853ac WTF::Thread::current() + 168 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ThreadingPthreads.cpp:193) 9 JavaScriptCore 0x000000018ab853ac WTF::Thread::current() + 168 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ThreadingPthreads.cpp:193) 10 JavaScriptCore 0x000000018ab853ac WTF::Thread::current() + 168 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ThreadingPthreads.cpp:193) ... <repeat line 10 all the way down> (In reply to Mark Lam from comment #29) > (In reply to Yusuke Suzuki from comment #28) > > Now I'm considering about very edge cases. Can you have a stacktrace for > > this infinite recursion? When is Thread::current() called in this case? > > I guess this is related to TLS deallocation things... > > Here's the stack trace: > > Thread 8 Crashed: > 0 libsystem_malloc.dylib 0x0000000182c58cb0 > _nano_malloc_check_clear + 516 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/ > nano_malloc.c:0) > 1 libsystem_malloc.dylib 0x0000000182c58b44 > _nano_malloc_check_clear + 152 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/ > nano_malloc.c:681) > 2 libsystem_malloc.dylib 0x0000000182c57b8c nano_malloc + 44 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/ > nano_malloc.c:871) > 3 libsystem_malloc.dylib 0x0000000182c46a70 malloc_zone_malloc + > 172 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/ > malloc.c:1333) > 4 libsystem_malloc.dylib 0x0000000182c49428 malloc + 32 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libmalloc/libmalloc-139/src/ > malloc.c:1622) > 5 libc++abi.dylib 0x00000001825df708 operator new(unsigned > long) + 44 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libcxxabi/libcxxabi-400.7/ > src/stdlib_new_delete.cpp:33) > 6 JavaScriptCore 0x000000018ab84fbc > WTF::ThreadHolder::initialize(WTF::Thread&) + 60 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ > ThreadHolderPthreads.cpp:57) > 7 JavaScriptCore 0x000000018ab853a8 WTF::Thread::current() > + 164 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ > ThreadingPthreads.cpp:311) > 8 JavaScriptCore 0x000000018ab853ac WTF::Thread::current() > + 168 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ > ThreadingPthreads.cpp:193) > 9 JavaScriptCore 0x000000018ab853ac WTF::Thread::current() > + 168 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ > ThreadingPthreads.cpp:193) > 10 JavaScriptCore 0x000000018ab853ac WTF::Thread::current() > + 168 > (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WTF/WTF-7604.1.30/wtf/ > ThreadingPthreads.cpp:193) > ... <repeat line 10 all the way down> Thank you! Can we have more logs? I would like to see which code originally issues Thread::current(). (In reply to Yusuke Suzuki from comment #30) > Thank you! Can we have more logs? I would like to see which code originally > issues Thread::current(). The trace is from a crash log, and the trace is cut off (there was more frames than the trace was able to capture). So, unfortunately, I don't have the caller unless I can reproduce it. I haven't worked on reproducing it yet. @Yusuke, here's another crash trace related to this patch but is not the infinite recursion one: Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Subtype: KERN_INVALID_ADDRESS at 0x000000000000001f Termination Signal: Segmentation fault: 11 Termination Reason: Namespace SIGNAL, Code 0xb Terminating Process: exc handler [0] Triggered by Thread: 0 Thread 0 name: Dispatch queue: com.apple.main-thread Thread 0 Crashed: 0 JavaScriptCore 0x0000000102d47f3c WTF::Thread::currentMayBeNull() + 16 1 JavaScriptCore 0x0000000102d481b0 WTF::Thread::current() + 20 2 JavaScriptCore 0x0000000102d52a78 WTF::WTFThreadData::createAndRegisterForGetspecificDirect() + 40 3 JavaScriptCore 0x0000000102d12adc WTF::AtomicStringImpl::addSlowCase(WTF::StringImpl&) + 304 4 WebCore 0x00000001042fd020 WebCore::ResourceResponseBase::ResourceResponseBase(WebCore::URL const&, WTF::String const&, long long, WTF::String const&) + 108 5 WebCore 0x00000001034eb768 WebCore::ArchiveResource::create(WTF::RefPtr<WebCore::SharedBuffer>&&, WebCore::URL const&, WTF::String const&, WTF::String const&, WTF::String const&, WebCorWebCore::ResourceResponse const&) + 144 6 WebKitLegacy 0x000000010210cea4 -[WebResource(WebResourcePrivate) _initWithData:URL:MIMEType:textEncodingName:frameName:response:copyData:] + 312 7 WebKitLegacy 0x000000010210c02c -[WebResource initWithData:URL:MIMEType:textEncodingName:frameName:] + 36 ... Thread 0 crashed with ARM Thread State (64-bit): x0: 0x000000000000001f x1: 0x0000000000000008 x2: 0x0000000000000000 x3: 0x0000000000000251 x4: 0x000000016f021340 x5: 0x000000010a8e1008 x6: 0x0000000000000038 x7: 0x0000000000000005 x8: 0x00000001b7b34c60 x9: 0x00002217eac00000 x10: 0x0000000000000576 x11: 0x00000001b9e7d9ca x12: 0x00000001b9e7d9ca x13: 0x0000000000000003 x14: 0x0000000000000001 x15: 0x0000000000000881 x16: 0x0000000186817d48 x17: 0x0000000000000080 x18: 0x0000000000000000 x19: 0x00000001c4855150 x20: 0x00000001148f8020 x21: 0x0000000102df8308 x22: 0x000000016f021348 x23: 0x00000001148d0000 x24: 0x000000016f021350 x25: 0x0000000000000251 x26: 0x000000016f021058 x27: 0x00000001148fc000 x28: 0x0000000000000008 fp: 0x000000016f020f50 lr: 0x0000000102d47f38 sp: 0x000000016f020f50 pc: 0x0000000102d47f3c cpsr: 0x80000000 This one has nothing to do with Thread destruction. (In reply to Mark Lam from comment #32) > @Yusuke, here's another crash trace related to this patch but is not the > infinite recursion one: > > Exception Type: EXC_BAD_ACCESS (SIGSEGV) > Exception Subtype: KERN_INVALID_ADDRESS at 0x000000000000001f > Termination Signal: Segmentation fault: 11 > Termination Reason: Namespace SIGNAL, Code 0xb > Terminating Process: exc handler [0] > Triggered by Thread: 0 > > Thread 0 name: Dispatch queue: com.apple.main-thread > Thread 0 Crashed: > 0 JavaScriptCore 0x0000000102d47f3c > WTF::Thread::currentMayBeNull() + 16 > 1 JavaScriptCore 0x0000000102d481b0 > WTF::Thread::current() + 20 > 2 JavaScriptCore 0x0000000102d52a78 > WTF::WTFThreadData::createAndRegisterForGetspecificDirect() + 40 > 3 JavaScriptCore 0x0000000102d12adc > WTF::AtomicStringImpl::addSlowCase(WTF::StringImpl&) + 304 > 4 WebCore 0x00000001042fd020 > WebCore::ResourceResponseBase::ResourceResponseBase(WebCore::URL const&, > WTF::String const&, long long, WTF::String const&) + 108 > 5 WebCore 0x00000001034eb768 > WebCore::ArchiveResource::create(WTF::RefPtr<WebCore::SharedBuffer>&&, > WebCore::URL const&, WTF::String const&, WTF::String const&, WTF::String > const&, WebCorWebCore::ResourceResponse const&) + 144 > 6 WebKitLegacy 0x000000010210cea4 > -[WebResource(WebResourcePrivate) > _initWithData:URL:MIMEType:textEncodingName:frameName:response:copyData:] + > 312 > 7 WebKitLegacy 0x000000010210c02c -[WebResource > initWithData:URL:MIMEType:textEncodingName:frameName:] + 36 > ... > > Thread 0 crashed with ARM Thread State (64-bit): > x0: 0x000000000000001f x1: 0x0000000000000008 x2: 0x0000000000000000 > x3: 0x0000000000000251 > x4: 0x000000016f021340 x5: 0x000000010a8e1008 x6: 0x0000000000000038 > x7: 0x0000000000000005 > x8: 0x00000001b7b34c60 x9: 0x00002217eac00000 x10: 0x0000000000000576 > x11: 0x00000001b9e7d9ca > x12: 0x00000001b9e7d9ca x13: 0x0000000000000003 x14: 0x0000000000000001 > x15: 0x0000000000000881 > x16: 0x0000000186817d48 x17: 0x0000000000000080 x18: 0x0000000000000000 > x19: 0x00000001c4855150 > x20: 0x00000001148f8020 x21: 0x0000000102df8308 x22: 0x000000016f021348 > x23: 0x00000001148d0000 > x24: 0x000000016f021350 x25: 0x0000000000000251 x26: 0x000000016f021058 > x27: 0x00000001148fc000 > x28: 0x0000000000000008 fp: 0x000000016f020f50 lr: 0x0000000102d47f38 > sp: 0x000000016f020f50 pc: 0x0000000102d47f3c cpsr: 0x80000000 > > This one has nothing to do with Thread destruction. Thank you! I think this is because wtfThreadData() is called before calling WTF::initializeThreading(). I thought that WTFThreadData depends on WTF::initializeThreading() because it uses ThreadSpecific<> things. But I think we should not do that. WTFThreadData can be called without calling WTF::initializeThreading(). I think having StackBounds in both WTFThreadData / Thread would be the simplest design here. I'm not sure it fixes this infinite recursion, but it seems worth doing. (In reply to Yusuke Suzuki from comment #33) > Thank you! > I think this is because wtfThreadData() is called before calling > WTF::initializeThreading(). > I thought that WTFThreadData depends on WTF::initializeThreading() because > it uses ThreadSpecific<> things. > > But I think we should not do that. WTFThreadData can be called without > calling WTF::initializeThreading(). > I think having StackBounds in both WTFThreadData / Thread would be the > simplest design here. > I'm not sure it fixes this infinite recursion, but it seems worth doing. Hmmm ... how is WTFThreadData different than WTF::Thread? Taking a quick glance at it, I wonder if the 2 should be consolidated. Aren't they both describing the thread, and stored in a ThreadSpecific? (In reply to Mark Lam from comment #34) > > Hmmm ... how is WTFThreadData different than WTF::Thread? Taking a quick > glance at it, I wonder if the 2 should be consolidated. Aren't they both > describing the thread, and stored in a ThreadSpecific? Yes, merging WTFThreadData and WTF::Thread is one of the goal of my threading refactoring patches. But it requires substaintial changes in WTFThread, ThreadHolder and WTF::Thread. So, in this patch, I think having 2 copies of StackBounds is nice. Currently, I'm planning to merge them when threading patches are almost landed. (In reply to Yusuke Suzuki from comment #35) > (In reply to Mark Lam from comment #34) > > > > Hmmm ... how is WTFThreadData different than WTF::Thread? Taking a quick > > glance at it, I wonder if the 2 should be consolidated. Aren't they both > > describing the thread, and stored in a ThreadSpecific? > > Yes, merging WTFThreadData and WTF::Thread is one of the goal of my > threading refactoring patches. > But it requires substaintial changes in WTFThread, ThreadHolder and > WTF::Thread. > So, in this patch, I think having 2 copies of StackBounds is nice. > Currently, I'm planning to merge them when threading patches are almost > landed. OK. Going with 2 copies of StackBounds works for now. I've found the source of the infinite recursion. Basically, it goes something like this: [JSContext currentContext] calls wtfThreadData(), which calls WTFThreadData::createAndRegisterForGetspecificDirect(), which calls WTFThreadData() constructor, which calls Thread::current(), which calls Thread::initializeCurrentThreadEvenIfNonWTFCreated(). However, this code path has not called WTF::initializeThreading() yet. I added a call to WTF::initializeThreading() in [JSContext currentContext] before the call to wtfThreadData(), and all is well again. In summary, the issue is identical to the other one. (In reply to Mark Lam from comment #37) > I've found the source of the infinite recursion. Basically, it goes > something like this: > > [JSContext currentContext] calls > wtfThreadData(), which calls > WTFThreadData::createAndRegisterForGetspecificDirect(), which > calls > WTFThreadData() constructor, which calls > Thread::current(), which calls > Thread::initializeCurrentThreadEvenIfNonWTFCreated(). > > However, this code path has not called WTF::initializeThreading() yet. I > added a call to WTF::initializeThreading() in [JSContext currentContext] > before the call to wtfThreadData(), and all is well again. > > In summary, the issue is identical to the other one. Great! I'll upload the patch with this fix. Created attachment 315544 [details]
Patch
Comment on attachment 315544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315544&action=review r=me. I'm going to do a local build and test on iOS to make sure that the previously seen issues are truly resolved (and that there aren't any other surprises). Will let you know when all clear to land. Thanks. > Source/WTF/wtf/WTFThreadData.h:101 > + StackStats::PerThreadStats m_stackStats { }; This is not needed. It should be invoked by default. (In reply to Mark Lam from comment #40) > Comment on attachment 315544 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315544&action=review > > r=me. I'm going to do a local build and test on iOS to make sure that the > previously seen issues are truly resolved (and that there aren't any other > surprises). Will let you know when all clear to land. Thanks. Looks like the iOS are indeed resolved. You're good to land with the 1 remaining issue fixed. Comment on attachment 315544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315544&action=review >> Source/WTF/wtf/WTFThreadData.h:101 >> + StackStats::PerThreadStats m_stackStats { }; > > This is not needed. It should be invoked by default. Thanks! Fixed. Committed r219647: <http://trac.webkit.org/changeset/219647> |