Summary: | Fix non-thread safe use of WeakPtr under sendSecItemRequest() | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, commit-queue, jiewen_tan, rniwa, tsavell, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 199922 | ||||||||||||
Attachments: |
|
Description
Chris Dumez
2019-07-29 15:41:49 PDT
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. 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> All reviewed patches have been landed. Closing bug. |