Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rnewman, Assigned: jhugman)

Tracking

(Depends on: 2 bugs)

unspecified
All
iOS
Dependency tree / graph

Firefox Tracking Flags

(fxios-v3.0 wontfix, fxios-v4.0 fixed, fxios+)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Reporter)

Updated

3 years ago
No longer blocks: 1166812
(Reporter)

Updated

3 years ago
tracking-fxios: --- → ?
OS: iOS 8 → iOS
(Reporter)

Comment 1

3 years ago
Need to find the card!
tracking-fxios: ? → 2.0+
We should have up to 10 mins of background execution time when the user leaves the app to run a long-running background task to perform the sync:

https://developer.apple.com/library/ios/documentation/iPhone/Conceptual/iPhoneOSProgrammingGuide/BackgroundExecution/BackgroundExecution.html

Shouldn't take longer than 10 mins. Need to double check to see what we're allowed to do during that time.
(Reporter)

Updated

3 years ago
Summary: Sync on exit and/or soon after launch → Sync on exit
(Reporter)

Updated

3 years ago
See Also: → bug 1168502
(Reporter)

Updated

3 years ago
Rank: 4
tracking-fxios: 2.0+ → +
(Assignee)

Comment 3

3 years ago
Is this as simple as calling syncEverything() in the AppDelegate, then putting the existing shutdown in an upon block?

https://github.com/mozilla/firefox-ios/blob/master/Client/Application/AppDelegate.swift#L293
Flags: needinfo?(rnewman)
(Assignee)

Updated

3 years ago
Assignee: nobody → jhugman
Status: NEW → ASSIGNED
(Reporter)

Comment 4

3 years ago
Almost. Two big things to think about: the green light and DB closure.

Syncs can take an unbounded amount of time. For that reason the caller provides a 'green light' function. The one we use now limits any sync to one minute and foreground-only: if we've exceeded the time limit or entered the background when the sync operation checks in, it'll gracefully give up on the rest of the sync.

https://github.com/mozilla/firefox-ios/blob/873459e170ba8d5fac47ee8fad1f20bf726ab3d3/Providers/Profile.swift#L966-L966

Running a background sync will involve changing that behavior, otherwise we'll immediately abort the new sync!

We can probably remove the foreground-only constraint altogether, but to do so we will ought to make sure we don't forcibly shut down the profile (closing its databases) until a sync — either one that's already in progress when we background, or one we trigger in the background — is done.

The existing shutdown code attempts to close each of our two databases by nilling out the shared DB connection. (The deinitializer for the connection instance will take care of the rest.)

Our hypothesis is that this is relatively safe in the presence of an existing sync, but we almost certainly do leave a connection open sometimes, and we don't want to close these then immediately reopen them.

It's not enough to fire off a sync with an upon -- we might already be syncing, and so we need to grab the existing sync's Deferred if one exists.

To do that, calls to syncEverything (or individual syncs, like the one we trigger in onLoginDidChange) should put their `Deferred` results somewhere, so there's always a hook to chain off an ongoing sync.

Right now we use a spinlock (syncLock in Profile.swift) to track whether a sync is in progress. That should probably be some kind of atomic property instead.
Flags: needinfo?(rnewman)
(Assignee)

Comment 5

3 years ago
Created attachment 8718560 [details] [review]
Pull request

This feels like it's on the right track, but I haven't done anything with atomicity/spinlocks yet.

I'm also not happy with having needing an extra method (or its name) on the SyncManager.
Attachment #8718560 - Flags: feedback?(rnewman)
(Reporter)

Comment 6

3 years ago
Comment on attachment 8718560 [details] [review]
Pull request

Comments on the PR. I think everything should be pushed down into `syncSeveral`; callers shouldn't be aware that a sync is in process, so on exit we can just call

  self.profile.syncManager.syncEverything()

and if a sync for all engines is in progress, we'll just return that; if not, we'll chain off the existing sync to sync the remainder; if there isn't one, we'll run a new full sync.
Attachment #8718560 - Flags: feedback?(rnewman) → feedback+
Comment hidden (spam)
Comment hidden (spam)
Please stop spamming bugs.
Flags: needinfo?(jhugman)
(Reporter)

Comment 10

3 years ago
Test scenarios:

* Background the app with no account set up. Nothing untoward should happen.
* Likewise, but with the account in Unverified (create a new account). Sync shouldn't work, because the account isn't syncable.
* Start a sync from Settings, then immediately background the app. There should be no additional sync.
* Background the app seconds after launch (never synced before). Sync or don't sync, but specify the behavior!
* Allow a scheduled sync to run (fifteen seconds after launch) and complete, then background the app within the rate-limiting period. We expect no sync.
* Likewise, but outside the rate-limiting period. We expect another sync.
* Save a login. Wait for the post-login-change sync to occur, then background the app. We expect another sync to occur immediately, syncing everything but logins.
(Assignee)

Comment 11

3 years ago
I've just pushed more commits to PR https://github.com/mozilla/firefox-ios/pull/1526 . It needs some more feedback.

rnewman: please could you have a look at it.

Also: are there any automated tests for this (it doesn't feel untestable, just that it hasn't been automated yet)?
Flags: needinfo?(rnewman)
(Reporter)

Comment 12

3 years ago
You could test the sync continuation stuff, but syncing on exit itself is a little more challenging -- I don't think we have good control over the OS lifecycle callbacks.
(Reporter)

Updated

3 years ago
Flags: needinfo?(rnewman)
Attachment #8718560 - Flags: feedback+ → feedback?
(Reporter)

Comment 13

3 years ago
Comment on attachment 8718560 [details] [review]
Pull request

Comments on the PR. Nice work!
Attachment #8718560 - Flags: feedback? → feedback+
(Assignee)

Updated

3 years ago
Attachment #8718560 - Flags: review?(rnewman)
(Assignee)

Comment 14

3 years ago
I think this is ready for a review now. I've kept using the overall structure approximately the same, while (I think) addressing the feedback from earlier f+s.
(Reporter)

Comment 15

3 years ago
Comment on attachment 8718560 [details] [review]
Pull request

Relatively speaking just nits, but they're the kind of nits that would make some people squint, so happy to take another look at this after.

Tests would be super appreciated for this, but adding tests would be difficult without breaking out a little library for intermediate deferreds with accumulation. Consider whether you want to do that so you can test it…
Attachment #8718560 - Flags: review?(rnewman)
(Assignee)

Comment 16

3 years ago
Comment on attachment 8718560 [details] [review]
Pull request

Left a comment in the PR, but I think this is ready for review.
Attachment #8718560 - Flags: review?(rnewman)
(Reporter)

Comment 17

3 years ago
Comment on attachment 8718560 [details] [review]
Pull request

Lovely. Really nice to see tests for this!

I added my nits and an extra test as four commits here:

https://github.com/mozilla/firefox-ios/compare/rnewman/async-reducer?expand=1
Attachment #8718560 - Flags: review?(rnewman) → review+
(Assignee)

Comment 18

3 years ago
Addressed nits, tested on device.

Merged. Yay!
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
tracking-fxios: + → 4.0+
(Reporter)

Comment 19

3 years ago
https://github.com/mozilla/firefox-ios/commit/9df6ce66e1dd43d86a21ca6d71836984b0aece42

Thanks for your patience, James!
status-fxios-v3.0: --- → wontfix
status-fxios-v4.0: --- → fixed
tracking-fxios: 4.0+ → +
Target Milestone: --- → 4.0
Depends on: 1264225
Depends on: 1264248

Updated

3 years ago
Depends on: 1264279
You need to log in before you can comment on or make changes to this bug.