Bug 173975 – WTF::Thread should have the threads stack bounds.

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Keith Miller]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 314377 [details]
Patch

[Yusuke Suzuki]

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.

[Mark Lam]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 314409 [details]
Patch

[Yusuke Suzuki]

Created attachment 314410 [details]
Patch

[Yusuke Suzuki]

Created attachment 314411 [details]
Patch

[Mark Lam]

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/

[Mark Lam]

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.

[Yusuke Suzuki]

(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!

[Yusuke Suzuki]

Committed r219060: <http://trac.webkit.org/changeset/219060>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 174108

[Tim Horton]

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).

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 314555 [details]
Patch

[Keith Miller]

Comment on attachment 314555 [details]
Patch

r=me.

[Mark Lam]

Comment on attachment 314555 [details]
Patch

r=me too.

[Yusuke Suzuki]

Comment on attachment 314555 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 314555 [details]
Patch

Clearing flags on attachment: 314555

Committed r219176: <http://trac.webkit.org/changeset/219176>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 174436

[Mark Lam]

(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>.

[Mark Lam]

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().

[Mark Lam]

(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.

[Yusuke Suzuki]

(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().

[Mark Lam]

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.

[Yusuke Suzuki]

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...

[Mark Lam]

(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>

[Yusuke Suzuki]

(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().

[Mark Lam]

(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.

[Mark Lam]

@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.

[Yusuke Suzuki]

(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.

[Mark Lam]

(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?

[Yusuke Suzuki]

(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.

[Mark Lam]

(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.

[Mark Lam]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 315544 [details]
Patch

[Mark Lam]

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.

[Mark Lam]

(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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r219647: <http://trac.webkit.org/changeset/219647>