Bug 197893 - [WK2][iOS] UIProcess may get killed because it is taking too long to release its background task after expiration
Summary: [WK2][iOS] UIProcess may get killed because it is taking too long to release ...
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: 2019-05-14 13:49 PDT by Chris Dumez
Modified: 2019-05-21 11:26 PDT (History)
6 users (show)

See Also:


Attachments
Patch (14.75 KB, patch)
2019-05-14 14:35 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (14.87 KB, patch)
2019-05-15 10:13 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 2019-05-14 13:49:50 PDT
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.
Comment 1 Chris Dumez 2019-05-14 13:52:39 PDT
<rdar://problem/50234105>
Comment 2 Chris Dumez 2019-05-14 14:30:14 PDT
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.
Comment 3 Chris Dumez 2019-05-14 14:35:29 PDT
Created attachment 369894 [details]
Patch
Comment 4 Alex Christensen 2019-05-15 07:42:03 PDT
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/
Comment 5 Chris Dumez 2019-05-15 08:50:53 PDT
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.
Comment 6 Chris Dumez 2019-05-15 10:13:55 PDT
Created attachment 369964 [details]
Patch
Comment 7 Chris Dumez 2019-05-15 10:15:48 PDT
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.
Comment 8 WebKit Commit Bot 2019-05-15 11:29:46 PDT
Comment on attachment 369964 [details]
Patch

Clearing flags on attachment: 369964

Committed r245334: <https://trac.webkit.org/changeset/245334>
Comment 9 WebKit Commit Bot 2019-05-15 11:29:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Geoffrey Garen 2019-05-21 10:15:15 PDT
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?
Comment 11 Chris Dumez 2019-05-21 11:26:19 PDT
(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.