Chrome always waits 3 minutes to suspend after backgrounding. |
|||||||||||||||
Issue description
This is related to how iOS CriticalClosure's work with a BLOCK_SHUTDOWN trait vs a SKIP_ON_SHUTDOWN.
Per base/task_scheduler/task.cc, tasks with a delay and BLOCK_SHUTDOWN trait are overriden as SKIP_ON_SHUTDOWN. This means if the task has not started, the task is skipped during shutdown. If the task has started, it still blocks shutdown.
On iOS, tasks with a BLOCK_SHUTDOWN trait immediately create an ios::ScopedCriticalAction regardless of delay, which immediately requests a beginBackgroundTaskWithExpirationHandler token.
This means doing the following in sql/initialization.cc:
base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
FROM_HERE, base::BindOnce(&RecordSqliteMemoryWeek),
base::TimeDelta::FromDays(7));
In a thread created from components/webdata_services/web_data_service_wrapper.cc:
auto db_task_runner = base::CreateSingleThreadTaskRunnerWithTraits(
{base::MayBlock(), base::TaskPriority::USER_VISIBLE,
base::TaskShutdownBehavior::BLOCK_SHUTDOWN});
Will permanently block suspend on iOS (until Jetsam gives up at 180 seconds)
,
Jul 9
My brief suggestion over IM, without having looked at this code, was that it sounds as if the PostDelayedTask() in question should instead be a (one shot or repeating? Dunno) timer, which when fired posts a non-delayed task to the appropriate task runner to actually collect the data.
,
Jul 9
,
Jul 9
,
Jul 9
https://chromium-review.googlesource.com/c/chromium/src/+/1129829 is simple enough that perhaps it should be considered for M68.
,
Jul 9
pkl@ I'm also seeing CCTClearcutUploader and CCTClearcutUploader-wrapper as a long running task we might considering removing. It takes about 13 seconds of background usage and does eventually exit.
,
Jul 9
+danblakemore for comment 6 about CCTClearcutUploader
,
Jul 9
For the CCTClearcut* tasks, these are intentional and should be left in place, if possible. This is the upload task for the log transport used by GrowthKit.
,
Jul 10
,
Jul 10
,
Jul 10
Dan: do those uploader tasks only run when going to the background, or are they started during normal state and can complete before the app is suspended, if necessary?
,
Jul 10
,
Jul 10
cmasso@/ kariahda@ I think the workaround to disable the sql tasks is safe to land on M68. The real fix can probably wait until M70. Marking this as M68 blocker. crrev.com/c/1129829 for the workaround should land shortly.
,
Jul 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19ba5438485d0a4abdab60f495921559fcad9e53 commit 19ba5438485d0a4abdab60f495921559fcad9e53 Author: Justin Cohen <justincohen@google.com> Date: Tue Jul 10 16:47:30 2018 sql: Disable very long running ScopedCriticalAction's on iOS. We are currently marking all critical actions as iOS long running tasks, even tasks with delays. Because some tasks have long delays, we always use the maximum 3 minutes before the system suspends us. This should be reverted once crbug.com/861889 is fixed and delayed long running tasks are not marked as iOS-critical until they begin running. Bug: 861889 Change-Id: I3654ac9b5d94b5d52abef14a79253bd8a9290077 Reviewed-on: https://chromium-review.googlesource.com/1129829 Commit-Queue: Justin Cohen <justincohen@chromium.org> Reviewed-by: Rohit Rao <rohitrao@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#573770} [modify] https://crrev.com/19ba5438485d0a4abdab60f495921559fcad9e53/sql/initialization.cc
,
Jul 10
,
Jul 10
This bug requires manual review: Less than 10 days to go before AppStore submit on M68 Please contact the milestone owner if you have questions. Owners: cmasso@(Android), kariahda@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 10
Is it possible to get canary verification for this?
,
Jul 11
Mike: The way it's set to upload currently is with the "startAutoUpload" method here: https://cs.corp.google.com/piper///depot/google3/googlemac/Shared/Metrics/Clearcut/Classes/CCTClearcutUploader.h?g=0&l=200 What that does is set clearcut to upload on background, on foreground after 5 seconds, and in the foreground every 15 minutes allowing the system to delay that for up to 5 minutes (using dispatch_source_set_timer). With that in mind, it is likely they will always perform the update on background for any events logged in a session shorter than 15 minutes.
,
Jul 11
Given that most browser sessions are likely < 15minutes, this means that all updates will be performed when going to the background. I'm not sure that's what we want, or what most clients of a library expect. We want to minimize networking when the user stops using the app, not guarantee it. Let's discuss this via email.
,
Jul 16
+ Srikanth for canary verification.
,
Jul 16
It will take some time for us to verify the fix. I will do some test setup with the fixed builds on test devices and monitor the usage. Please go ahead with the merge approval. I will post the usage states after I get them.
,
Jul 16
Thanks Srikanth!
,
Jul 17
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/93d0b1f443873539b0cab73f5a28b6542ae2d673 commit 93d0b1f443873539b0cab73f5a28b6542ae2d673 Author: Justin Cohen <justincohen@google.com> Date: Tue Jul 17 16:34:57 2018 sql: Disable very long running ScopedCriticalAction's on iOS. We are currently marking all critical actions as iOS long running tasks, even tasks with delays. Because some tasks have long delays, we always use the maximum 3 minutes before the system suspends us. This should be reverted once crbug.com/861889 is fixed and delayed long running tasks are not marked as iOS-critical until they begin running. Bug: 861889 Change-Id: I3654ac9b5d94b5d52abef14a79253bd8a9290077 Reviewed-on: https://chromium-review.googlesource.com/1129829 Commit-Queue: Justin Cohen <justincohen@chromium.org> Reviewed-by: Rohit Rao <rohitrao@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#573770}(cherry picked from commit 19ba5438485d0a4abdab60f495921559fcad9e53) Reviewed-on: https://chromium-review.googlesource.com/1140553 Reviewed-by: Justin Cohen <justincohen@chromium.org> Cr-Commit-Position: refs/branch-heads/3440@{#693} Cr-Branched-From: 010ddcfda246975d194964ccf20038ebbdec6084-refs/heads/master@{#561733} [modify] https://crrev.com/93d0b1f443873539b0cab73f5a28b6542ae2d673/sql/initialization.cc
,
Jul 19
Verified on iPhone6s, iOS11.4.1 M68.0.3440.70 canary by checking for the logs "dump all assertions". Also verified the chrome batter usage from iOS device Battery stats and everything looks normal. |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by justincohen@chromium.org
, Jul 9