Bug 209928 – [iOS] AuxiliaryProcessProxy::sendWithAsyncReply() should prevent auxiliary process suspension while processing the IPC

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

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 395297 [details]
Patch

[Geoffrey Garen]

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.

[Chris Dumez]

Comment on attachment 395297 [details]
Patch

Will investigate failures.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 395316 [details]
Patch

[Chris Dumez]

I am working on figuring out why some of the tests are timing out. Looks like I can reproduce locally.

[Chris Dumez]

Created attachment 398651 [details]
WIP Patch

[Alex Christensen]

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

[Chris Dumez]

Created attachment 398653 [details]
WIP Patch

[Chris Dumez]

Created attachment 398654 [details]
WIP Patch

[Chris Dumez]

Created attachment 398670 [details]
Patch

[Chris Dumez]

Created attachment 398673 [details]
Patch

[Chris Dumez]

Created attachment 398676 [details]
Patch

[youenn fablet]

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.

[Chris Dumez]

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.

[Chris Dumez]

I have checked that throttler() is still used outside those classes and thus still needs to be public.

[EWS]

Committed r261288: <https://trac.webkit.org/changeset/261288>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398676 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62976705>