request-sync still occurs after sync task has been unregistered

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
3 months ago

People

(Reporter: jrburke, Assigned: baku)

Tracking

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment)

Reporter

Description

4 years ago
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.
Flags: needinfo?(amarchesini)
Assignee

Comment 1

4 years ago
Can I reproduce this issue with the latest gecko+gaia?
Flags: needinfo?(amarchesini) → needinfo?(jrburke)
Reporter

Comment 2

4 years ago
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.
Flags: needinfo?(jrburke) → needinfo?(amarchesini)
Assignee

Updated

4 years ago
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
OS: Mac OS X → All
Hardware: x86 → All
Assignee

Comment 3

4 years ago
Posted patch rs.patchSplinter Review
When I removed the timer from the _registrations object, I forgot this one.
Attachment #8570183 - Flags: review?(ehsan)
Assignee

Comment 4

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

Let's add fabrice too. Fabrice reviewed the previous code.
Attachment #8570183 - Flags: review?(fabrice)

Comment 5

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

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

Please add a test!
Attachment #8570183 - Flags: review?(ehsan) → review+
Assignee

Updated

4 years ago
Attachment #8570183 - Flags: review?(fabrice)
Assignee

Comment 7

4 years ago
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.
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

4 years ago
I agree with Olli.
https://hg.mozilla.org/mozilla-central/rev/3f5ec730b1db
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
triage: it's affecting email app. we'll need the fix for 2.2 (gecko37)
blocking-b2g: 2.2? → 2.2+
Flags: needinfo?(amarchesini)
Assignee

Comment 12

4 years ago
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
Flags: needinfo?(amarchesini)
Attachment #8570183 - Flags: approval-mozilla-b2g37?
Attachment #8570183 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.