Closed Bug 1561012 Opened 6 years ago Closed 6 years ago

Unknown category for SetEventRecordingEnabled: fxmonitor

Categories

(Firefox :: Firefox Monitor, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: robwu, Assigned: nhnt11)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

I've noticed the following warning in the global JS console when I start Firefox Nightly (buildID 20190618041042), and also reproduced this in Firefox Beta (68.0b12):

Unknown category for SetEventRecordingEnabled: fxmonitor

In Firefox 67.0.4:

Unkown category for SetEventRecordingEnabled.

This was introduced in bug 1525519 (which added the code), and exposed in bug 1531838 (which enabled FxMonitor by default in Fx68, which exposes this issue; apparently the feature is also enabled via Normandy, which triggers the issue in Firefox 67 as well).

This warning also shows that the extensions.fxmonitor.telemetryDisabled preference does not work at all. This preference was added when Fxmonitor was moved in-tree, because data review was pending (bug 1525977). Since the collection request has been approved, it should be fine to remove this preference.

The proper fix to this bug (besides potentially removing the preference) is to move the telemetry event registration to Events.yaml.

Hi Nihanth, can we get a priority on this one? (Came up in regression triage) Thanks!

Flags: needinfo?(nhnt11)

The issue is that the events are dynamically registered, and I seem to have missed that the documentation says that dynamically registered events can't be disabled.

The solution (without messing with Events.yaml) is to put the registration itself behind the pref rather than using the pref for enabling/disabling. I don't want to use Events.yaml until we implement Firefox Monitor as a direct browser feature rather than an extension - if we ever do.

Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Flags: needinfo?(nhnt11)
Priority: -- → P1
Attachment #9075762 - Attachment description: Bug 1561012 - Don't register fxmonitor events if telemetry is disabled. r=johannh → Bug 1561012 - Don't register fxmonitor events if telemetry is disabled. r=johannh,robwu

Nihanth, what's the status of the patch on this bug? Should Luke take it over?

Flags: needinfo?(nhnt11)
Flags: needinfo?(lcrouch)

Bah, sorry, this got lost in my backlog since I was on PTO for 2 weeks. I just need to land it, I'll take care of that. Thanks for the bump!

Flags: needinfo?(nhnt11)
Flags: needinfo?(lcrouch)
Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/90dad26c3b63 Don't register fxmonitor events if telemetry is disabled. r=johannh,robwu
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Is this something we should consider uplifting to Beta for Fx69 or can it ride to release with Fx70?

Flags: needinfo?(nhnt11)

It's not critical to uplift, but it's also easy to do so and eliminates some console spam, so maybe we should do it. I was worried that the patch may need rebasing due to the formatting changes but it seems like those are in beta already so we should be good. I'll request uplift. Thanks!

Flags: needinfo?(nhnt11)

Comment on attachment 9075762 [details]
Bug 1561012 - Don't register fxmonitor events if telemetry is disabled. r=johannh,robwu

Beta/Release Uplift Approval Request

  • User impact if declined: Not much - there will be some noise in the browser console.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch does some slight refactoring to avoid a function call that was redundant and caused an error in the console.
  • String changes made/needed: none
Attachment #9075762 - Flags: approval-mozilla-beta?

Comment on attachment 9075762 [details]
Bug 1561012 - Don't register fxmonitor events if telemetry is disabled. r=johannh,robwu

Fixes logspam by not trying to register fxmonitor events if Telemetry is disabled. Approved for 69.0b10.

Attachment #9075762 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: