Bug 197385 - [iOS] The UIProcess may get killed for trying to stay runnable in the background for more than 30 seconds
Summary: [iOS] The UIProcess may get killed for trying to stay runnable in the backgro...
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-04-29 13:55 PDT by Chris Dumez
Modified: 2019-04-29 15:40 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.19 KB, patch)
2019-04-29 14:03 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-04-29 13:55:51 PDT
The UIProcess may get killed for trying to stay runnable in the background for more than 30 seconds.
Comment 1 Chris Dumez 2019-04-29 13:56:06 PDT
<rdar://problem/50001505>
Comment 2 Chris Dumez 2019-04-29 14:03:16 PDT
Created attachment 368490 [details]
Patch
Comment 3 Geoffrey Garen 2019-04-29 14:18:12 PDT
Comment on attachment 368490 [details]
Patch

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

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:72
> +        [self _updateBackgroundTask];

I think you need to capture a weak reference to self, otherwise you have a memory leak (the global Notification Center has a reference to self, and never goes away).

Once that leak is fixed, don't we also need to unregister for this notification in our dealloc method?
Comment 4 Chris Dumez 2019-04-29 14:19:42 PDT
Comment on attachment 368490 [details]
Patch

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

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:72
>> +        [self _updateBackgroundTask];
> 
> I think you need to capture a weak reference to self, otherwise you have a memory leak (the global Notification Center has a reference to self, and never goes away).
> 
> Once that leak is fixed, don't we also need to unregister for this notification in our dealloc method?

self is a static object though, so I would not expect the dealloc method to ever get called.
Comment 5 Chris Dumez 2019-04-29 14:23:21 PDT
Comment on attachment 368490 [details]
Patch

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

>>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:72
>>> +        [self _updateBackgroundTask];
>> 
>> I think you need to capture a weak reference to self, otherwise you have a memory leak (the global Notification Center has a reference to self, and never goes away).
>> 
>> Once that leak is fixed, don't we also need to unregister for this notification in our dealloc method?
> 
> self is a static object though, so I would not expect the dealloc method to ever get called.

I mean that WKProcessAssertionBackgroundTaskManager (self) is a singleton, so "leaking" it is the expectation.
Comment 6 Geoffrey Garen 2019-04-29 14:26:41 PDT
Comment on attachment 368490 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2019-04-29 15:40:53 PDT
Comment on attachment 368490 [details]
Patch

Clearing flags on attachment: 368490

Committed r244761: <https://trac.webkit.org/changeset/244761>
Comment 8 WebKit Commit Bot 2019-04-29 15:40:55 PDT
All reviewed patches have been landed.  Closing bug.