RESOLVED FIXED 197893
[WK2][iOS] UIProcess may get killed because it is taking too long to release its background task after expiration
https://bugs.webkit.org/show_bug.cgi?id=197893
Summary [WK2][iOS] UIProcess may get killed because it is taking too long to release ...
Chris Dumez
Reported 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.
Attachments
Patch (14.75 KB, patch)
2019-05-14 14:35 PDT, Chris Dumez
no flags
Patch (14.87 KB, patch)
2019-05-15 10:13 PDT, Chris Dumez
no flags
Chris Dumez
Comment 1 2019-05-14 13:52:39 PDT
Chris Dumez
Comment 2 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.
Chris Dumez
Comment 3 2019-05-14 14:35:29 PDT
Alex Christensen
Comment 4 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/
Chris Dumez
Comment 5 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.
Chris Dumez
Comment 6 2019-05-15 10:13:55 PDT
Chris Dumez
Comment 7 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.
WebKit Commit Bot
Comment 8 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>
WebKit Commit Bot
Comment 9 2019-05-15 11:29:48 PDT
All reviewed patches have been landed. Closing bug.
Geoffrey Garen
Comment 10 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?
Chris Dumez
Comment 11 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.
Note You need to log in before you can comment on or make changes to this bug.