Bug 209928 - [iOS] AuxiliaryProcessProxy::sendWithAsyncReply() should prevent auxiliary process suspension while processing the IPC
Summary: [iOS] AuxiliaryProcessProxy::sendWithAsyncReply() should prevent auxiliary pr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-02 13:51 PDT by Chris Dumez
Modified: 2020-05-07 08:02 PDT (History)
13 users (show)

See Also:


Attachments
Patch (28.77 KB, patch)
2020-04-02 13:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (27.96 KB, patch)
2020-04-02 16:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (40.49 KB, patch)
2020-05-06 13:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (40.64 KB, patch)
2020-05-06 13:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (40.65 KB, patch)
2020-05-06 14:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (46.31 KB, patch)
2020-05-06 15:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (46.32 KB, patch)
2020-05-06 15:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (46.36 KB, patch)
2020-05-06 15:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-04-02 13:51:09 PDT
AuxiliaryProcessProxy::sendWithAsyncReply() should prevent auxiliary process suspension on iOS while processing the IPC. If the process is suspended either before or during the IPC, this could result in hangs. This is a speculative fix for the flaky timeouts we see on many tests on iOS only.
Comment 1 Chris Dumez 2020-04-02 13:55:24 PDT
Created attachment 395297 [details]
Patch
Comment 2 Geoffrey Garen 2020-04-02 15:30:19 PDT
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 3 Chris Dumez 2020-04-02 15:34:08 PDT
Comment on attachment 395297 [details]
Patch

Will investigate failures.
Comment 4 Chris Dumez 2020-04-02 15:47:58 PDT
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.
Comment 5 Chris Dumez 2020-04-02 16:15:04 PDT
Created attachment 395316 [details]
Patch
Comment 6 Chris Dumez 2020-04-03 08:28:43 PDT
I am working on figuring out why some of the tests are timing out. Looks like I can reproduce locally.
Comment 7 Chris Dumez 2020-05-06 13:48:02 PDT
Created attachment 398651 [details]
WIP Patch
Comment 8 Alex Christensen 2020-05-06 13:49:10 PDT
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).
Comment 9 Chris Dumez 2020-05-06 13:53:56 PDT
Created attachment 398653 [details]
WIP Patch
Comment 10 Chris Dumez 2020-05-06 14:00:07 PDT
Created attachment 398654 [details]
WIP Patch
Comment 11 Chris Dumez 2020-05-06 15:04:02 PDT
Created attachment 398670 [details]
Patch
Comment 12 Chris Dumez 2020-05-06 15:16:39 PDT
Created attachment 398673 [details]
Patch
Comment 13 Chris Dumez 2020-05-06 15:48:01 PDT
Created attachment 398676 [details]
Patch
Comment 14 youenn fablet 2020-05-07 01:51:21 PDT
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 15 Chris Dumez 2020-05-07 07:39:20 PDT
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.
Comment 16 Chris Dumez 2020-05-07 07:57:28 PDT
I have checked that throttler() is still used outside those classes and thus still needs to be public.
Comment 17 EWS 2020-05-07 08:01:35 PDT
Committed r261288: <https://trac.webkit.org/changeset/261288>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398676 [details].
Comment 18 Radar WebKit Bug Importer 2020-05-07 08:02:18 PDT
<rdar://problem/62976705>