Bug 225324 - [iOS] Use async API to take RunningBoard assertions
Summary: [iOS] Use async API to take RunningBoard assertions
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: 2021-05-03 13:06 PDT by Chris Dumez
Modified: 2021-05-04 11:04 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.55 KB, patch)
2021-05-03 13:13 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (13.59 KB, patch)
2021-05-03 13:31 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (13.59 KB, patch)
2021-05-03 13:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.60 KB, patch)
2021-05-03 22:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.54 KB, patch)
2021-05-03 23:15 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 2021-05-03 13:06:49 PDT
Use async API to take RunningBoard assertions to avoid hanging the main thread.
Comment 1 Chris Dumez 2021-05-03 13:07:17 PDT
<rdar://76972252>
Comment 2 Chris Dumez 2021-05-03 13:13:10 PDT
Created attachment 427590 [details]
Patch
Comment 3 Chris Dumez 2021-05-03 13:31:14 PDT
Created attachment 427593 [details]
Patch
Comment 4 Chris Dumez 2021-05-03 13:38:40 PDT
Created attachment 427596 [details]
Patch
Comment 5 Simon Fraser (smfr) 2021-05-03 18:38:55 PDT
Comment on attachment 427596 [details]
Patch

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

> Source/WebKit/UIProcess/ProcessAssertion.h:78
> +    bool m_isValid { false }; // Assume valid while in the process of being acquired.

The comment seems to disagree with the initialization value.

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:66
> +    std::atomic<bool> m_hasValidBackgroundAssertion; // Assume valid while in the process of being acquired.

The comment here is also confusing.

> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:168
> +        m_hasValidBackgroundAssertion = true;

This is where I'd put the comment. Would it be more truthful to use an Optional<> to indicate that we don't know the state yet?
Comment 6 Geoffrey Garen 2021-05-03 20:59:39 PDT
Comment on attachment 427596 [details]
Patch

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

>> Source/WebKit/UIProcess/ProcessAssertion.h:78
>> +    bool m_isValid { false }; // Assume valid while in the process of being acquired.
> 
> The comment seems to disagree with the initialization value.

The dissonance between comment and initialization here threw me for a loop too at first. I'd suggest changing the default initialization to true, removing the comment, and removing the (additional) initialization from the cross platform constructor. The iOS constructor can explicitly set m_isValid to false in the !pid case.

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:66
>> +    std::atomic<bool> m_hasValidBackgroundAssertion; // Assume valid while in the process of being acquired.
> 
> The comment here is also confusing.

I'd call this _backgroundTaskIsValid and remove the comment.

>> Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm:168
>> +        m_hasValidBackgroundAssertion = true;
> 
> This is where I'd put the comment. Would it be more truthful to use an Optional<> to indicate that we don't know the state yet?

I don't think we can use Optional<> here because we never transition from not knowing to knowing. We're always in the "I tried to acquire state" until we're in the "I got invalidated" state.

That said, I think it's fine to say that our state is "valid" from the moment we try to acquire. Our definition of "valid" is "there is no need to acquire".
Comment 7 Chris Dumez 2021-05-03 22:02:29 PDT
I was thinking of renaming the flag to m_wasInvalidated so it is clearer what it means. The definition of is valid is a bit ambiguous otherwise.

And Geoff is right that we don’t know when we’ve successfully acquired the assertion. The only signal we get is the invalidation (which may be a failure to acquire).
Comment 8 Geoffrey Garen 2021-05-03 22:14:43 PDT
(In reply to Chris Dumez from comment #7)
> I was thinking of renaming the flag to m_wasInvalidated so it is clearer
> what it means. The definition of is valid is a bit ambiguous otherwise.

m_wasInvalidated sounds good to me. Clearer. And it makes the initialization case clearer. (Anything with 'was' in the name must start out false, since nothing has happened yet.) (Remember to set wasInvalidated in the !pid case.)
Comment 9 Chris Dumez 2021-05-03 22:24:54 PDT
Created attachment 427639 [details]
Patch
Comment 10 Chris Dumez 2021-05-03 22:29:46 PDT
(In reply to Geoffrey Garen from comment #8)
> (In reply to Chris Dumez from comment #7)
> > I was thinking of renaming the flag to m_wasInvalidated so it is clearer
> > what it means. The definition of is valid is a bit ambiguous otherwise.
> 
> m_wasInvalidated sounds good to me. Clearer. And it makes the initialization
> case clearer. (Anything with 'was' in the name must start out false, since
> nothing has happened yet.) (Remember to set wasInvalidated in the !pid case.)

isValid() was already taking care of the !pid case:
```
bool ProcessAssertion::isValid() const
{
-    return m_rbsAssertion && m_rbsAssertion.get().valid;
+    return m_rbsAssertion && !m_wasInvalidated;
}
```
Comment 11 Chris Dumez 2021-05-03 23:15:17 PDT
Created attachment 427640 [details]
Patch
Comment 12 Geoffrey Garen 2021-05-04 10:54:13 PDT
Comment on attachment 427640 [details]
Patch

r=me
Comment 13 EWS 2021-05-04 11:04:23 PDT
Committed r276969 (237298@main): <https://commits.webkit.org/237298@main>

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