Bug 199652

Summary: Stop using GenericTaskQueue from multiple threads
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: MediaAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, commit-queue, eric.carlson, ews-watchlist, ggaren, jer.noble, rniwa, ryanhaddad, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 199639    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2019-07-09 19:42:12 PDT 
Stop using GenericTaskQueue from multiple threads as its implementation is not thread-safe.
Comment 1 Chris Dumez 2019-07-09 19:45:58 PDT 
Thread 7 Crashed:: Dispatch queue: WebCoreAVFLoaderDelegate queue
0   com.apple.JavaScriptCore      	0x0000000411b68cfe WTFCrash + 14 (Assertions.cpp:305)
1   com.apple.WebCore             	0x00000003f9f2c92b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x00000003fa2033ae WTF::WeakPtrFactory<WebCore::GenericTaskQueue<WebCore::Timer, std::__1::atomic<unsigned int> > >::createWeakPtr(WebCore::GenericTaskQueue<WebCore::Timer, std::__1::atomic<unsigned int> >&) const + 142 (WeakPtr.h:132)
3   com.apple.WebCore             	0x00000003fa203293 WTF::WeakPtr<WebCore::GenericTaskQueue<WebCore::Timer, std::__1::atomic<unsigned int> > > WTF::makeWeakPtr<WebCore::GenericTaskQueue<WebCore::Timer, std::__1::atomic<unsigned int> > >(WebCore::GenericTaskQueue<WebCore::Timer, std::__1::atomic<unsigned int> >&) + 51 (WeakPtr.h:207)
4   com.apple.WebCore             	0x00000003fa1de75a WebCore::GenericTaskQueue<WebCore::Timer, std::__1::atomic<unsigned int> >::enqueueTask(WTF::Function<void ()>&&) + 90 (GenericTaskQueue.h:101)
5   com.apple.WebCore             	0x00000003fa1df030 -[WebCoreAVFLoaderDelegate resourceLoader:shouldWaitForLoadingOfRequestedResource:] + 144 (MediaPlayerPrivateAVFoundationObjC.mm:3449)
6   com.apple.CoreFoundation      	0x00007fff301dea0c __invoking___ + 140
7   com.apple.CoreFoundation      	0x00007fff301de8c4 -[NSInvocation invoke] + 273
8   com.apple.avfoundation        	0x00007fff2bd7dff7 __104-[AVAssetResourceLoader _performDelegateSelector:withObject:representingNewRequest:key:fallbackHandler:]_block_invoke + 403
9   com.apple.avfoundation        	0x00007fff2bd7dc77 -[AVAssetResourceLoader _performDelegateCallbackSynchronouslyIfCurrentDelegateQueueIsQueue:delegateCallbackBlock:] + 301
10  libdispatch.dylib             	0x00007fff67802683 _dispatch_call_block_and_release + 12
11  libdispatch.dylib             	0x00007fff6780360e _dispatch_client_callout + 8
12  libdispatch.dylib             	0x00007fff67808bd2 _dispatch_lane_serial_drain + 597
13  libdispatch.dylib             	0x00007fff67809556 _dispatch_lane_invoke + 363
14  libdispatch.dylib             	0x00007fff67812bc5 _dispatch_workloop_worker_thread + 582
15  libsystem_pthread.dylib       	0x00007fff67a5c860 _pthread_wqthread + 336
16  libsystem_pthread.dylib       	0x00007fff67a5c69b start_wqthread + 15
Comment 2 Chris Dumez 2019-07-09 19:49:33 PDT 
Created attachment 373812 [details]
Patch
Comment 3 Geoffrey Garen 2019-07-09 20:50:04 PDT 
Comment on attachment 373812 [details]
Patch

r=me
Comment 4 WebKit Commit Bot 2019-07-09 21:19:54 PDT 
Comment on attachment 373812 [details]
Patch

Clearing flags on attachment: 373812

Committed r247292: <https://trac.webkit.org/changeset/247292>
Comment 5 WebKit Commit Bot 2019-07-09 21:19:56 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2019-07-09 21:24:15 PDT 
<rdar://problem/52871301>
Comment 7 Ryan Haddad 2019-07-10 11:01:13 PDT 
This change caused TestWebKitAPI.CloseWebViewDuringEnterFullscreen.VideoFullscreen to time out on Mojave bots:
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20(Tests)/builds/5227

The test is still passing on High Sierra, so EWS didn't catch it.
Comment 8 Chris Dumez 2019-07-10 11:30:24 PDT 
(In reply to Ryan Haddad from comment #7)
> This change caused
> TestWebKitAPI.CloseWebViewDuringEnterFullscreen.VideoFullscreen to time out
> on Mojave bots:
> https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20(Tests)/
> builds/5227
> 
> The test is still passing on High Sierra, so EWS didn't catch it.

Ok, looking now.
Comment 9 Chris Dumez 2019-07-10 11:42:01 PDT 
(In reply to Chris Dumez from comment #8)
> (In reply to Ryan Haddad from comment #7)
> > This change caused
> > TestWebKitAPI.CloseWebViewDuringEnterFullscreen.VideoFullscreen to time out
> > on Mojave bots:
> > https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20(Tests)/
> > builds/5227
> > 
> > The test is still passing on High Sierra, so EWS didn't catch it.
> 
> Ok, looking now.

Does not reproduce for me locally on Catalina, odd.
Comment 10 Chris Dumez 2019-07-10 13:13:33 PDT 
Reverted r247292 for reason:

Caused CloseWebViewDuringEnterFullscreen.VideoFullscreen API test to time out on Mojave bots

Committed r247323: <https://trac.webkit.org/changeset/247323>
Comment 11 Chris Dumez 2019-07-10 13:32:24 PDT 
Created attachment 373862 [details]
Patch
Comment 12 EWS Watchlist 2019-07-10 13:36:30 PDT 
Attachment 373862 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3391:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3392:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3394:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3395:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3396:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3398:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3399:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3401:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3402:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3415:  preprocessor directives (e.g., #ifdef, #define, #import) should never be indented.  [whitespace/indent] [4]
Total errors found: 10 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Eric Carlson 2019-07-10 14:43:00 PDT 
Comment on attachment 373862 [details]
Patch

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

r=me with a couple of minor nits

> Source/WebCore/ChangeLog:16
> +        Stop last template parameter which was used exclusively by WebCoreAVFLoaderDelegate to try and

Nit: s/Stop/Remove/

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3391
> +    #if ENABLE(WIRELESS_PLAYBACK_TARGET)

Nit: indentation

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3395
> +    #if ENABLE(LEGACY_ENCRYPTED_MEDIA) || ENABLE(ENCRYPTED_MEDIA)

Ditto.

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3401
> +    #if !RELEASE_LOG_DISABLED

Ditto.
Comment 14 Chris Dumez 2019-07-10 15:07:35 PDT 
Created attachment 373868 [details]
Patch
Comment 15 Chris Dumez 2019-07-10 15:08:09 PDT 
(In reply to Eric Carlson from comment #13)
> Comment on attachment 373862 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373862&action=review
> 
> r=me with a couple of minor nits
> 
> > Source/WebCore/ChangeLog:16
> > +        Stop last template parameter which was used exclusively by WebCoreAVFLoaderDelegate to try and
> 
> Nit: s/Stop/Remove/
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3391
> > +    #if ENABLE(WIRELESS_PLAYBACK_TARGET)
> 
> Nit: indentation
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3395
> > +    #if ENABLE(LEGACY_ENCRYPTED_MEDIA) || ENABLE(ENCRYPTED_MEDIA)
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:3401
> > +    #if !RELEASE_LOG_DISABLED
> 
> Ditto.

Thanks Eric.
Comment 16 WebKit Commit Bot 2019-07-10 16:08:35 PDT 
Comment on attachment 373868 [details]
Patch

Clearing flags on attachment: 373868

Committed r247331: <https://trac.webkit.org/changeset/247331>
Comment 17 WebKit Commit Bot 2019-07-10 16:08:37 PDT 
All reviewed patches have been landed.  Closing bug.