Bug 225324 – [iOS] Have ProcessAssertion take the RunningBoard assertion asynchronously by default

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Use async API to take RunningBoard assertions to avoid hanging the main thread.

[Chris Dumez]

<rdar://76972252>

[Chris Dumez]

Created attachment 427590 [details]
Patch

[Chris Dumez]

Created attachment 427593 [details]
Patch

[Chris Dumez]

Created attachment 427596 [details]
Patch

[Simon Fraser (smfr)]

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?

[Geoffrey Garen]

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".

[Chris Dumez]

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).

[Geoffrey Garen]

(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.)

[Chris Dumez]

Created attachment 427639 [details]
Patch

[Chris Dumez]

(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;
}
```

[Chris Dumez]

Created attachment 427640 [details]
Patch

[Geoffrey Garen]

Comment on attachment 427640 [details]
Patch

r=me

[EWS]

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].

[Chris Dumez]

Reverted r276969 for reason:

Causes previous assertion to get released before the new one is taken (asynchronously)

Committed r279412 (239277@main): <https://commits.webkit.org/239277@main>

[Chris Dumez]

Created attachment 432716 [details]
Patch

[Chris Dumez]

Created attachment 432718 [details]
Patch

[Chris Dumez]

Created attachment 432719 [details]
Patch

[Chris Dumez]

Created attachment 432740 [details]
Patch

[Chris Dumez]

Created attachment 432819 [details]
Patch

[Chris Dumez]

Had to rebase because of Bug 227552 landing.

[Chris Dumez]

Created attachment 432829 [details]
Patch

[Chris Dumez]

Comment on attachment 432829 [details]
Patch

EWS is happy, patch is ready for review.

[Geoffrey Garen]

Comment on attachment 432829 [details]
Patch

r=me

[EWS]

Committed r279601 (239425@main): <https://commits.webkit.org/239425@main>

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

[Chris Dumez]

Follow-up in https://commits.webkit.org/r279679 to address assertions on the iOS debug bots.