Bug 171435

Summary: [iOS] We should not take a background assertion for the UIProcess when app is MobileMail
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, barraclough, commit-queue, ggaren, mitz, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ggaren: review+
WIP Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-04-28 10:10:51 PDT
We should not take a background process assertion for the UIProcess when app is MobileMail. Let MobileMail handle its own process assertions.
Comment 1 Chris Dumez 2017-04-28 10:11:07 PDT
<rdar://problem/31132330>
Comment 2 Chris Dumez 2017-04-28 10:13:22 PDT
Created attachment 308545 [details]
Patch
Comment 3 Geoffrey Garen 2017-04-28 10:31:27 PDT
Comment on attachment 308545 [details]
Patch

Aren't there lots of apps that will have their own process assertions?
Comment 4 Chris Dumez 2017-04-28 10:33:41 PDT
(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).
Comment 5 Chris Dumez 2017-04-28 10:34:36 PDT
(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.
Comment 6 Geoffrey Garen 2017-04-28 10:53:12 PDT
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".
Comment 7 mitz 2017-04-28 11:00:14 PDT
(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.
Comment 8 Chris Dumez 2017-04-28 11:04:32 PDT
(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.
Comment 9 Chris Dumez 2017-04-28 12:41:21 PDT
Created attachment 308575 [details]
WIP Patch
Comment 10 Chris Dumez 2017-04-28 12:57:49 PDT
Created attachment 308581 [details]
Patch
Comment 11 Chris Dumez 2017-04-28 12:58:49 PDT
Created attachment 308582 [details]
Patch
Comment 12 Chris Dumez 2017-04-28 14:05:12 PDT
Comment on attachment 308582 [details]
Patch

Dan, is this what you had in mind?
Comment 13 mitz 2017-04-28 14:48:42 PDT
(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 :-)
Comment 14 Chris Dumez 2017-04-28 14:58:09 PDT
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.
Comment 15 Geoffrey Garen 2017-04-28 15:11:30 PDT
Comment on attachment 308582 [details]
Patch

r=me
Comment 16 WebKit Commit Bot 2017-04-28 15:41:59 PDT
Comment on attachment 308582 [details]
Patch

Clearing flags on attachment: 308582

Committed r215954: <http://trac.webkit.org/changeset/215954>
Comment 17 WebKit Commit Bot 2017-04-28 15:42:01 PDT
All reviewed patches have been landed.  Closing bug.