Bug 199652 – Stop using GenericTaskQueue from multiple threads

• Component: Media
• Status: RESOLVED FIXED

[Chris Dumez]

Stop using GenericTaskQueue from multiple threads as its implementation is not thread-safe.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 373812 [details]
Patch

[Geoffrey Garen]

Comment on attachment 373812 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 373812 [details]
Patch

Clearing flags on attachment: 373812

Committed r247292: <https://trac.webkit.org/changeset/247292>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52871301>

[Ryan Haddad]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

Reverted r247292 for reason:

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

Committed r247323: <https://trac.webkit.org/changeset/247323>

[Chris Dumez]

Created attachment 373862 [details]
Patch

[EWS Watchlist]

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.

[Eric Carlson]

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.

[Chris Dumez]

Created attachment 373868 [details]
Patch

[Chris Dumez]

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

[WebKit Commit Bot]

Comment on attachment 373868 [details]
Patch

Clearing flags on attachment: 373868

Committed r247331: <https://trac.webkit.org/changeset/247331>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.