Fix non-thread safe use of WeakPtr under sendSecItemRequest(): Thread 1 Crashed:: Dispatch queue: com.apple.CFNetwork.Connection 0 com.apple.WebKit 0x0000000106433bc3 WTFCrashWithInfo(int, char const*, char const*, int) + 19 (Assertions.h:568) 1 com.apple.WebKit 0x000000010662346b WebKit::NetworkProcess::WeakValueType* WTF::WeakPtrImpl::get<WebKit::NetworkProcess>() + 75 2 com.apple.WebKit 0x00000001066231b8 WebKit::sendSecItemRequest(WebKit::SecItemRequestData::Type, __CFDictionary const*, __CFDictionary const*) + 92 (DumbPtrTraits.h:43) 3 com.apple.WebKit 0x000000010662303b WebKit::webSecItemCopyMatching(__CFDictionary const*, void const**) + 39 (Optional.h:371) 4 com.apple.CFNetwork 0x00007fff537b2889 StorageQuery::performQuery() + 59 5 com.apple.CFNetwork 0x00007fff537e9ed4 PersistentCredentialStorage::copyDefaultCredentialForProtectionSpace(_CFURLProtectionSpace*) + 92 6 com.apple.CFNetwork 0x00007fff537ec470 CFURLCredentialStorageCopyDefaultCredentialForProtectionSpace + 74 7 com.apple.CFNetwork 0x00007fff53913493 -[NSURLCredentialStorage defaultCredentialForProtectionSpace:] + 53 8 com.apple.CFNetwork 0x00007fff539135ad -[NSURLCredentialStorage getDefaultCredentialForProtectionSpace:task:completionHandler:] + 22 9 com.apple.CFNetwork 0x00007fff5382cec1 NSXCredentialStorage::copyDefaultCredentialForProtectionSpace(_CFURLProtectionSpace*, NSURLSessionTask const*) const + 179 10 com.apple.CFNetwork 0x00007fff5386da53 HTTPProtocol::_CFHTTPProtHasCredentialsForChallenge(__CFHTTPMessage*) + 3085 11 com.apple.CFNetwork 0x00007fff538704f7 HTTPProtocol::attemptAuthentication(__CFHTTPMessage*) + 113 12 com.apple.CFNetwork 0x00007fff53770a03 HTTPProtocol::performHeaderRead(__CFHTTPMessage*) + 1637 13 com.apple.CFNetwork 0x00007fff53770258 HTTPProtocol::handleStreamEvent(__CFHTTPMessage*, dispatch_data_s*, CFStreamError const*) + 180 14 com.apple.CFNetwork 0x00007fff53757e9b HTTPTransaction::_onqueue_invokeHandler() + 281 15 com.apple.CFNetwork 0x00007fff5375aba5 HTTPConnection::_onqueue_responseDataArrived(dispatch_data_s*, CFStreamError, bool) + 231 16 com.apple.CFNetwork 0x00007fff5375aa73 HTTPEngine::_readBodyFinish(dispatch_data_s*, CFStreamError, bool) + 129 17 com.apple.CFNetwork 0x00007fff5375a937 HTTPEngine::_deliverBodyBytes(dispatch_data_s*, CFStreamError, bool) + 305 18 com.apple.CFNetwork 0x00007fff5392ffad invocation function for block in HTTPEngine::_getBodyIntelligently(void (dispatch_data_s*, CFStreamError, bool) block_pointer) + 160 19 libdispatch.dylib 0x00007fff7c7a05fa _dispatch_call_block_and_release + 12 20 libdispatch.dylib 0x00007fff7c798db8 _dispatch_client_callout + 8 21 libdispatch.dylib 0x00007fff7c7ad217 _dispatch_queue_serial_drain + 635 22 libdispatch.dylib 0x00007fff7c7a0166 _dispatch_queue_invoke + 373 23 libdispatch.dylib 0x00007fff7c7ad07a _dispatch_queue_serial_drain + 222 24 libdispatch.dylib 0x00007fff7c7a0166 _dispatch_queue_invoke + 373 25 libdispatch.dylib 0x00007fff7c7adf0d _dispatch_root_queue_drain_deferred_wlh + 332 26 libdispatch.dylib 0x00007fff7c7b1d21 _dispatch_workloop_worker_thread + 880 27 libsystem_pthread.dylib 0x00007fff7cae9fd2 _pthread_wqthread + 980 28 libsystem_pthread.dylib 0x00007fff7cae9be9 start_wqthread + 13
Created attachment 375113 [details] Patch
Comment on attachment 375113 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375113&action=review > Source/WebKit/Shared/mac/SecItemShim.cpp:89 > + globalNetworkProcess()->parentProcessConnection()->sendWithReply(Messages::SecItemShimProxy::SecItemRequest(SecItemRequestData(requestType, query, attributesToMatch)), 0, workQueue(), [&](auto reply) { As long as we're trying this, let's use sendWithAsyncReply. I tried using a synchronous message in https://bugs.webkit.org/show_bug.cgi?id=197747 but it didn't work. This is a better approach, though.
Created attachment 375125 [details] Patch
(In reply to Alex Christensen from comment #2) > Comment on attachment 375113 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375113&action=review > > > Source/WebKit/Shared/mac/SecItemShim.cpp:89 > > + globalNetworkProcess()->parentProcessConnection()->sendWithReply(Messages::SecItemShimProxy::SecItemRequest(SecItemRequestData(requestType, query, attributesToMatch)), 0, workQueue(), [&](auto reply) { > > As long as we're trying this, let's use sendWithAsyncReply. I tried using a > synchronous message in https://bugs.webkit.org/show_bug.cgi?id=197747 but it > didn't work. This is a better approach, though. Oh, what are you making me do? :) Please take another look before I land.
Comment on attachment 375125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375125&action=review > Source/WebKit/Shared/mac/SecItemShim.cpp:72 > + RunLoop::main().dispatch([&] { Can't we just use callOnMainRunLoopAndWait?
Comment on attachment 375125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375125&action=review >> Source/WebKit/Shared/mac/SecItemShim.cpp:72 >> + RunLoop::main().dispatch([&] { > > Can't we just use callOnMainRunLoopAndWait? I don't think so since the lambda calls sendWithAsyncReply() with is async and takes a completion handler. We only want to go back to resume the background thread once the sendWithAsyncReply()'s completion handler has been called. I guess I could use callOnMainRunLoopAndWait() if the lambda did a sendSync() but this would not seem like an improvement.
Comment on attachment 375125 [details] Patch Clearing flags on attachment: 375125 Committed r247932: <https://trac.webkit.org/changeset/247932>
All reviewed patches have been landed. Closing bug.
<rdar://problem/53687336>
It looks like the changes in https://trac.webkit.org/changeset/247932/webkit has caused 8 api test timeouts: Timeout TestWebKitAPI.ProcessSwap.NumberOfCachedProcesses Received data during response processing, queuing it. Received data during response processing, queuing it. Received data during response processing, queuing it. Received data during response processing, queuing it. TestWebKitAPI.WKWebsiteDataStore.RemoveNonPersistentCredentials TestWebKitAPI.Challenge.SecIdentity TestWebKitAPI.WKWebsiteDataStore.FetchNonPersistentCredentials TestWebKitAPI.WKWebsiteDataStore.RemoveAndFetchData TestWebKitAPI.WKWebsiteDataStore.FetchPersistentCredentials TestWebKitAPI.ResourceLoadStatistics.GrandfatherCallback TestWebKitAPI.Challenge.BasicProposedCredential Build: https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/14548 I was able to reproduce this locally and it looks like EWS caught this.
Reverted r247932 for reason: Broke 8 API tests across all platforms. Committed r248006: <https://trac.webkit.org/changeset/248006>
(In reply to Truitt Savell from comment #11) > Reverted r247932 for reason: > > Broke 8 API tests across all platforms. > > Committed r248006: <https://trac.webkit.org/changeset/248006> Ok, I will investigate. Thanks.
(In reply to Truitt Savell from comment #10) > It looks like the changes in https://trac.webkit.org/changeset/247932/webkit > > has caused 8 api test timeouts: > Timeout > > TestWebKitAPI.ProcessSwap.NumberOfCachedProcesses > Received data during response processing, queuing it. > Received data during response processing, queuing it. > Received data during response processing, queuing it. > Received data during response processing, queuing it. > > TestWebKitAPI.WKWebsiteDataStore.RemoveNonPersistentCredentials > TestWebKitAPI.Challenge.SecIdentity > TestWebKitAPI.WKWebsiteDataStore.FetchNonPersistentCredentials > TestWebKitAPI.WKWebsiteDataStore.RemoveAndFetchData > TestWebKitAPI.WKWebsiteDataStore.FetchPersistentCredentials > TestWebKitAPI.ResourceLoadStatistics.GrandfatherCallback > TestWebKitAPI.Challenge.BasicProposedCredential > > Build: > https://build.webkit.org/builders/ > Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/14548 > > I was able to reproduce this locally and it looks like EWS caught this. The issue seems to be that sendSecItemRequest(), in which case the main thread is blocked on the semaphore and we never process the response to the SecItemShimProxy::SecItemRequest async IPC (which needs to be processed by the same main thread).
Created attachment 375159 [details] Patch
Comment on attachment 375159 [details] Patch If we're on the main thread we should send a synchronous message. If we're not on the main thread, we should use sendWithAsyncReply to not block the main thread.
(In reply to Alex Christensen from comment #15) > Comment on attachment 375159 [details] > Patch > > If we're on the main thread we should send a synchronous message. If we're > not on the main thread, we should use sendWithAsyncReply to not block the > main thread. This is the previous patch iteration, which did not work. The async reply never gets processed since the main thread is stuck on the semaphore...
(In reply to Chris Dumez from comment #16) > (In reply to Alex Christensen from comment #15) > > Comment on attachment 375159 [details] > > Patch > > > > If we're on the main thread we should send a synchronous message. If we're > > not on the main thread, we should use sendWithAsyncReply to not block the > > main thread. > > This is the previous patch iteration, which did not work. The async reply > never gets processed since the main thread is stuck on the semaphore... Oh, I re-read, so 2 separate IPC messages? I guess we can do that.
Created attachment 375162 [details] Patch
Comment on attachment 375162 [details] Patch Let's give EWS a change to run this time :P
Comment on attachment 375162 [details] Patch Looks like there are some API tests failures on macOS still.
Comment on attachment 375162 [details] Patch Clearing flags on attachment: 375162 Committed r248014: <https://trac.webkit.org/changeset/248014>