SelfSupportBackend should not rely on FHR prefs

VERIFIED FIXED in Firefox 41

Status

()

P1
normal
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox40 wontfix, firefox41+ verified, firefox42 verified)

Details

(Whiteboard: [rC] [unifiedTelemetry] [uplift3])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1120356
(Assignee)

Updated

3 years ago
Assignee: nobody → alessio.placitelli
Whiteboard: [b5] [unifiedTelemetry] [uplift2]
(Assignee)

Comment 1

3 years ago
Created attachment 8630060 [details] [diff] [review]
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
Attachment #8630060 - Flags: review?(gfritzsche)

Updated

3 years ago
Priority: -- → P1
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 `=`.
Attachment #8630060 - Flags: review?(gfritzsche) → review+
https://hg.mozilla.org/mozilla-central/rev/2eca494baf74
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Whiteboard: [b5] [unifiedTelemetry] [uplift2] → [rC] [unifiedTelemetry] [uplift3]
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.
Attachment #8631435 - Flags: approval-mozilla-aurora?
status-firefox40: --- → wontfix
status-firefox41: --- → affected

Comment 8

3 years ago
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.
Attachment #8631435 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 9

3 years ago
Tracked. Requesting QE team to verify when possible. Thanks!
tracking-firefox41: --- → +
Flags: qe-verify+

Comment 11

3 years ago
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!
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 12

3 years ago
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
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 13

3 years ago
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

3 years ago
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
status-firefox41: fixed → verified

Comment 15

3 years ago
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
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.