843816 - Detect duplicate sessions

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Detect duplicate sessions, v1

Investigation in bug 842824 revealed we could perform duplicate session recording in FHR. This bug tracks just that issue.

The attached patch should fix the issue. While I changed the behavior of getState() and setState(), I audited the code base for existing consumers and nothing appears to be impacted.

Comment 1

[Richard Newman [:rnewman]]

Comment on attachment 716773 [details] [diff] [review]
Detect duplicate sessions, v1

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

::: services/healthreport/providers.jsm
@@ +501,5 @@
>      this._log.debug("Found " + Object.keys(sessions).length + " previous sessions.");
>  
>      let daily = this.getMeasurement("previous", 3);
>  
> +    let lastRecordedSession = yield this.getState("lastSession");

There's a coupling between prunedIndex and lastSession. If one of these gets changed and not the other, we'll stop recording sessions for some period of time.

Please note this in a comment, and consider whether you want to use a sentinel default value for prunedIndex to know if you ought to reset lastSession.

::: services/healthreport/tests/xpcshell/test_provider_sessions.js
@@ +164,5 @@
> +
> +  // If we try to insert a lower-numbered session, it will be ignored.
> +  let recorder3 = new SessionRecorder("testing.collect.sessions.");
> +  recorder3._currentIndex = 2;
> +  recorder3._prunedIndex = recorder3._currentIndex;

I would rather you achieve this by swapping in a copy of the internal state of recorder2 prior to flushing, which more closely mirrors the real-life behavior.

::: services/metrics/dataprovider.jsm
@@ +664,5 @@
>  
>    /**
>     * Obtain persisted provider state.
>     *
> +   * Provider state consists of key-value pairs of strings names and values.

s/strings/string

@@ +686,5 @@
>  
> +  /**
> +   * Set state for this provider.
> +   *
> +   * This is the complementary API for `getState`.

… and obeys the same restrictions on storage use.

Comment 2

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6df33f26df

Will nom for 21.

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 716773 [details] [diff] [review]
Detect duplicate sessions, v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Firefox Health Report
User impact if declined: Crashes or lack of prefs save on shutdown may cause explosive growth of FHR data.
Testing completed (on m-c, etc.): None yet. Should let this bake for a few days first.
Risk to taking this patch (and alternatives if risky): Code is contained to JS in FHR. Should be very low risk.
String or UUID changes made by this patch: None

Comment 4

[Richard Newman [:rnewman]]

Nom tracking-21 for baking.

Comment 5

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/1d6df33f26df

Comment 6

[Alex Keybl [:akeybl]]

Comment on attachment 716773 [details] [diff] [review]
Detect duplicate sessions, v1

Low risk change preventing one possible case of FHR-caused instability.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fa0f989692c

Comment 8

[u279076]

This seems to be working okay. I'm not seeing any explosive growth in my FHR data after forcing some restarts and triggering some crashes. I'm not seeing any degradation in stability either. I'm marking this verified fixed but I'll continue to track it as we move up to Beta in case something arises.