Summary: | [iOS] AuxiliaryProcessProxy::sendWithAsyncReply() should prevent auxiliary process suspension while processing the IPC | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | achristensen, ap, beidson, eric.carlson, ews-watchlist, ggaren, glenn, jer.noble, philipj, ryanhaddad, sergio, webkit-bug-importer, youennf | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=211421 | ||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2020-04-02 13:51:09 PDT
Created attachment 395297 [details]
Patch
Comment on attachment 395297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395297&action=review iOS-wk2 EWS seems angry. Lots of hangs, and this crashing backtrace, implying misuses of some C++ object: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 libsystem_kernel.dylib 0x00007fff523bc33a __pthread_kill + 10 1 libsystem_pthread.dylib 0x00007fff52465e60 pthread_kill + 430 2 libsystem_c.dylib 0x00007fff5234ba5c abort + 120 3 libc++abi.dylib 0x00007fff502497f8 abort_message + 231 4 libc++abi.dylib 0x00007fff502499c7 demangling_terminate_handler() + 262 5 libobjc.A.dylib 0x00007fff513fbd7c _objc_terminate() + 96 6 libc++abi.dylib 0x00007fff50256e97 std::__terminate(void (*)()) + 8 7 libc++abi.dylib 0x00007fff50256ae9 __cxa_rethrow + 99 8 libobjc.A.dylib 0x00007fff513fbcb4 objc_exception_rethrow + 37 9 com.apple.CoreFoundation 0x00007fff23bce0ea CFRunLoopRunSpecific + 570 10 com.apple.GeoServices 0x00007fff384c0bb0 GSEventRunModal + 65 11 com.apple.UIKitCore 0x00007fff48092d4d UIApplicationMain + 1621 12 org.webkit.WebKitTestRunnerApp 0x00000001022457cf main + 114 13 libdyld.dylib 0x00007fff5227ec25 start + 1 > Source/WebKit/UIProcess/ProcessThrottler.cpp:87 > + m_quietBackgroundActivities.reset(); Is this call to reset() sufficient? I believe it will revoke all pointers to the QuietActivityCount, but it won't invoke QuietActivityCount::m_throttler.updateAssertionIfNeeded(), so it won't drop any extant assertion. Comment on attachment 395297 [details]
Patch
Will investigate failures.
Comment on attachment 395297 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395297&action=review >> Source/WebKit/UIProcess/ProcessThrottler.cpp:87 >> + m_quietBackgroundActivities.reset(); > > Is this call to reset() sufficient? I believe it will revoke all pointers to the QuietActivityCount, but it won't invoke QuietActivityCount::m_throttler.updateAssertionIfNeeded(), so it won't drop any extant assertion. I will do it because it is safer. That said, this is only called either when ProcessThrottler is destroyed or the assertion has been invalidated so it does not currently matter AFAICT. Created attachment 395316 [details]
Patch
I am working on figuring out why some of the tests are timing out. Looks like I can reproduce locally. Created attachment 398651 [details]
WIP Patch
Comment on attachment 398651 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398651&action=review > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1181 > + sendWithAsyncReply(Messages::NetworkProcess::SetFirstPartyWebsiteDataRemovalModeForTesting(sessionID, mode), [completionHandler = WTFMove(completionHandler)]() mutable { > completionHandler(); > }); the whole lambda can be replaced by WTFMove(completionHandler). Created attachment 398653 [details]
WIP Patch
Created attachment 398654 [details]
WIP Patch
Created attachment 398670 [details]
Patch
Created attachment 398673 [details]
Patch
Created attachment 398676 [details]
Patch
Comment on attachment 398676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398676&action=review > Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:155 > + if (asyncReplyInfo && canSendMessage() && shouldStartProcessThrottlerActivity == ShouldStartProcessThrottlerActivity::Yes) { This is fine like this, but it seems that, in theory we should only do that when sending the messages, so for State::Running. We would then need to take a similar assertion in AuxiliaryProcessProxy::didFinishLaunching. With a helper routine, we would share the code between both. > Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:53 > + virtual ProcessThrottler& throttler() = 0; Do we need to have it public? > Source/WebKit/UIProcess/GPU/GPUProcessProxy.h:60 > + ProcessThrottler& throttler() final { return m_throttler; } Do we need to have it public? > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:196 > + ProcessThrottler& throttler() final { return m_throttler; } Do we need to have it public? > Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h:92 > + ProcessThrottler& throttler() final { return m_throttler; } Do we need to have it public? > Source/WebKit/UIProcess/ProcessThrottler.h:68 > + if (!isQuietActivity()) { We are losing some logging, this might be fine but just wondering whether we should add some logging at some of call sites to replace this logging. Comment on attachment 398676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398676&action=review >> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:155 >> + if (asyncReplyInfo && canSendMessage() && shouldStartProcessThrottlerActivity == ShouldStartProcessThrottlerActivity::Yes) { > > This is fine like this, but it seems that, in theory we should only do that when sending the messages, so for State::Running. > We would then need to take a similar assertion in AuxiliaryProcessProxy::didFinishLaunching. > With a helper routine, we would share the code between both. It does not hurt to start the activity before the process has finished launching and it decreases code complexity to do this only in one place. When the process has finished launching the process throttler will take the correct assertion based on currently running activities. >> Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:53 >> + virtual ProcessThrottler& throttler() = 0; > > Do we need to have it public? It was public in the subclasses. I will check but I assume it needs to the public. >> Source/WebKit/UIProcess/ProcessThrottler.h:68 >> + if (!isQuietActivity()) { > > We are losing some logging, this might be fine but just wondering whether we should add some logging at some of call sites to replace this logging. This logging would be too verbose for IPC in my opinion. I don’t think we want to log 2 lines for every IPC, or even one line. Note that if the starting of the activity does result in an assertion being taken, logging will happen at process throttler and process assertion level. I have checked that throttler() is still used outside those classes and thus still needs to be public. Committed r261288: <https://trac.webkit.org/changeset/261288> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398676 [details]. |