1179226 - SelfSupportBackend should not rely on FHR prefs

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

With FHR possibly being turned off when unified telemetry lands, we need to make sure selfsupport keeps working. Right now, it depends on FHR prefs [1].

* We should make SelfSupport work if either FHR is on or Unified Telemetry is on.
* We should check the tests and make sure the use the correct prefs.

[1] https://hg.mozilla.org/mozilla-central/annotate/079b6f1ae1c3/browser/modules/SelfSupportBackend.jsm#l82

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1179226.patch

This patch changes the SelfSupportBackend.jsm [0] module for Heartbeat to make it work with FHR turned off.

The tests don't need to be changed.

SelfSupportService.js [1] doesn't need to be changed now: about:healthreport needs to be changed to stop using the FHR helper function from MozSelfSupport first. 

I've manually checked that MozSelfSupport works (for the telemetry part) by opening about:healthreport and issuing MozSelfSupport.getTelemetry* calls from the web console.

[0] - https://hg.mozilla.org/mozilla-central/annotate/cef11c3e86c3/browser/modules/SelfSupportBackend.jsm
[1] - https://hg.mozilla.org/mozilla-central/annotate/cef11c3e86c3/browser/components/selfsupport/SelfSupportService.js

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8630060 [details] [diff] [review]
bug1179226.patch

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

::: browser/modules/SelfSupportBackend.jsm
@@ +25,5 @@
>  const PREF_URL = "browser.selfsupport.url";
>  // FHR status.
>  const PREF_FHR_ENABLED = "datareporting.healthreport.service.enabled";
> +// Unified Telemetry status.
> +const PREF_TELEMETRY_UNIFIED    = "toolkit.telemetry.unified";

Too much whitespace before the `=`.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: bug1179226.patch - v2

Thanks. Try push:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc5b47f5350

Comment 4

[Pulsebot]

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

Comment 5

[Alessio Placitelli [:Dexter]]

 https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=2eca494baf74

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2eca494baf74

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8631435 [details] [diff] [review]
bug1179226.patch - v2

Approval Request Comment
[Feature/regressing bug #]:
This is part of the uplift3 batch for the unified Telemetry project: http://bit.ly/1TCl4r8
This is targetting 41 now and these are the last minor "feature" patches blocking shipping - any further patches will be fixes for data quality etc. with very limited scope & impact.
[User impact if declined]:
Unified Telemetry can't ship.
This is a fix needed to keep selfsupport working.
[Describe test coverage new/current, TreeHerder]:
This has automated test-coverage.
[Risks and why]:
Low-risk, well understood small behavior change limited to selfsupport code.
[String/UUID change made/needed]:
No string changes.

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8631435 [details] [diff] [review]
bug1179226.patch - v2

Seems safe to uplift as this code has been in m-c for more than 2 weeks. Approved.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked. Requesting QE team to verify when possible. Thanks!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b1832903bda

Comment 11

[Ada [:adalucinet]]

Alessio, could you please provide more detailed instructions on how to verify this fix?
Based on comment 1, after calling MozSelfSupport.getTelemetry* (both Ping and Pinglist) calls from the Web Console, I get: http://i.imgur.com/ixzez9k.png with latest 41.0b8 and “ReferenceError: MozSelfSupport is not defined” is thrown with 40.0b9, but nothing different visible in about:healthreport.


Thanks in advance!

Comment 12

[Alessio Placitelli [:Dexter]]

Hi Alexandra! MozSelfSupport is not directly involved with SelfSupportBackend. To test the latter, I think your best shot is to check if it gets disabled due to FHR or UT being disabled [0], by disabling both the unified telemetry pref and the FHR service pref.

Please do not hesitate to contact me back if you need more info!

[0] - http://mxr.mozilla.org/mozilla-central/source/browser/modules/SelfSupportBackend.jsm#90

Comment 13

[Alessio Placitelli [:Dexter]]

Note: to check if it gets disabled, just check the browser console for the "init - Disabling SelfSupport because FHR and Unified Telemetry are disabled." log line.

Comment 14

[Ada [:adalucinet]]

Verified fixed with latest 41.0b8, across platforms [1] - with FHR & UT disabled and 'browser.selfsupport.log.level' set to 'Trace', the following lines are visible in the console:
1441718116253	Browser.SelfSupportBackend	TRACE	init
1441718116253	Browser.SelfSupportBackend	CONFIG	init - Disabling SelfSupport because FHR and Unified Telemetry are disabled.
 
[1] Ubuntu 14.04 32-bit, Mac OS X 10.10.4, Windows 7 64-bit

Comment 15

[Ada [:adalucinet]]

Also confirming this fix with 42.0b1 (Build ID: 20150921151815), across platforms [1].

[1] Windows 7 64-bit, Mac OS X 10.11 Beta and Ubuntu 14.04 32-bit