Closed Bug 1430396 Opened 6 years ago Closed 6 years ago

TypeError: event.target is undefined when check on/off "Allow Nightly to send technical and interaction data to Mozilla"

Categories

(Firefox :: Settings UI, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: magicp.jp, Assigned: myk)

References

Details

Attachments

(2 files)

Steps to reproduce:
1. Launch Nightly
2. Go to about:preferences#privacy > Nightly Data Collection and Use
3. Check off(or on) "Allow Nightly to send technical and interaction data to Mozilla"

Actual results:
The following error is logged in the browser console.

TypeError: event.target is undefined: injectOptOutStudyCheckbox/<@resource://shield-recipe-client/lib/ShieldPreferences.jsm:135:13
emit@resource://gre/modules/EventEmitter.jsm:152:11
set value@chrome://global/content/preferencesBindings.js:479:9
observe@chrome://global/content/preferencesBindings.js:97:9
updateSubmitHealthReport@chrome://browser/content/preferences/in-content/privacy.js:1583:5
EventListener.handleEvent*setEventListener@chrome://browser/content/preferences/in-content/privacy.js:244:7
init@chrome://browser/content/preferences/in-content/privacy.js:411:5
init@chrome://browser/content/preferences/in-content/preferences.js:55:7
initializeCategories@chrome://browser/content/preferences/in-content/findInPage.js:70:11
init/<@chrome://browser/content/preferences/in-content/findInPage.js:27:40
requestIdleCallback handler*init@chrome://browser/content/preferences/in-content/findInPage.js:27:7
init_all@chrome://browser/content/preferences/in-content/preferences.js:73:3
EventListener.handleEvent*@chrome://browser/content/preferences/in-content/preferences.js:61:1
  EventEmitter.jsm:156

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d760cf06ca40056077e2a354b8b69350a5765ae3&tochange=0adedb70b7883ab39239b8531d07f1308ffc10c7


Expected results:
No error.
Blocks: 1379338
Has Regression Range: --- → yes
Has STR: --- → yes
I can't reproduce this on Mac, so I'm building with `export MOZ_TELEMETRY_REPORTING=1` on Linux to try to reproduce there.
Assignee: nobody → myk
Status: NEW → ASSIGNED
Over in https://github.com/mozilla/normandy/pull/938, Osmose landed https://github.com/mozilla/normandy/pull/938/commits/abac7dd484f025ae196458544787241170b9e5df, which tries to avoid leaks by avoiding a "direct reference" to the about:preferences document and instead retrieving the optOutStudiesEnabled checkbox via event.target.ownerDocument.

I regressed this in bug 1379338, which converts preference instances from EventTarget to EventEmitter, so their event objects no longer have an Event.target property.

There are a couple of issues here.

First, if I understand correctly, the existing code doesn't actually avoid leaking anything, as the event listener function closes over the outer function's scope, so the symbols defined in that scope—including the |doc| argument and the |checkbox| const—will continue to be held until the listener is collected, whether or not the listener references them.

If we really want to ensure that the document doesn't leak because of the event listener, we would need to remove it on unload.  I don't think that's actually necessary, however.  I've done it speculatively in this patch to show how it would be done, but I recommend landing the fix without that change.

The second issue is that browser/extensions/shield-recipe-client/test/browser/browser-about-preferences.js doesn't seem to get run in automation, which is why I didn't catch this regression sooner.  It looks like browser/extensions/shield-recipe-client/test/browser/browser.ini skips the test if FHR/Telemetry aren't available:

[browser_about_preferences.js]
# Skip this test when FHR/Telemetry aren't available.
skip-if = !healthreport || !telemetry

Are there any plans to run this test in automation, perhaps on GitHub (via the existing TaskCluster and CircleCI integrations)?  Even if it isn't in Treeherder, and thus doesn't get caught by engineers/sheriffs upstream, it'd be helpful to at least catch it downstream in the GitHub repo.
Attachment #8943356 - Flags: review?(gijskruitbosch+bugs)
(Note: while I was here, I removed the obsolete optOutPref const.)
> Are there any plans to run this test in automation, perhaps on GitHub (via the existing TaskCluster and CircleCI integrations)?  Even if it isn't in Treeherder, and thus doesn't get caught by engineers/sheriffs upstream, it'd be helpful to at least catch it downstream in the GitHub repo.

Downstream runs tests the same way as upstream (via ./mach mochitest), so the only way this is going to run is if it is enabled in Treeherder as well as Github. I'm not sure why this test is disabled in the first place though. I'll have to look more in to this.

However, the Normandy client's days of being a pseudo-separate project on Github are numbered anyways. I'm planning to move this code fully in-tree this quarter, so this idea of having a separate up and downstream will go away.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> The second issue is that
> browser/extensions/shield-recipe-client/test/browser/browser-about-
> preferences.js doesn't seem to get run in automation, which is why I didn't
> catch this regression sooner.  It looks like
> browser/extensions/shield-recipe-client/test/browser/browser.ini skips the
> test if FHR/Telemetry aren't available:
> 
> [browser_about_preferences.js]
> # Skip this test when FHR/Telemetry aren't available.
> skip-if = !healthreport || !telemetry
> 
> Are there any plans to run this test in automation, perhaps on GitHub (via
> the existing TaskCluster and CircleCI integrations)?  Even if it isn't in
> Treeherder, and thus doesn't get caught by engineers/sheriffs upstream, it'd
> be helpful to at least catch it downstream in the GitHub repo.

I assume that the reason this is conditioned on healthreport/telemetry is that you can't run into this problem if the telemetry/healthreport checkbox is disabled, which it seems to be for my local builds, at least. I don't know how other telemetry testing gets around this.
(In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> If we really want to ensure that the document doesn't leak because of the
> event listener, we would need to remove it on unload.  I don't think that's
> actually necessary, however. 

Why not?
Flags: needinfo?(myk)
Comment on attachment 8943356 [details] [diff] [review]
reuse optOutStudiesEnabled checkbox reference, remove event listener on unload

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

r=me with the off() thing fixed rather than removed.

Note that we will need to port this over to github pending it merging to m-c. But in any case, it's going to be a bit interesting if shield want to ship to multiple versions of Firefox including ones with and without the scriptification of the prefs.

:mythmon, can you clarify the requirements from shield's POV? If this code needs to support older Firefox versions which still have the XBL-based prefs, we will need to make more changes here.

::: browser/extensions/shield-recipe-client/lib/ShieldPreferences.jsm
@@ +128,5 @@
> +    function onChangeFHRPref(event) {
> +      checkbox.disabled = !Services.prefs.getBoolPref(FHR_UPLOAD_ENABLED_PREF);
> +    }
> +    fhrPref.on("change", onChangeFHRPref);
> +    doc.addEventListener("unload", () => fhrPref.off(onChangeFHRPref), { once: true });

This doesn't actually work... `off`'s first parameter isn't the event listener, but the event type - just like on() ?
Attachment #8943356 - Flags: review?(mcooper)
Attachment #8943356 - Flags: review?(gijskruitbosch+bugs)
Attachment #8943356 - Flags: review+
Comment on attachment 8943356 [details] [diff] [review]
reuse optOutStudiesEnabled checkbox reference, remove event listener on unload

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

> Note that we will need to port this over to github pending it merging to m-c. But in any case, it's going to be a bit interesting if shield want to ship to multiple versions of Firefox including ones with and without the scriptification of the prefs.

Thanks for thinking about porting this to github, but this won't be necessary anymore. Normandy is going to move to being in-tree primarily, and we'll be removing the code on github soon.

> :mythmon, can you clarify the requirements from shield's POV? If this code needs to support older Firefox versions which still have the XBL-based prefs, we will need to make more changes here.

We don't plan to ship this change to older Firefoxes, so I think this change is good as  is.
Attachment #8943356 - Flags: review?(mcooper) → review+
(In reply to :Gijs from comment #6)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #2)
> > If we really want to ensure that the document doesn't leak because of the
> > event listener, we would need to remove it on unload.  I don't think that's
> > actually necessary, however. 
> 
> Why not?

I haven't been able to figure that out.

The event listener function closes over the scope of its outer function (injectOptOutStudyCheckbox), which references its Document argument (doc), which references its Window property (defaultView), which references the Preferences global defined by preferencesBindings.js, which references a property containing a hash of Preference instances (_all), which references a Preference instance for the datareporting.healthreport.uploadEnabled pref, which references a property containing a Map of arrays of event listeners (_eventEmitterListeners), which references the event listener.

That's true with or without this change, which doesn't alter that cycle.  So I would expect the page to leak.  Nevertheless, after unloading about:preferences and triggering garbage collection, the page no longer appears in the about:memory report (both with and without this change).

So it doesn't appear to be necessary to remove the listener on unload.  Nevertheless, I've left that in, fixing the issue you identified with the call to EventEmitter.off.  I also fixed a second issue I noticed after further testing: the target for the addEventListener call was |doc| but needs to be |doc.defaultView|.

This is the version of the patch I'll commit.
Flags: needinfo?(myk)
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/484983773650
reuse optOutStudiesEnabled checkbox reference, remove event listener on unload; r=Gijs,mythmon
https://hg.mozilla.org/mozilla-central/rev/484983773650
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.