Closed Bug 1004506 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

(firefox31+ verified, firefox32 verified)

VERIFIED FIXED
Firefox 32
Tracking Status
firefox31 + verified
firefox32 --- verified

People

(Reporter: kjozwiak, Assigned: gfritzsche)

References

Details

(Whiteboard: p=3 s=it-32c-31a-30b.2 [qa!])

Attachments

(1 file, 4 obsolete files)

Attached image "NaN" day's remaining under about:addons (obsolete) —
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"
Flags: firefox-backlog?
Whiteboard: p=3
Version: unspecified → Trunk
Blocks: telex
Assignee: nobody → georg.fritzsche
Flags: firefox-backlog? → firefox-backlog+
Flags: needinfo?(jbecerra)
Whiteboard: p=3 → p=3 s=IT-32c-31a-30b.1 [qa+]
Attached patch Fix install state handling (obsolete) — Splinter Review
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.
Attachment #8418884 - Flags: review?(irving)
Status: NEW → ASSIGNED
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)
Attachment #8418884 - Flags: review?(irving) → review-
Attached patch Fix install state handling (obsolete) — Splinter Review
With the CSS parts i guess this needs a look from Blair.
Attachment #8418884 - Attachment is obsolete: true
Attachment #8419532 - Flags: review?(bmcbride)
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.
Attachment #8419532 - Flags: review?(bmcbride) → review-
Fixed above issue.

https://tbpl.mozilla.org/?tree=Try&rev=7e7fe36be8cb
Attachment #8415907 - Attachment is obsolete: true
Attachment #8419925 - Flags: review?(bmcbride)
Attachment #8419532 - Attachment is obsolete: true
Attachment #8419925 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/eddce10b8511
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 32 → ---
Whiteboard: p=3 s=IT-32c-31a-30b.1 [qa+] → p=3 s=it-32c-31a-30b.2 [qa+]
https://hg.mozilla.org/mozilla-central/rev/0f50b9a4d1e6
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Flags: needinfo?(jbecerra)
Hi Juan, can you assign a QA contact for verification of this bug.
Flags: needinfo?(jbecerra)
Flags: needinfo?(jbecerra)
QA Contact: kamiljoz
[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.
Attachment #8419925 - Attachment is obsolete: true
Attachment #8423895 - Flags: approval-mozilla-aurora?
OS: Mac OS X → All
Hardware: x86 → All
Attachment #8423895 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa+] → p=3 s=it-32c-31a-30b.2 [qa!]
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.