1184458 - TelemetryEnvironment needs to shut down properly

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

TelemetryEnvironment adds some observers that are never removed, as [1] is never called.

[1] - http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryEnvironment.jsm#814

Comment 1

[Georg Fritzsche [:gfritzsche]]

This isn't breaking anything now, so this doesn't block shipping.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Currently, nothing is calling TelemetryEnvironment._removeObservers:
https://dxr.mozilla.org/mozilla-central/rev/4f4615ffec6a6a7ec40ff61ffda90a46c53f8d31/toolkit/components/telemetry/TelemetryEnvironment.jsm#853

This points out that we don't have any proper shutdown for the TelemetryEnvironment.
What should happen, is that the TelemetryController shutdown routine calls TelemetryEnvironment.shutdown() around here:
https://dxr.mozilla.org/mozilla-central/rev/4f4615ffec6a6a7ec40ff61ffda90a46c53f8d31/toolkit/components/telemetry/TelemetryController.jsm#812

TelemetryEnvironment already has a |this._shutdown| flag, but its never set to |true|.
|TelemetryEnvironment.shutdown()| should do that and call |this._removeObservers()|.

Comment 3

[Alessio Placitelli [:Dexter]]

ni?me there to follow up with testing steps & test coverage.

Comment 4

[Alessio Placitelli [:Dexter]]

We should add a new test, just before [0], that does the following:

- Watches a test preference (e.g. "toolkit.telemetry.test.pref1")
- Registers a change listener that asserts if a change is propagated
- Shuts down TelemetryEnvironment
- Flips the testing preference
- Unregisters the listener

In order to test that everything works correctly:

./mach test toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
./mach test toolkit/components/telemetry

[0] - https://dxr.mozilla.org/mozilla-central/rev/9ddf0da90fb3bc1ae29966dc596013fc54a44bd2/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js#1200

Comment 5

[chaithanya]

I would like to work on this bug. I went through the code and got a few doubts. How do I write code to shut down TelemetryEnvironment?

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to chaithanya from comment #5)
> I would like to work on this bug. I went through the code and got a few
> doubts. How do I write code to shut down TelemetryEnvironment?

You could add a |shutdown| method just after the |unregisterChangeListener| at [0], which calls |shutdown| on the "instantiaced" TelemetryEnvironment (see what unregisterChangeListener is doing).

The implementation |shutdown| in EnvironmentCache.prototype would need to:

- Print a log line to let us know shutdown is taking place.
- Set this._shutdown to true.
- Remove the observers.
- Stop watching the preferences.

[0] - https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f3dc63e6/toolkit/components/telemetry/TelemetryEnvironment.jsm#73

Comment 7

[chaithanya]

Attachment: Made changes as per comment 6. Please let me know if there are any changes to make.

Comment 8

[Alessio Placitelli [:Dexter]]

Comment on attachment 8704241 [details] [diff] [review]
Made changes as per comment 6. Please let me know if there are any changes to make.

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

Georg should probably be reviewing this, but I'll gladly take a first look.

A few observations:

- As stated in comment 2, TelemetryController should call TelemetryEnvironment.shutdown().
- Test the patch by building your local copy (./mach build) and then 

./mach test toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
./mach test toolkit/components/telemetry

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +71,5 @@
>  
>    unregisterChangeListener: function(name) {
>      return getGlobal().unregisterChangeListener(name);
>    },
> +  _shutdown: function() {

Drop the "_" prefix, as this function is meant to be directly callable.

Also, this function should not contain the implementation, but rather |return getGlobal().shutdown();|. The implementation should live in |EnvironmentCache.prototype|, just after the implementation for |unregisterChangeListener|.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1196,5 @@
>    data = TelemetryEnvironment.currentEnvironment;
>    Assert.equal(data.settings.searchCohort, "testcohort");
>  });
>  
> + // Set the Environment preferences to watch.

You should declare your test code within an |add_task(... )| as done with other tests within this file.

Comment 9

[chaithanya]

Attachment: Made changes as by comment 8. Kindly let me know if further changes are to be made

Comment 10

[chaithanya]

Attachment: Made changes as by comment 8. Kindly let me know if further changes are to be made

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8704595 [details] [diff] [review]
Made changes as by comment 8. Kindly let me know if further changes are to be made

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

Thank you for your submission!

The code you added doesn't get called yet, as TelemetryController should call TelemetryEnvironment.shutdown(). Please follow the instructions in comment 2 for how to do that and check comment 8 on how to properly test your changes locally.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +71,5 @@
>  
>    unregisterChangeListener: function(name) {
>      return getGlobal().unregisterChangeListener(name);
>    },
> +  shutdown: function() {

Could you add a blank line before this function?

@@ +72,5 @@
>    unregisterChangeListener: function(name) {
>      return getGlobal().unregisterChangeListener(name);
>    },
> +  shutdown: function() {
> +	return getGlobal().shutdown();

Please indent this code using whitespaces (see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Indentation ).

@@ +73,5 @@
>      return getGlobal().unregisterChangeListener(name);
>    },
> +  shutdown: function() {
> +	return getGlobal().shutdown();
> +},

This should be at the same level of "shutdown" (e.g. indented with 2 whitespaces)

@@ +773,5 @@
>        return;
>      }
>      this._changeListeners.delete(name);
>    },
> +  shutdown: function{

Please add a blank line before "shutdown" and after the function implemention.

Also, it's missing "()" after function.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1195,5 @@
>    Services.obs.notifyObservers(null, "browser-search-service", "init-complete");
>    data = TelemetryEnvironment.currentEnvironment;
>    Assert.equal(data.settings.searchCohort, "testcohort");
>  });
> +add_task(function*() {

Please add a new task 

add_task(function*() {
// TEST CODE
});

and leave the other test tasks untouched.

Comment 12

[chaithanya]

Attachment: Made changes as by comment 11. Please let me know if further changes are to be made.

Comment 13

[Alessio Placitelli [:Dexter]]

Comment on attachment 8706332 [details] [diff] [review]
Made changes as by comment 11. Please let me know if further changes are to be made.

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

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +804,5 @@
>      // Now do an orderly shutdown.
>      try {
>        // Stop the datachoices infobar display.
>        TelemetryReportingPolicy.shutdown();
> +      TelemetryEnvironment.shutdown(); 

Please remove the trailing whitespaces.

@@ +805,5 @@
>      try {
>        // Stop the datachoices infobar display.
>        TelemetryReportingPolicy.shutdown();
> +      TelemetryEnvironment.shutdown(); 
> +      

Please remove the trailing whitespaces, but keep the newline.

@@ +810,3 @@
>        // Stop any ping sending.
>        yield TelemetrySend.shutdown();
> +     

Please remove the trailing whitespaces, but keep the newline.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +71,5 @@
>  
>    unregisterChangeListener: function(name) {
>      return getGlobal().unregisterChangeListener(name);
>    },
> +  

Please remove the trailing whitespaces, but keep the newline.

@@ +74,5 @@
>    },
> +  
> +  shutdown: function() {
> +    return getGlobal().shutdown();
> +},

Properly align "}," at the same level of shutdown.

@@ +774,5 @@
>        return;
>      }
>      this._changeListeners.delete(name);
>    },
> +  

Please remove the trailing whitespaces, but keep the newline.

@@ +775,5 @@
>      }
>      this._changeListeners.delete(name);
>    },
> +  
> +  shutdown: function (){

Please remove the whitespace after "function", and add it before {, so we get:

function() {

@@ +780,5 @@
> +    this._log.trace("shutdown");
> +    this._shutdown = true;
> +    this._removeObservers();
> +    this._stopWatchingPrefs();
> +    },

The closing parenthesis must be at the same level of shutdown, so two spaces to the left.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ -702,5 @@
>    Preferences.set(PREF_TEST_5, expectedValue);
>  
>    // Set the Environment preferences to watch.
>    TelemetryEnvironment._watchPreferences(PREFS_TO_WATCH);
> -  let deferred = PromiseUtils.defer();

There's no need to remove this, and that's probably what's causing problems with your tests.

@@ +1196,5 @@
>    Assert.equal(data.settings.searchCohort, "testcohort");
>  });
>  
>  add_task(function*() {
> +   do_test_finished();

There's no need to change this line.

@@ +1201,3 @@
>  });
> +
> +add_task(function* shutdown() {

Let's call this test "test_environmentShutdown" and move it before the previous add_task.

Also, properly align the code within this function and remove the trailing spaces (where needed).

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style for more information about the style conventions.

@@ +1209,5 @@
> +  Preferences.reset(PREF_TEST);
> +// Set the Environment preferences to watch.
> + TelemetryEnvironment._watchPreferences(PREFS_TO_WATCH);
> + let deferred = PromiseUtils.defer();
> + TelemetryEnvironment.registerChangeListener("testSearchEngine_pref", deferred.resolve);

I don't think we need to wait on a promise here: no change is propagated during shutdown, so it will just be stuck here, timing out your test.

It would also be a good idea to change the listener name to something different as, for example, "testEnvironmentShutdownChange": 

TelemetryEnvironment.registerChangeListener("testEnvironmentShutdownChange", () => {
  // Your handler code here.
});

@@ +1211,5 @@
> + TelemetryEnvironment._watchPreferences(PREFS_TO_WATCH);
> + let deferred = PromiseUtils.defer();
> + TelemetryEnvironment.registerChangeListener("testSearchEngine_pref", deferred.resolve);
> + // Register a new change listener that asserts if change is propogated
> +  Assert.ok(false, "No change should be propagated after shutdown.");

This will not assert if a change is detected but, instead, it will assert as soon as the code is executed, making the test fail.

You should move this function within the body of the change listener handler.

@@ +1212,5 @@
> + let deferred = PromiseUtils.defer();
> + TelemetryEnvironment.registerChangeListener("testSearchEngine_pref", deferred.resolve);
> + // Register a new change listener that asserts if change is propogated
> +  Assert.ok(false, "No change should be propagated after shutdown.");
> + //Flip the test preferences

You should shut down the environment before this line.

@@ +1214,5 @@
> + // Register a new change listener that asserts if change is propogated
> +  Assert.ok(false, "No change should be propagated after shutdown.");
> + //Flip the test preferences
> +  Preferences.set(PREF_TEST, 1); 
> +  yield deferred.promise;

This line should be removed, as we won't be using the promise here.

@@ +1216,5 @@
> + //Flip the test preferences
> +  Preferences.set(PREF_TEST, 1); 
> +  yield deferred.promise;
> +  // Unregister the listener.
> +  TelemetryEnvironment.unregisterChangeListener("testWatchPrefs_reset");

The change listener name here needs to match the one used in registerChangeListener. Please change it to "testEnvironmentShutdownChange".

Comment 14

[Benjamin Smedberg]

I'm late to the party, but I don't understand why we need to make any change at all, other than removing the unused _removeObservers method. For an object that lasts the lifetime of the application, there's usually no reason to explicitly remove observers, and it adds extra time at shutdown.

Comment 15

[Alessio Placitelli [:Dexter]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #14)
> I'm late to the party, but I don't understand why we need to make any change
> at all, other than removing the unused _removeObservers method. For an
> object that lasts the lifetime of the application, there's usually no reason
> to explicitly remove observers, and it adds extra time at shutdown.

Good point about |_removeObservers|.

Right now, in TelemetryEnvironment.jsm, we prevent rogue environment change events from interfering with the shutdown process [0]. Unfortunately, |this._shutdown| is never flipped to true. I think it would still make sense for this bug to do that, to be safe.

[0] - https://dxr.mozilla.org/mozilla-central/rev/6020a4cb41a77a09484c24a5875bb221714c0e6a/toolkit/components/telemetry/TelemetryEnvironment.jsm#752,766,1299

Comment 16

[chaithanya]

Attachment: Made changes as per comment 13. Please let me know if further changes are to be made.

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8707419 [details] [diff] [review]
Made changes as per comment 13. Please let me know if further changes are to be made.

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

chaithanya, please make sure to address the observations from the previous review rounds before requesting a new review.

Also, make sure to test the patch and that it's working. To test the patch, build your local copy (./mach build) and then 

./mach test toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
./mach test toolkit/components/telemetry

You should not see any failure.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +809,2 @@
>        // Stop any ping sending.
> +      yield TelemetrySend.shutdown()

There's a missing semicolon (";") at the end of this line.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +this.EXPORTED776_SYMBOLS = [

This line shouldn't be touched at all. A typo was added here: it should be EXPORTED_SYMBOLS, without the "776".

@@ +74,5 @@
>    },
>  
> +  shutdown: function() {
> +    return getGlobal().shutdown();
> +                        },

The closing } should be at the same level of "shutdown", so only two spaces to the right from the beginning of the line.

@@ +775,5 @@
>      }
>      this._changeListeners.delete(name);
>    },
>  
> +  shutdown: function()  {

There are two whitespaces between () and {, we just need one.

@@ +779,5 @@
> +  shutdown: function()  {
> +    this._log.trace("shutdown");
> +    this._shutdown = true;
> +    this._removeObservers();
> +    this._stopWatchingPrefs();

Since we're here, would you remove both the lines with _removeOBservers and _stopWatchingPrefs? You did the right thing, but as per comment 14 and comment 15 those need to go away.

@@ +780,5 @@
> +    this._log.trace("shutdown");
> +    this._shutdown = true;
> +    this._removeObservers();
> +    this._stopWatchingPrefs();
> +                        },

The closing } should be at the same level of "shutdown", so only two spaces to the right from the beginning of the line.

Also, add a newline after this.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +702,5 @@
>    Preferences.set(PREF_TEST_5, expectedValue);
>  
>    // Set the Environment preferences to watch.
>    TelemetryEnvironment._watchPreferences(PREFS_TO_WATCH);
> +let deferred = PromiseUtils.defer();

Please discard the changes to this line. There's no need to touch it.

@@ +1194,5 @@
>    Services.obs.notifyObservers(null, "browser-search-service", "init-complete");
>    data = TelemetryEnvironment.currentEnvironment;
>    Assert.equal(data.settings.searchCohort, "testcohort");
>  });
> +test_environmentShutdown();

You should not directly call the test function. Instead, rename the function at line 1203 from "shutdown" to "test_environmentShutdown".

@@ +1199,3 @@
>  add_task(function*() {
>    do_test_finished();
> +                      });

You should not touch this line. The closing }); should go all the way to the left.

@@ +1199,5 @@
>  add_task(function*() {
>    do_test_finished();
> +                      });
> +
> +add_task(function* shutdown() {

Please properly align the code in this function before requesting a new review.

@@ +1205,5 @@
> +  const PREF_TEST = "toolkit.telemetry.test.pref1";
> +  const PREFS_TO_WATCH = new Map([
> +    [PREF_TEST, {what: TelemetryEnvironment.RECORD_PREF_STATE}],
> +  ]);
> +  Preferences.reset(PREF_TEST);

To make sure your changes don't get throttled, please add the following two lines after this line:

gNow = futureDate(gNow, 10 * MILLISECONDS_PER_MINUTE);
fakeNow(gNow);

@@ +1211,5 @@
> + TelemetryEnvironment._watchPreferences(PREFS_TO_WATCH);
> + TelemetryEnvironment.registerChangeListener("test_environmentShutdownChange",() =>{
> +   // Register a new change listener that asserts if change is propogated
> +    Assert.ok(false, "No change should be propagated after shutdown.");
> +                                                                                    }

This is missing ); after the closing }.

@@ +1215,5 @@
> +                                                                                    }
> +    TelemetryEnvironment.shutdown();
> +   //Flip the test preferences
> +  Preferences.set(PREF_TEST, 1);
> +  yield deferred.promise;

This line can be removed, you're not declaring/using this anymore.

Comment 18

[chaithanya]

Attachment: Made changes as per comment 17 and the patch was test locally. Please let me know if any changes are to be made.

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8708200 [details] [diff] [review]
Made changes as per comment 17 and the patch was test locally. Please let me know if any changes are to be made.

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

We're almost there.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +779,5 @@
> +  shutdown: function() {
> +    this._log.trace("shutdown");
> +    this._shutdown = true;
> +  },
> +  

Please remove the trailing whitespaces here.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +699,5 @@
>    ]);
>  
>    Preferences.set(PREF_TEST_4, expectedValue);
>    Preferences.set(PREF_TEST_5, expectedValue);
> +  let deferred = PromiseUtils.defer();

Please discard *any* change to this section of the file. I see in your patch that you're trying to restore the line you deleted, but to a different location.

let deferred = PromiseUtils.defer(); should be right after TelemetryEnvironment._watchPreferences(PREFS_TO_WATCH);

@@ +1197,2 @@
>  add_task(function*() {
> + // Define and reset the test preference.

Even though the test passes, as stated in the previous reviews, you should add the test code before the final task and that task should be called "test_environmentShutdown", not "shutdown":

add_task(function* test_environmentShutdown() {
  // Your test code here.
});

add_task(function*() {
  do_test_finished();
});

@@ +1202,5 @@
> +  ]);
> +  Preferences.reset(PREF_TEST);
> +  gNow = futureDate(gNow, 10 * MILLISECONDS_PER_MINUTE);
> +  fakeNow(gNow);
> +// Set the Environment preferences to watch.

As mentioned in the previous rounds, please indent your code.

Comment 20

[chaithanya]

Attachment: Made changes as by comment 19.Patch has been tested locally.Please review.

Comment 21

[chaithanya]

Attachment: Indented the code as by comment 19. Please review.

Comment 22

[chaithanya]

Attachment: Indented the code and added the code to test preferences. Please review.

Comment 23

[Alessio Placitelli [:Dexter]]

Comment on attachment 8708307 [details] [diff] [review]
Indented the code and added the code to test preferences. Please review.

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

This looks much better. Please take care of the issues there, make sure it passes the tests and I think we're good to go.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1210,5 @@
> +  TelemetryEnvironment._watchPreferences(PREFS_TO_WATCH);
> +  TelemetryEnvironment.registerChangeListener("test_environmentShutdownChange",() =>{
> +    // Register a new change listener that asserts if change is propogated
> +    Assert.ok(false, "No change should be propagated after shutdown.");
> +  });

Please call TelemetryEnvironment.shutdown(); right after this line.

@@ +1211,5 @@
> +  TelemetryEnvironment.registerChangeListener("test_environmentShutdownChange",() =>{
> +    // Register a new change listener that asserts if change is propogated
> +    Assert.ok(false, "No change should be propagated after shutdown.");
> +  });
> +  //Flip the test preferences

nit: add a whitespace right after "//" and before "Flip".

@@ +1213,5 @@
> +    Assert.ok(false, "No change should be propagated after shutdown.");
> +  });
> +  //Flip the test preferences
> +  Preferences.set(PREF_TEST, 1);
> +  yield deferred.promise;

This should be dropped, deferred is not declared anywhere. The test will definitely fail.

@@ +1215,5 @@
> +  //Flip the test preferences
> +  Preferences.set(PREF_TEST, 1);
> +  yield deferred.promise;
> +  // Unregister the listener.
> +  TelemetryEnvironment.unregisterChangeListener("testEnvironmentShutdownChange");

You should unregister the same listener that you register before at line 1211 ("test_environmentShutdownChange").

Comment 24

[chaithanya]

Attachment: Made changes as by 23 and passed the test. Hope it gets through this time. Please review.

Comment 25

[Alessio Placitelli [:Dexter]]

Comment on attachment 8708447 [details] [diff] [review]
Made changes as by 23 and passed the test. Hope it gets through this time. Please review.

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

Thanks, this looks good to me now. There's only a nit to be addressed (see below).

I'm bouncing the review to :gfritzsche, he'll take care of it.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1207,5 @@
> +  gNow = futureDate(gNow, 10 * MILLISECONDS_PER_MINUTE);
> +  fakeNow(gNow);
> +  // Set the Environment preferences to watch.
> +  TelemetryEnvironment._watchPreferences(PREFS_TO_WATCH);
> +  TelemetryEnvironment.registerChangeListener("test_environmentShutdownChange",() =>{

nit: there should be a whitespaces after the comma and before "()"

Comment 26

[chaithanya]

Attachment: Made changes as by comment 25 and test was successful. Kindly review.

Comment 27

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8709020 [details] [diff] [review]
Made changes as by comment 25 and test was successful. Kindly review.

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

Sorry for the delay here, this week was a bit full.
This looks good, but i have a few style comments. Especially the test could be more readable.
Please address the comments below, then we can get this landed.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +804,5 @@
>      // Now do an orderly shutdown.
>      try {
>        // Stop the datachoices infobar display.
>        TelemetryReportingPolicy.shutdown();
> +      TelemetryEnvironment.shutdown();

Nit: Keep an empty line after this one.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1205,5 @@
> +  ]);
> +  Preferences.reset(PREF_TEST);
> +  gNow = futureDate(gNow, 10 * MILLISECONDS_PER_MINUTE);
> +  fakeNow(gNow);
> +  // Set the Environment preferences to watch.

Nit: Add an empty line before this comment.
The comment should reflect everything that is happening in the next section, e.g.:
// Set up the preferences and listener, then trigger shutdown.

@@ +1207,5 @@
> +  gNow = futureDate(gNow, 10 * MILLISECONDS_PER_MINUTE);
> +  fakeNow(gNow);
> +  // Set the Environment preferences to watch.
> +  TelemetryEnvironment._watchPreferences(PREFS_TO_WATCH);
> +  TelemetryEnvironment.registerChangeListener("test_environmentShutdownChange", () =>{

Nit: Add a space after the arrow, i.e.:
... () => {

@@ +1212,5 @@
> +    // Register a new change listener that asserts if change is propogated
> +    Assert.ok(false, "No change should be propagated after shutdown.");
> +  });
> +  TelemetryEnvironment.shutdown();
> +  // Flip the test preferences

Add an empty line before this and explain a bit what is expected to happen here:
// Flipping the test preference after shutdown should not trigger the listener.

@@ +1214,5 @@
> +  });
> +  TelemetryEnvironment.shutdown();
> +  // Flip the test preferences
> +  Preferences.set(PREF_TEST, 1);
> +  // Unregister the listener.

Nit: Add an empty line before this one.

Comment 28

[chaithanya]

Attachment: Made changes and tested the patch locally. Please review.

Comment 29

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8710984 [details] [diff] [review]
Made changes and tested the patch locally. Please review.

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

This looks good to me, thanks!
Sorry for the long wait, i am a bit swamped at the moment.

I have only two nits below - can you please upload a new patch with those changes, then Alessio can push this patch to try & take care of landing it if everything is fine.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +774,5 @@
>        return;
>      }
>      this._changeListeners.delete(name);
>    },
> +  shutdown: function() {

Nit: Add a newline before this method.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1194,5 @@
>    Services.obs.notifyObservers(null, "browser-search-service", "init-complete");
>    data = TelemetryEnvironment.currentEnvironment;
>    Assert.equal(data.settings.searchCohort, "testcohort");
>  });
> +add_task(function* test_environmentShutdown() {

Nit: This should have a newline before it.

Comment 30

[chaithanya]

Attachment: Made changes by comment 29. I think there's already a newline in line 744 in the patch. Please review.

Comment 31

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8712756 [details] [diff] [review]
Made changes by comment 29. I think there's already a newline in line 744 in the patch. Please review.

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

Thanks!

Comment 32

[Georg Fritzsche [:gfritzsche]]

Alessio, can you get this to try and landing?

Comment 33

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1dd21c63672

Comment 34

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/bae683bcf1560a24b869a71dcbdec5528bd155e4
Bug 1184458- TelemetryEnvironment needs to shut down properly r=gfritzsche

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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