Summary: | [iOS] We should not take a background assertion for the UIProcess when app is MobileMail | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | WebKit2 | Assignee: | 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
Chris Dumez
2017-04-28 10:10:51 PDT
Created attachment 308545 [details]
Patch
Comment on attachment 308545 [details]
Patch
Aren't there lots of apps that will have their own process assertions?
(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). (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 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. (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. Created attachment 308575 [details]
WIP Patch
Created attachment 308581 [details]
Patch
Created attachment 308582 [details]
Patch
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 :-) 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 on attachment 308582 [details]
Patch
r=me
Comment on attachment 308582 [details] Patch Clearing flags on attachment: 308582 Committed r215954: <http://trac.webkit.org/changeset/215954> All reviewed patches have been landed. Closing bug. |