1004506 - Telemetry experiments: NaN days remaining when having multiple experiments under about:addons

Status: VERIFIED FIXED

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

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: "NaN" day's remaining under about:addons

When you have multiple experiments listed under the add-ons manager, the "days remaining" will appear as NaN until the add-ons manager is refreshed. I'm not sure if this is because it's the same experiment with a different ID but it's worth checking out to see if this could happen to our users when there's multiple experiments being listed under about:addons.

- Attached a screenshot to illustrate the issue

Steps to reproduce the issue:

1) Open Firefox Nightly and install the first initial experiment
2) Go to about:addons and select the "Experiments" container and leave it opened
3) Change the ID of the same experiment in firefox-manifest.json
4) Once changed, open about:config in a new tab and enable/disable/enable "experiments.enabled" (should install another instance of the same experiment)
5) Switch back to the "about:addons" tab and you'll notice that "Active" will appear as NaN (refreshing the manager will set this back to the correct value)

Current Behaviour:

- Days remaining appears as NaN before refreshing the add-ons manager

Expected Behaviour:

- We should displaying the correct value rather than displaying "NaN"

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix install state handling

After some confusion around the binding constructors etc., this turned out to be rather simple.
I cleared up some other install state related handling along the way.

Manually verified, automatic test coverage is coming in bug 1000114.

Comment 2

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8418884 [details] [diff] [review]
Fix install state handling

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

I don't know CSS, so I can't comment on the extensions.css change.

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1378,3 @@
>              this._experimentState.value = gStrings.ext.GetStringFromName(stateKey);
>  
> +            if (!showProgress) {

I'd prefer the logic through here to be structured like:

if (showProgress) {
  this._experimentState.value = gStrings...("installInstalling")
} else {
  stateKey = prefix + ...
  this._experimentState.value = ...
  let now = Date.now();
  ...
}

(or, if you really feel strongly about only having one copy of the line
  this._experimentState.value = gStrings.ext.GetStringFromName()
set stateKey in the if/then and move the this._experimentState.value assignment to after the whole if/then)

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix install state handling

With the CSS parts i guess this needs a look from Blair.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8419532 [details] [diff] [review]
Fix install state handling

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

::: toolkit/mozapps/extensions/content/extensions.xml
@@ +1368,5 @@
>              let prefix = "experiment.";
>              let active = this.mAddon.isActive;
>  
> +            if (showProgress) {
> +              this._experimentState.value = gStrings.ext.GetStringFromName("installInstalling")

Should just be hiding the experiment state here, this installing message will already be shown.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: "NaN" day's remaining under about:addons

Fixed above issue.

https://tbpl.mozilla.org/?tree=Try&rev=7e7fe36be8cb

Comment 6

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/fx-team/rev/eddce10b8511

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eddce10b8511

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Turns out something in this push was causing frequent browser_experiments.js failures on OSX 10.8 opt builds. Backed out.
https://hg.mozilla.org/mozilla-central/rev/7586c29906b5

https://tbpl.mozilla.org/php/getParsedLog.php?id=39384864&tree=Fx-Team

Comment 9

[Georg Fritzsche [:gfritzsche]]

Relanded per bug 1000114, comment 8:
https://hg.mozilla.org/integration/fx-team/rev/0f50b9a4d1e6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f50b9a4d1e6

Comment 11

[Marco Mucci [:MarcoM]]

Hi Juan, can you assign a QA contact for verification of this bug.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: "NaN" day's remaining under about:addons

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Telemetry experiments.
User impact if declined: Broken labels and state for experiment addons in addon manager UI when the UI stays open over installation and installation completion.
Testing completed (on m-c, etc.): Baked fine on m-c, general QA coverage around these bugs.
Risk to taking this patch (and alternatives if risky): Low, well understood and should only improve things.
String or IDL/UUID changes made by this patch: None.

Comment 13

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b766031e174d

Comment 14

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-22-03-02-04-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-22-00-40-03-mozilla-aurora/

- Ensured that I could reproduce the original issue 
- Changed the ID of the same experiment 8 different times and ensured that NaN wasn't appear under any of the experiments under about:addons
- Ensured that the previous experiments are being "disabled" and aren't appearing as NaN once a new experiment is installed
- Moved the date forward on the test machine and ensured that the correct time is being listed under all experiments and not NaN
- Ensured that all the experiments are appearing under about:support correctly
- Ensured that both the "Learn More" and "Telemetry Settings" buttons at the top of the about:addons container are working
- Ensured that you can double click or select "more" to expand individual experiments
- Ensured that the UI is being correctly displayed
- Ensured that "extensions.bootstrappedAddons" is empty and doesn't include the experiments