Store updated telemetry SEQ_COUNT before starting the telemetry service

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

(Blocks: 1 bug, {regression})

45 Branch
Firefox 47
All
Android
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45+ wontfix, firefox46+ fixed, firefox47+ fixed, fennec46+)

Details

Attachments

(1 attachment)

There's a very small chance that we get killed after starting the service but before updating the sequence count on disk (see bug 1249308 comment 4). It's better skip seq values than to duplicate them since if we hit a network error, we already skip seq values.

Note that the update on disk should be blocking so we're sure we updated the value before we start the upload service.
Created attachment 8722117 [details]
MozReview Request: Bug 1250250 - Store updated seq no. before sending upload Intent. r=mfinkle

Review commit: https://reviewboard.mozilla.org/r/35865/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35865/
Attachment #8722117 - Flags: review?(mark.finkle)
Comment on attachment 8722117 [details]
MozReview Request: Bug 1250250 - Store updated seq no. before sending upload Intent. r=mfinkle

https://reviewboard.mozilla.org/r/35865/#review32537
Attachment #8722117 - Flags: review?(mark.finkle) → review+
https://reviewboard.mozilla.org/r/35865/#review32539

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:3981
(Diff revision 1)
> +        // We store synchronously before sending the Intent to ensure this sequence number will not be re-used.

I wouldn't mind a blank line before the comment
Comment on attachment 8722117 [details]
MozReview Request: Bug 1250250 - Store updated seq no. before sending upload Intent. r=mfinkle

Since the doc IDs are the same in the duplicate pings (bug 1249308 comment 5), it's unlikely that this is the cause for the duplicate pings – let's just uplift to aurora.

Approval Request Comment
[Feature/regressing bug #]: bug 1205835, unlikely bug 1249308
[User impact if declined]: Users in really unlikely cases may upload duplicate sequence numbers for telemetry since we start the upload attempt before we store the data.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Extremely low – we move an existing line before the code to start the service.

[String/UUID change made/needed]: None
Attachment #8722117 - Flags: approval-mozilla-aurora?

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eaf669f32777
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Marking affected, regression from 45.
Should this uplift to beta?
status-firefox45: --- → affected
status-firefox46: --- → affected
tracking-firefox45: --- → +
tracking-firefox46: --- → +
tracking-firefox47: --- → +
Flags: needinfo?(michael.l.comella)
Keywords: regression
Comment on attachment 8722117 [details]
MozReview Request: Bug 1250250 - Store updated seq no. before sending upload Intent. r=mfinkle

Fix for minor recent regression in telemetry, ok to uplift to aurora.
Attachment #8722117 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 9

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/904b04ea24c3
status-firefox46: affected → fixed
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #7)
> Marking affected, regression from 45.
> Should this uplift to beta?

Since we don't know for sure that this is causing issues and it's close to the merge date, I'm fine letting this ride the trains.
tracking-fennec: --- → 46+
status-firefox45: affected → wontfix
Flags: needinfo?(michael.l.comella)
Version: unspecified → 45 Branch
You need to log in before you can comment on or make changes to this bug.