1136434 - request-sync still occurs after sync task has been unregistered

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[James Burke [:jrburke]]

This was found on master/trunk pvt nightlies on the Flame device, using the email app. Once an email account is configured with the email app:

* Open email app, tap menu button in upper left corner.
* Select gear icon to go to settings.
* Tap 5 times fairly quickly *below* the Add Account button to bring up secret debug menu.
* Tap the "Enable faster sync options" button, then press Back button.
* Tap an account to go to the account settings.
* For the "Check for new messages" item, select 100 seconds.
* Then long-tap the home button to bring up the app switcher and completely close the email app.
* Wait for the sync to occur, you can watch the adb logcat to see happen.

Once a sync does happen, go back to the account settings, and set the "Check for new messages" to "Manually". This will unregister the sync task. Long tap home to completely kill the email app.

You can verify that no syncs are registered, or at least visible to the APIs exposed to apps, by installing the requestsyncmanager app:

https://github.com/jrburke/requestsyncmanager

Or by using the secret debug menu to list the sync tasks in the logcat.

However, if you wait another 100 seconds or so, the logcat shows the email app starting up again and syncing.

So there is something that is still registered with the requestsync plumbing, even though the requestsync APIs exposed to the apps do not show any task.

The disconcerting part is a restart of the phone does not fix the problem, syncs still occur, with no mechanism to turn them off.

This affects master and 2.2/the mozilla37-b2g branch. So once fixed on master, it should be uplifted for the branch used by b2g 2.2.

Comment 1

[Andrea Marchesini [:baku]]

Can I reproduce this issue with the latest gecko+gaia?

Comment 2

[James Burke [:jrburke]]

I was seeing this with yesterday's pvt flashes, and I saw it even before then, so yes, I expect it still to be there in the latest gecko+gaia.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: rs.patch

When I removed the timer from the _registrations object, I forgot this one.

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8570183 [details] [diff] [review]
rs.patch

Let's add fabrice too. Fabrice reviewed the previous code.

Comment 5

[(no longer active)]

Comment on attachment 8570183 [details] [diff] [review]
rs.patch

Review of attachment 8570183 [details] [diff] [review]:
-----------------------------------------------------------------

Please add a test!

Comment 6

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5ec730b1db

Comment 7

[Andrea Marchesini [:baku]]

The funny thing is that we have a test. But we don't check if the task is re-triggered because we don't use a setTimeout just for this. I don't think we want to add this, because timers are usually a problem in mochitests.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Timers as such aren't the problem.
It is that they are occasionally used in a way which makes the tests flaky.
If you for example need to test that something *does not happen*, you need to use a timer
(and hope the timer value is long enough to catch the theoretical "does happen" case).

Comment 9

[(no longer active)]

I agree with Olli.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3f5ec730b1db

Comment 11

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: it's affecting email app. we'll need the fix for 2.2 (gecko37)

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8570183 [details] [diff] [review]
rs.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1018320
User impact if declined: deleting tasks from requestSyncService will not remove the timers, so the tasks are still schedulated once.
Testing completed: on device
Risk to taking this patch (and alternatives if risky): no risks. 
String or UUID changes made by this patch: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/60bf2d08cebb