Possible crash in ~UserGestureIndicator() when on non-main thread

[Chris Dumez]

Possible crash in ~UserGestureIndicator() when on non-main thread:
Thread 27 Crashed:: WebCore: Worker
0   com.apple.WebCore             	0x00007fffaef36330 WebCore::UserGestureIndicator::~UserGestureIndicator() + 176
1   com.apple.WebCore             	0x00007fffadeb9b70 WebCore::DOMTimer::fired() + 1008
2   com.apple.WebCore             	0x00007fffadde13e0 WebCore::ThreadTimers::sharedTimerFiredInternal() + 176
3   com.apple.WebCore             	0x00007fffaefead6d WebCore::WorkerRunLoop::runInMode(WebCore::WorkerGlobalScope*, WebCore::ModePredicate const&, WebCore::WorkerRunLoop::WaitMode) + 365
4   com.apple.WebCore             	0x00007fffaefeaba0 WebCore::WorkerRunLoop::run(WebCore::WorkerGlobalScope*) + 96
5   com.apple.WebCore             	0x00007fffaefee421 WebCore::WorkerThread::workerThread() + 929
6   com.apple.JavaScriptCore      	0x00007fffa8db1c62 WTF::threadEntryPoint(void*) + 178
7   com.apple.JavaScriptCore      	0x00007fffa8db1b8f WTF::wtfThreadEntryPoint(void*) + 15
8   libsystem_pthread.dylib       	0x00007fffbc8e2aab _pthread_body + 180
9   libsystem_pthread.dylib       	0x00007fffbc8e29f7 _pthread_start + 286
10  libsystem_pthread.dylib       	0x00007fffbc8e21fd thread_start + 13

[Chris Dumez]

<rdar://problem/30283071>

[Chris Dumez]

Created attachment 315472 [details]
Patch

[Brady Eidson]

Comment on attachment 315472 [details]
Patch

This seems like we could actually write a test for it?

[Chris Dumez]

(In reply to Brady Eidson from comment #3)
> Comment on attachment 315472 [details]
> Patch
> 
> This seems like we could actually write a test for it?

I will try but it seems pretty racy.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Brady Eidson from comment #3)
> > Comment on attachment 315472 [details]
> > Patch
> > 
> > This seems like we could actually write a test for it?
> 
> I will try but it seems pretty racy.

I *think* a user gesture needs to happen on the main thread and *while* this user gesture is happening (i.e. there is still a UserGestureIndicator object alive for the gesture), a DOMTimer needs to fire in a worker thread.

[Sam Weinig]

Comment on attachment 315472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=315472&action=review

> Source/WebCore/dom/UserGestureIndicator.cpp:71
> +    // It is only safe to use currentToken() on the main thread.
> +    m_previousToken = currentToken();

Seems like you should ASSERT(isMainThread()) in currentToken().

[Chris Dumez]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 315472 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315472&action=review
> 
> > Source/WebCore/dom/UserGestureIndicator.cpp:71
> > +    // It is only safe to use currentToken() on the main thread.
> > +    m_previousToken = currentToken();
> 
> Seems like you should ASSERT(isMainThread()) in currentToken().

Thanks, I'll add the ASSERT and am trying to write a test right now.

[Sam Weinig]

Comment on attachment 315472 [details]
Patch

r=me with the ASSERT.

[Chris Dumez]

Created attachment 315491 [details]
Patch

[Chris Dumez]

Comment on attachment 315491 [details]
Patch

Will wait for debug EWS before landing.

[Chris Dumez]

Comment on attachment 315491 [details]
Patch

Clearing flags on attachment: 315491

Committed r219531: <http://trac.webkit.org/changeset/219531>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.