MOZ_SERVICES_HEALTHREPORT should be enabled in xulrunner

RESOLVED FIXED

Status

Firefox Health Report
Client: Desktop
P4
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
... and prefs enabling it (datareporting.healthreport.service.enabled, datareporting.healthreport.uploadEnabled) should both default to false, and have an override on the application side (i.e. in browser/app/profile/firefox.js for desktop Firefox).
Component: Telemetry → General
Product: Toolkit → Firefox

Comment 1

6 years ago
I have no clue what we should do about this. Who uses xulrunner? Is FHR that important for it? I also don't know if xulrunner has all the dependencies necessary to run FHR. If it doesn't just work when enabled in the build system, then this might be non-trivial. In the grand scheme of things, I don't think this is too important at the moment.
(Assignee)

Comment 2

6 years ago
(In reply to Gregory Szorc [:gps] from comment #1)
> I have no clue what we should do about this. Who uses xulrunner? Is FHR that
> important for it? I also don't know if xulrunner has all the dependencies
> necessary to run FHR. If it doesn't just work when enabled in the build
> system, then this might be non-trivial. In the grand scheme of things, I
> don't think this is too important at the moment.

Fedora and Debian build Firefox as a xulrunner application.
(Assignee)

Comment 3

6 years ago
It's also not completely about FHR. IIRC, since the FHR landing, Telemetry depends on healthreport.

Comment 4

6 years ago
(In reply to Mike Hommey [:glandium] from comment #2)
> Fedora and Debian build Firefox as a xulrunner application.

This is important. But, I'm not sure the market share warrants us prioritizing this over perf improvements that affect the overwhelming majority of our users. But I'm not a PM.

(In reply to Mike Hommey [:glandium] from comment #3)
> It's also not completely about FHR. IIRC, since the FHR landing, Telemetry
> depends on healthreport.

Telemetry does not depend on FHR. The notification bar and the XPCOM service that drives is independent of FHR and Telemetry. Those are enabled if one of {crash reports, Telemetry, FHR} is enabled. See MOZ_DATA_REPORTING.
Component: General → Metrics and Firefox Health Report
Product: Firefox → Mozilla Services
(Assignee)

Comment 5

6 years ago
I was mislead by my check script. The problem i was seeing is that telemetry now enables data reporting, and the data reporting module imports healthreport modules. Reading the code, it handles the lack of healthreport properly, but my check script is not that smart.

That being said, i wouldn't completely oppose to FHR not being enabled on Firefox-on-xulrunner builds, but the browser/ part enables MOZ_SERVICES_HEALTHREPORT, and /that/ adds code to the browser parts that probably don't have their gre counterparts when MOZ_SERVICES_HEALTHREPORT is not set in xulrunner.

Now, I understand that we're not necessarily interested in receiving data from such builds, but i would certainly understand users wanting access to the collected data locally. That is, that about:healthreport and about:telemetry work and display data, whether the data is being sent to a mozilla server or not. As a user, I do want this, and I find it sad that i can't see anything significant in about:healthreport without pinging a mozilla server and it's not even clear the frame displays a page from a distant server.

Comment 6

6 years ago
If /browser/confvars.sh is not the proper place to enable MOZ_SERVICES_HEALTHREPORT for dekstop but not xulrunner, where should we? 

You are correct that it is advantageous for FHR to be enabled and collecting even if it is not submitting because about:healthreport could be useful.

Because xulrunner is being used to power some Firefox deployments, I think it makes sense to enable FHR there. If downstream builders need to make modifications to whether it submits data or where it submits data, we have prefs for that.

Again, this bug is a low priority for me unless someone tells me otherwise.
(Assignee)

Comment 7

6 years ago
I can take the bug, it's not the problem. The problem is agreeing on what to do :)
Let's do it.
Assignee: nobody → mh+mozilla
Priority: -- → P2
Flagging as P4, because this is less important than addressing FHR on Android.
Priority: P2 → P4

Updated

5 years ago
Component: Metrics and Firefox Health Report → Client: Desktop
Product: Mozilla Services → Firefox Health Report
(Assignee)

Updated

5 years ago
Duplicate of this bug: 876474

Comment 11

5 years ago
Created attachment 760469 [details] [diff] [review]
Fix for build with libxul v1

Okay, reattaching patch according to bug 831688 comment 4.
Attachment #760469 - Flags: review?(gps)
Comment on attachment 760469 [details] [diff] [review]
Fix for build with libxul v1

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

This is correct from a technical sense. But, I'm not sure if all the other pieces will be present in xulrunner, notably the data policy notification bar and about:healthreport. I also don't know if all the tests will pass because I've never attempted to run them in xulrunner.

If there is no notification bar, FHR will just collect a bunch of data but will never upload it. And, if there is no about:healthreport, the user won't be able to see said data.

So, I'm reluctantly r+'ing this. We definitely need verification of some kind, however.
Attachment #760469 - Flags: review?(gps) → review+

Comment 13

5 years ago
Created attachment 763445 [details] [diff] [review]
Fix for build with libxul v2

Attaching patch with commit message. Copying review from previous version.
Attachment #763445 - Flags: review+
Attachment #763445 - Flags: checkin?

Updated

5 years ago
Attachment #760469 - Attachment is obsolete: true
Attachment #763445 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/f172342f18c2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.