Bug 197893 – [WK2][iOS] UIProcess may get killed because it is taking too long to release its background task after expiration

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

[Chris Dumez]

UIProcess may get killed because it is taking too long to release its background task after its expiration handler was called. The reason is that the background task's expiration handler sequentially sends a ProcessWillSuspendImminently synchronous IPC to each of its child processes and only then ends the background task.
By the time we receive the response from all child processes, it may be too late and we get killed.

[Chris Dumez]

<rdar://problem/50234105>

[Chris Dumez]

Logging with the patch enabled, when our background assertion expires:
2019-05-14 14:26:18.125569-0700 0x3747f    Error       0x95005              9797   0    MobileSafari: (WebKit) [com.apple.WebKit:ProcessSuspension] Background task expired while holding WebKit ProcessAssertion (isMainThread? 1).
2019-05-14 14:26:18.127250-0700 0x3747f    Default     0x95005              9797   0    MobileSafari: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x123a2bb10 - WKProcessAssertionBackgroundTaskManager - _scheduleReleaseTask because the expiration handler has been called
2019-05-14 14:26:18.128797-0700 0x374a5    Default     0x0                  9798   0    com.apple.WebKit.Networking: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x1011f87f8 - NetworkProcess::processWillSuspendImminently() BEGIN
2019-05-14 14:26:18.128898-0700 0x374aa    Default     0x0                  9799   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x145e15a40 - WebProcess::processWillSuspendImminently() BEGIN
2019-05-14 14:26:18.131060-0700 0x374ba    Default     0x0                  9800   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x100e134d0 - WebProcess::processWillSuspendImminently() BEGIN
2019-05-14 14:26:18.153307-0700 0x374a5    Default     0x0                  9798   0    com.apple.WebKit.Networking: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x1011f87f8 - NetworkProcess::processWillSuspendImminently() END
2019-05-14 14:26:18.159562-0700 0x374ba    Default     0x0                  9800   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] WebProcess 9800 is freezing all layer trees
2019-05-14 14:26:18.159613-0700 0x374ba    Default     0x0                  9800   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x100e134d0 - WebProcess::destroyRenderingResources() took 0.00ms
2019-05-14 14:26:18.161355-0700 0x374ba    Default     0x0                  9800   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x100e134d0 - WebProcess::updateFreezerStatus() isFreezable: 0, success
2019-05-14 14:26:18.161811-0700 0x374ba    Default     0x0                  9800   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x100e134d0 - WebProcess::markAllLayersVolatile()
2019-05-14 14:26:18.161868-0700 0x374ba    Default     0x0                  9800   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x100e134d0 - WebProcess::markAllLayersVolatile() Successfuly marked all layers as volatile
2019-05-14 14:26:18.161909-0700 0x374ba    Default     0x0                  9800   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x100e134d0 - WebProcess::processWillSuspendImminently() END
2019-05-14 14:26:18.177892-0700 0x374aa    Default     0x0                  9799   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] WebProcess 9799 is freezing all layer trees
2019-05-14 14:26:18.177937-0700 0x374aa    Default     0x0                  9799   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x146810008 - WebPage (PageID=1) - Adding a reason 4 to freeze layer tree; current reasons are 2
2019-05-14 14:26:18.177986-0700 0x374aa    Default     0x0                  9799   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x145e15a40 - WebProcess::destroyRenderingResources() took 0.01ms
2019-05-14 14:26:18.179802-0700 0x374aa    Default     0x0                  9799   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x145e15a40 - WebProcess::updateFreezerStatus() isFreezable: 1, success
2019-05-14 14:26:18.179952-0700 0x374aa    Default     0x0                  9799   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x145e15a40 - WebProcess::markAllLayersVolatile()
2019-05-14 14:26:18.180721-0700 0x374aa    Default     0x0                  9799   0    com.apple.WebKit.WebContent: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x145e15a40 - WebProcess::processWillSuspendImminently() END
2019-05-14 14:26:20.282348-0700 0x3747f    Default     0x95005              9797   0    MobileSafari: (WebKit) [com.apple.WebKit:ProcessSuspension] 0x123a2bb10 - WKProcessAssertionBackgroundTaskManager - endBackgroundTask

Looks good.

[Chris Dumez]

Created attachment 369894 [details]
Patch

[Alex Christensen]

Comment on attachment 369894 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369894&action=review

r=me

> Source/WebKit/ChangeLog:15
> +        1. Send the ProcessWillSuspendImminently asynchronously so that all processes can do their processing in parallel

\o/

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:123
> +    _pendingTaskReleaseTask = dispatch_block_create((dispatch_block_flags_t)0, ^{

Do we need to do any freeing of this block?  I also don't understand why you used dispatch_block_create instead of just a BlockPtr and a regular block and setting it to null when you would've called dispatch_block_cancel below

> Source/WebKit/WebProcess/WebProcess.h:-421
> -    void didReceiveSyncWebProcessMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&);

\o/

[Chris Dumez]

Comment on attachment 369894 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369894&action=review

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:123
>> +    _pendingTaskReleaseTask = dispatch_block_create((dispatch_block_flags_t)0, ^{
> 
> Do we need to do any freeing of this block?  I also don't understand why you used dispatch_block_create instead of just a BlockPtr and a regular block and setting it to null when you would've called dispatch_block_cancel below

I followed the documentation for dispatch_block_cancel: https://developer.apple.com/documentation/dispatch/1431058-dispatch_block_cancel
"""
The result of passing NULL or a block object not returned by the dispatch_block_create or dispatch_block_create_with_qos_class function is undefined.
"""

So it seems I have to use dispatch_block_create() if I want to use dispatch_block_cancel(). I am not 100% sure if I need to do some cleanup to free the block though, I will confirm before landing.

[Chris Dumez]

Created attachment 369964 [details]
Patch

[Chris Dumez]

Comment on attachment 369964 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369964&action=review

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:130
> +    Block_release(_pendingTaskReleaseTask);

Looks like you're right that I am supposed to release a block constructed by dispatch_block_create() if we're not using ARC so I added a call to Block_release() here.
Please take a look.

[WebKit Commit Bot]

Comment on attachment 369964 [details]
Patch

Clearing flags on attachment: 369964

Committed r245334: <https://trac.webkit.org/changeset/245334>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

Comment on attachment 369964 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369964&action=review

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:130
>> +    Block_release(_pendingTaskReleaseTask);
> 
> Looks like you're right that I am supposed to release a block constructed by dispatch_block_create() if we're not using ARC so I added a call to Block_release() here.
> Please take a look.

Maybe use BlockPtr here?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #10)
> Comment on attachment 369964 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369964&action=review
> 
> >> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:130
> >> +    Block_release(_pendingTaskReleaseTask);
> > 
> > Looks like you're right that I am supposed to release a block constructed by dispatch_block_create() if we're not using ARC so I added a call to Block_release() here.
> > Please take a look.
> 
> Maybe use BlockPtr here?

I considered it but I need to call dispatch_block_create() and BlockPtr did not seem to have a way to adopt a block. It always refs the blocks.