1860020 - Debug builds with telemetry off are unusable

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Pier Angelo Vendrame]

For Tor Browser we disable the telemetry, both at compile time with MOZ_TELEMETRY_REPORTING and at runtime with various preferences (that we also lock, to prevent the code from changing them).

However, when building in debug mode with our configuration, CheckTelemetryPref (introduced with Bug 1444275 from what I can tell) asserts the pref value is the same as TelemetryPrefValue.

This results in at least two kinds of breakage (all tab crashing when the startup page is about:blank or a startup crash in the main process otherwise).

I think this could be avoided by changing the pref value to be false when MOZ_TELEMETRY_REPORTING isn't defined (and possibly not asserting in that case).

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1444275

Comment 2

[Pier Angelo Vendrame]

Attachment: Bug 1860020: Do not assert on telemetry values when MOZ_TELEMETRY_REPORTING is not defined. r?KrisWright

Bug 1444275 introduced an assertion on the parent process to check that
the telemetry preference value is as expected.
However, this expected value did not take MOZ_TELEMETRY_REPORTING into
account, therefore it prevented debug builds to work whenever telemetry
was disabled both at build time and with preferences.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1444275

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:KrisWright, could you have a look please?

For more information, please visit BugBot documentation.

Comment 5

[Pier Angelo Vendrame]

On the review there was a question about the macro I used, but I'm unsure on how to answer.
Should I find also a reviewer from the telemetry group?
Otherwise, could someone please land the patch for me, since I don't have the permissions?
Thanks in advance.

Comment 6

[Tom Ritter [:tjr]]

Travis, could you weigh in on the MOZ_TELEMETRY_REPORTING macro in phab, or redirect to someone?

Comment 7

[Travis Long [:travis_]]

I'm going to defer this to :chutten from my team who I feel would be a more suitable person to answer this question due to his experience with Desktop telemetry.

Comment 8

[Chris H-C :chutten]

Ugh, not the tangled mess of Telemetry prefs and defines again. Oh well, let's see what we're working with here.

If I understand the complaint properly, the problem is MOZ_ASSERT(NS_SUCCEEDED(Preferences::GetBool(kTelemetryPref, &value)) && value == TelemetryPrefValue());. Given how complicated Telemetry defines are, I'd recommend dealing with this as narrowly as possible. For example:

MOZ_TELEMETRY_REPORTING is not defined for local, unofficial, developer builds which need some parts of the Telemetry system active in order to write and run instrumentation tests. (This is accomplished through having everything besides upload working). This way of working must be preserved.

So what should we do here. Hrm. Well, since non-debug builds are operating fine, the condition that the assert prevents in debug must not be crucial. CheckTelemetryPref is only called in non-Android non-parent processes, and according to the comment, aims to ensure the return value of TelemetryPrefValue() matches what's stored in kTelemetryPref. Wait. But kTelemetryPref is toolkit.telemetry.enabled which "controls a diminishing number of things and is intended to be deprecated, and then removed.". Specifically, it doesn't and hasn't for at least eight years controlled whether Telemetry is on in Firefox Desktop.

(It hearkens from a time when "Telemetry" and "Firefox Health Report (FHR)" were two different systems, which hasn't been true since Fx42 circa 2015).

That this code is in place at all was because it was used to control whether Fennec's (not Fenix's) Telemetry was enabled because Fennec didn't get the Telemetry unification memo because it never had FHR.

This is all to say: I am aware of no situations where CheckTelemetryPref is worth calling: It's checking the value of a pref that's only valid in non-unified Telemetry which hasn't existed in Firefox Desktop for eight years and hasn't existed in Mobile since we moved to Fenix and it's only checking that when not building for Android... it's a perfect storm of being useless. Alas.

Why is it still there? Well, four years ago I had to deprioritize bug 1577863 in favour of other more pressing matters (Project Shredder, Accounts Ecosystem Telemetry, and the protoypes of what became Firefox on Glean and GLAM)

All this is to say that an ideal patch would de-fennec-strate mozilla-central or at least Firefox Desktop Telemetry. But a good-enough patch would be to just remove CheckTelemetryPref entirely.

Does this help?

Comment 9

[Pier Angelo Vendrame]

(In reply to Chris H-C :chutten|PTO (back November 29) from comment #8)

All this is to say that an ideal patch would de-fennec-strate mozilla-central or at least Firefox Desktop Telemetry. But a good-enough patch would be to just remove CheckTelemetryPref entirely.

Does this help?

Yes, thanks!

Should I also remove SetupTelemetryPref? It feels kinda strange it also tries to lock the value.

Comment 10

[Chris H-C :chutten]

(Sorry for the delayed response, I'm now back from PTO)

I'd leave SetupTelemetryPref for now as if we remove it we should really consider this a "remove pref toolkit.telemetry.enabled" action and I don't want to think too deeply about that right now. These things tend to spider pretty outrageously, and we probably all have other things we'd rather be doing : )

Comment 11

[Pier Angelo Vendrame]

(In reply to Chris H-C :chutten from comment #10)

(Sorry for the delayed response, I'm now back from PTO)

No problem at all, I saw it but commented anyway before I forgot about it 😄️.

I'd leave SetupTelemetryPref for now as if we remove it we should really consider this a "remove pref toolkit.telemetry.enabled" action and I don't want to think too deeply about that right now. These things tend to spider pretty outrageously, and we probably all have other things we'd rather be doing : )

Works for me, thanks again for your answers 😊️

Comment 12

[Pier Angelo Vendrame]

Attachment: Bug 1860020 - Remove the assertion on the value of toolkit.telemetry.enabled. r=KrisWright,chutten

Bug 1444275 introduced an assertion on the parent process to check that
the telemetry preference value is as expected.
However, this expected value did not take MOZ_TELEMETRY_REPORTING into
account, therefore it prevented debug builds to work whenever telemetry
was disabled both at build time and with preferences.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4d36b144efe
Remove the assertion on the value of toolkit.telemetry.enabled. r=KrisWright,chutten

Comment 14

[Narcis Beleuzu [:NarcisB]]

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

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:pierov, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox121 to wontfix.

For more information, please visit BugBot documentation.