Bug 171435 – [iOS] We should not take a background assertion for the UIProcess when app is MobileMail

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

We should not take a background process assertion for the UIProcess when app is MobileMail. Let MobileMail handle its own process assertions.

[Chris Dumez]

<rdar://problem/31132330>

[Chris Dumez]

Created attachment 308545 [details]
Patch

[Geoffrey Garen]

Comment on attachment 308545 [details]
Patch

Aren't there lots of apps that will have their own process assertions?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 308545 [details]
> Patch
> 
> Aren't there lots of apps that will have their own process assertions?

My understanding is that Mail is special in that it has has an unbounded power assertion (allowing to stay alive as long as it wants in the background).

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> (In reply to Geoffrey Garen from comment #3)
> > Comment on attachment 308545 [details]
> > Patch
> > 
> > Aren't there lots of apps that will have their own process assertions?
> 
> My understanding is that Mail is special in that it has has an unbounded
> power assertion (allowing to stay alive as long as it wants in the
> background).

Normally, having both the app and us taking assertions is not an issue. However, it seems to be an issue for Mail due to the special kind of assertion they are using.

[Geoffrey Garen]

Comment on attachment 308545 [details]
Patch

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

It's still not super clear to me why it's OK to avoid taking any assertion in Mail. If we're busy writing to a file, we might not finish.

> Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:215
> +    // There is no need to take UIAssertion if the application is MobileMail and it has its own (rdar://problem/31132330).

I'd change this comment to "Mail has a special entitlement to do unbounded work in the background. Avoid taking an assertion because it might drain the battery".

(In reply to Geoffrey Garen from comment #6)
> Comment on attachment 308545 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308545&action=review
> 
> It's still not super clear to me why it's OK to avoid taking any assertion
> in Mail. If we're busy writing to a file, we might not finish.
> 
> > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:215
> > +    // There is no need to take UIAssertion if the application is MobileMail and it has its own (rdar://problem/31132330).
> 
> I'd change this comment to "Mail has a special entitlement to do unbounded
> work in the background. Avoid taking an assertion because it might drain the
> battery".

Either way it’s phrased, it’s evident that this is a poor way to address the issue. Instead of checking for the presence of the entitlement, or taking an explicit cue from the client app that it wants this behavior, it is making a policy decision based on a correlation that exists in the shipping software, but may not exist in the future. Even worse, if the correlation ceases to exist, there would be no obvious signal to the Mail developers or the WebKit developers that it has happened. That’s why I proposed a different approach when I was asked about this before.

[Chris Dumez]

(In reply to mitz from comment #7)
> (In reply to Geoffrey Garen from comment #6)
> > Comment on attachment 308545 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=308545&action=review
> > 
> > It's still not super clear to me why it's OK to avoid taking any assertion
> > in Mail. If we're busy writing to a file, we might not finish.
> > 
> > > Source/WebKit2/UIProcess/ios/ProcessAssertionIOS.mm:215
> > > +    // There is no need to take UIAssertion if the application is MobileMail and it has its own (rdar://problem/31132330).
> > 
> > I'd change this comment to "Mail has a special entitlement to do unbounded
> > work in the background. Avoid taking an assertion because it might drain the
> > battery".
> 
> Either way it’s phrased, it’s evident that this is a poor way to address the
> issue. Instead of checking for the presence of the entitlement, or taking an
> explicit cue from the client app that it wants this behavior, it is making a
> policy decision based on a correlation that exists in the shipping software,
> but may not exist in the future. Even worse, if the correlation ceases to
> exist, there would be no obvious signal to the Mail developers or the WebKit
> developers that it has happened. That’s why I proposed a different approach
> when I was asked about this before.

Ok, I can add SPI for this behavior instead.

[Chris Dumez]

Created attachment 308575 [details]
WIP Patch

[Chris Dumez]

Created attachment 308581 [details]
Patch

[Chris Dumez]

Created attachment 308582 [details]
Patch

[Chris Dumez]

Comment on attachment 308582 [details]
Patch

Dan, is this what you had in mind?

(In reply to Chris Dumez from comment #12)
> Comment on attachment 308582 [details]
> Patch
> 
> Dan, is this what you had in mind?

API-wise it looks good! I have no idea about the implementation :-)

[Chris Dumez]

Comment on attachment 308582 [details]
Patch

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

> Source/WebKit2/UIProcess/ProcessThrottler.cpp:97
> +    if (m_shouldTakeUIBackgroundAssertion)

I basically added a shouldTakeUIBackgroundAssertion ProcessPoolConfiguration flag and based on this flag I take either a ProcessAssertion or a ProcessAndUIAssertion in the ProcessThrottler.
ProcessAssertion was already a base class of ProcessAndUIAssertion but I had to make it virtual so I could use polymorphism here.

I think this is nicer than my previous approach that was claiming to take a ProcessAndUIAssertion but then the implementation would sometimes not take a UI Assertion. Behavior-wise though, this should be equivalent to my previous patch.

[Geoffrey Garen]

Comment on attachment 308582 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 308582 [details]
Patch

Clearing flags on attachment: 308582

Committed r215954: <http://trac.webkit.org/changeset/215954>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.