Bug 200249 – Fix non-thread safe use of WeakPtr under sendSecItemRequest()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

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

[Chris Dumez]

Created attachment 375113 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

Created attachment 375125 [details]
Patch

[Chris Dumez]

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

[Ryosuke Niwa]

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?

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 375125 [details]
Patch

Clearing flags on attachment: 375125

Committed r247932: <https://trac.webkit.org/changeset/247932>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53687336>

[Truitt Savell]

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.

[Truitt Savell]

Reverted r247932 for reason:

Broke 8 API tests across all platforms.

Committed r248006: <https://trac.webkit.org/changeset/248006>

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 375159 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 375162 [details]
Patch

[Chris Dumez]

Comment on attachment 375162 [details]
Patch

Let's give EWS a change to run this time :P

[Chris Dumez]

Comment on attachment 375162 [details]
Patch

Looks like there are some API tests failures on macOS still.

[WebKit Commit Bot]

Comment on attachment 375162 [details]
Patch

Clearing flags on attachment: 375162

Committed r248014: <https://trac.webkit.org/changeset/248014>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.