Closed Bug 1250250 Opened 6 years ago Closed 6 years ago

Store updated telemetry SEQ_COUNT before starting the telemetry service

Categories

(Firefox for Android Graveyard :: General, defect)

45 Branch
All
Android
defect
Not set
normal

Tracking

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

RESOLVED FIXED
Firefox 47
Tracking Status
firefox45 + wontfix
firefox46 + fixed
firefox47 + fixed
fennec 46+ ---

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

(Keywords: regression)

Attachments

(1 file)

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.
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?
https://hg.mozilla.org/mozilla-central/rev/eaf669f32777
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Marking affected, regression from 45.
Should this uplift to beta?
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+
(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+
Flags: needinfo?(michael.l.comella)
Version: unspecified → 45 Branch
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.