REGRESSION(AppleWebKit/605.1.15): WebDownloadDelegate delegate methods called on non-main thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Nick Beadman]

Created attachment 353110 [details]
Xcode project of test program showing the problem

When using the older WebView API to build a simple application that can download files the thread that the WebDownloadDelegate methods are called on changed between AppleWebKit/604.5.6 and AppleWebKit/605.1.15. I have confirmed that the issue still occurs with r237410.

I have attached a very simple (and hacked together) macOS Xcode project that shows the problem when downloading WebKit Build Archives from https://webkit.org/build-archives

In the attached project all of the code that exercises the bug is in WindowController.m. First, there is an implementation of -[WebPolicyDelegate webView:decidePolicyForMIMEType:request:frame:decisionListener:] that checks for the MIME type "binary/octet-stream" and calls -[WebPolicyDecisionListener download] for that MIME type (and -use for all others). This method is called on the main thread as expected.

So running the application and clicking on a WebKit Build Archive will result in the WebDownloadDelegate methods getting called. Before AppleWebKit/605.1.15, in my case macOS Sierra 10.12.6 (16G1510) with AppleWebKit/603.3.8, all of the WebDownloadDelegate methods are called on the main thread as expected. Under AppleWebKit/605.1.15, macOS High Sierra 10.13.6 or above including macOS Mojave, the following WebDownloadDelegate methods are called on the main thread:

- (void)downloadDidBegin:(NSURLDownload *)download;
- (void)downloadDidFinish:(NSURLDownload *)download;

Unfortunately, the following delegate methods are called on a different thread:

- (void)download:(NSURLDownload *)download decideDestinationWithSuggestedFilename:(NSString *)filename;
- (void)download:(NSURLDownload *)download didCreateDestination:(NSString *)path;

Which thread the following are called on is unknown (untested):
- (BOOL)download:(NSURLDownload *)download shouldDecodeSourceDataOfMIMEType:(NSString *)encodingType;
- (void)download:(NSURLDownload *)download didFailWithError:(NSError *)error;
- (NSWindow *)downloadWindowForAuthenticationSheet:(WebDownload *)download;

My testing on macOS High Sierra 10.13.6 (17G64) the thread that -download:decideDestinationWithSuggestedFilename: and -download:didCreateDestination: are called on is an NSURLConnection thread (enqueued in order from com.apple.network.connections, com.apple.CFNetwork.Connection, com.apple.NSURLSession-work).

I have confirmed that this also occurs with AppleWebKit/605.1.15 running on macOS Mojave 10.14 and on macOS Sierra 10.12.6 when I use the r237410 WebKit Build Archive to launch the test application. During testing I initially installed macOS High Sierra 10.13.3 which includes AppleWebKit/604.5.6 which does not have this issue so the regression happened between 604.5.6 and 605.1.15.

While the attached test application does not crash, the application that this was originally reported against does because it calls WebKit from one of the delegate methods that no longer get called back on the main thread. I am able to workaround this issue by calling -[NSObject performSelectorOnMainThread:withObject:waitUntilDone:]

[Radar WebKit Bug Importer]

<rdar://problem/45603890>

[Chris Dumez]

Documention at https://developer.apple.com/documentation/foundation/nsurldownloaddelegate says:
"""
Note that these delegate methods will be called on the thread that started the asynchronous load operation for the associated NSURLDownload object.
"""

[Alex Christensen]

Created attachment 366836 [details]
Patch

[Darin Adler]

Comment on attachment 366836 [details]
Patch

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

> Source/WebKitLegacy/mac/Misc/WebDownload.mm:85
> +    callOnMainThread([realDelegate = retainPtr(realDelegate), download = retainPtr(download)] {

Why the mix of callOnMainThread and dispatch_sync(dispatch_get_main_queue())?

[Alex Christensen]

I use dispatch_sync where I need to return a result synchronously on the non-main thread.  This is equivalent to the use of m_semaphore in WebCoreResourceHandleAsOperationQueueDelegate.  I use callOnMainThread where I just need to forward a call with no immediate callback.

[Alex Christensen]

I'm going to add a check to see if we are on the main thread before calling dispatch_sync to be conservative and prevent a deadlock I think should be impossible.

[Alex Christensen]

Created attachment 366841 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r243947