1359801 - Improve keeping track of observer topics in TelemetrySession

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

As highlighted in bug 1346223 comment 4, introducing a variable [1] in TelemetrySession.jsm to keep track of the state of an observer for safely removing it is becoming a common pattern.

Instead of introducing a new boolean variable every time we add a new observer, we should:

- create an addObserver function in TelemetrySession to register a single observer; this function will call Services.obs.addObserver and keep track of the active observer in `this._observedTopics`;
- register all the current observers in TelemetrySession using this new helper [2].
- create a removeObserver helper function that calls Services.obs.removeObservers and removes the observer reference from `this._observedTopics`.

[1] - http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/toolkit/components/telemetry/TelemetrySession.jsm#662
[2] - http://searchfox.org/mozilla-central/search?q=addObserver&case=false&regexp=false&path=TelemetrySession.jsm

Comment 1

[Georg Fritzsche [:gfritzsche]]

Alejandro might look into this bug.

Initial steps to get started are:
- building Firefox: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
- applying changes per comment 0
- running tests: mach test toolkit/components/telemetry/tests/unit
- running eslint: mach eslint toolkit/components/telemetry/tests/unit
- submitting a patch for review: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 2

[Alejandro Rodriguez Salamanca [:alejandro]]

This bug looks like an interesting starting point for me to start contributing to Mozilla. I will work on it during next week.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Great, assigning it to you.

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> - running tests: mach test toolkit/components/telemetry/tests/unit

The single unit test that should work first is:
> mach test toolkit/components/telemetry/tests/unit/test_TelemetrySession.js

Comment 4

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Create addObserver and removeObserver functions in TelemetrySession to register a single observer

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8866318 [details] [diff] [review]
Create addObserver and removeObserver functions in TelemetrySession to register a single observer

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

This is a solid first pass.
I left some comments for improvements below; some of these are pretty specific to the Firefox JavaScript code.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +722,5 @@
>    _lastEnvironmentChangeDate: 0,
> +  // Keep track of the active observers
> +  _observedTopics: [],
> +
> +  addObserver: function addObserver(anObserver, aTopic) {

Below you always call this as `this.addObserver(this, ...)`.
Lets just drop the first argument?
Ditto for `removeObserver()`.

Not that for new methods we use the short-hand method syntax:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions

@@ +729,5 @@
> +  },
> +
> +  removeObserver: function removeObserver(anObserver, aTopic) {
> +    Services.obs.removeObserver(anObserver, aTopic)
> +    this._observedTopics.splice(this._observedTopics.indexOf(aTopic), 1 )

It seems like the better datatype for `_observedTopics` is a `Set`.
Switching to that, you can just use .add()/.has()/.delete().
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/

@@ +1842,4 @@
>     */
>    uninstall: function uninstall() {
>      this.detachObservers();
> +    if (this._observedTopics.includes("sessionstore-windows-restored")) {

I think we can simplify this a lot.
- `detachObservers()` is only called from this function, so we can just roll the contents in here.
- We should be able to just remove any observers that are left in `this._observedTopics`, so this can become a single loop.

If that fails any tests lets talk about what needs fixing.

@@ +1976,1 @@
>        this._hasXulWindowVisibleObserver = false;

Left-over?

@@ +1981,4 @@
>        }
>        break;
>      case "sessionstore-windows-restored":
> +      this.removeObserver(this, "sessionstore-windows-restored");

Left-over?

Comment 6

[Alejandro Rodriguez Salamanca [:alejandro]]

I have already changed the signature of `addObserver` and `removeObserver` as suggested, and changed both method to use the short-hand method syntax.

I have also changed `_activeObservers` from `Array` to `Set`.

Regarding the `uninstall()` function, I have moved the code of `detachObservers()` to `uninstall()`. About removing any observer left in `_oberverdTopics`, would it be correct to use the function `clear()`?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/clear

The tests seems to pass.


Finally,
@@ +1981,4 @@
>        }
>        break;
>      case "sessionstore-windows-restored":
> +      this.removeObserver(this, "sessionstore-windows-restored");

Why is this a left-over?

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Alejandro Rodriguez Salamanca from comment #6)
> Regarding the `uninstall()` function, I have moved the code of
> `detachObservers()` to `uninstall()`. About removing any observer left in
> `_oberverdTopics`, would it be correct to use the function `clear()`?
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Set/clear

Yes, `clear()` is correct to use.
  
> Finally,
> @@ +1981,4 @@
> >        }
> >        break;
> >      case "sessionstore-windows-restored":
> > +      this.removeObserver(this, "sessionstore-windows-restored");
> 
> Why is this a left-over?

I seem to have to put the comment on the wrong line. The next line is:
> this._hasWindowRestoredObserver = false;
This explicit "has observer" flag should not be needed anymore.

Comment 8

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Apply requested changes in to TelemetrySession

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8869937 [details] [diff] [review]
Apply requested changes in  to TelemetrySession

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

This looks close, thanks!

There are two smaller things i commented on below, then i think this should be good.

Note that this doesn't apply cleanly on the current mozilla-central code anymore. You will have to rebase before we can land this.
Also, the commit message should be of the form "Bug xxx - Short patch description."

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1841,3 @@
>      }
> +
> +    this._observedTopics.clear()

Ah, i think i misunderstood your question:
`clear()` empties the `Set`, but does not remove the observers (it is just a set of strings really).

We should loop over `_observedTopics`, calling `removeObserver` on each topic.
That already removes them from the `Set`, so we don't need to call `clear()`.

Also, we don't need to call `removeObserver()` for "idle-daily" and `TOPIC_CYCLE_COLLECTOR_BEGIN` separately that way.
You probably need to wrap the logic in the loop in the try-catch pattern above and move the comment there.

@@ +1944,4 @@
>      switch (aTopic) {
>      case "content-child-shutdown":
>        // content-child-shutdown is only registered for content processes.
> +      this.removeObserver("content-child-shutdown");

We call `uninstall()` right after this, which will remove the observer.
Can we remove this or does that break any tests?

Comment 10

[Alejandro Rodriguez Salamanca [:alejandro]]

Oh, okay, now I understand what you mean with the loop. 

I'm not sure what you mean with:

> You probably need to wrap the logic in the loop in the try-catch pattern above and move the comment there.

Do I have to put the logic for removing "idle-daily" and `TOPIC_CYCLE_COLLECTOR_BEGIN` inside of the loop and check whether the item is equal to "idle-daily" or `TOPIC_CYCLE_COLLECTOR_BEGIN` to treat those two cases in a different way?


Here,
@@ +1944,4 @@
>      switch (aTopic) {
>      case "content-child-shutdown":
>        // content-child-shutdown is only registered for content processes.
> +      this.removeObserver("content-child-shutdown");

It's possible to remove the last line and tests still pass, so I'll do it.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Alejandro Rodriguez Salamanca from comment #10)
> Oh, okay, now I understand what you mean with the loop. 
> 
> I'm not sure what you mean with:
> 
> > You probably need to wrap the logic in the loop in the try-catch pattern above and move the comment there.
> 
> Do I have to put the logic for removing "idle-daily" and
> `TOPIC_CYCLE_COLLECTOR_BEGIN` inside of the loop and check whether the item
> is equal to "idle-daily" or `TOPIC_CYCLE_COLLECTOR_BEGIN` to treat those two
> cases in a different way?

An error in removing observers is not critical.
So lets keep it simple and just always do the try-catch + warning, not treating any specific cases differently.

Comment 12

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Remove left-overs and remove observed topics at uninstall

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8870945 [details] [diff] [review]
Remove left-overs and remove observed topics at uninstall

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

This looks good now, thanks!

Can you update the other patch with these changes?
For patch reviews on bugzilla here, we want see the patch updated with the changes.
"hg qfold" can be helpful here.

The first patch still fails to apply for me, please make sure to rebase this to the current mozilla-central code.

Comment 14

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: resolve conflicts

Comment 15

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: merge all previous patches

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8871476 [details] [diff] [review]
merge all previous patches

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

This looks good, thanks!

One small thing: can you change the commit message to be meaningful and add me as the reviewer?
E.g. "Bug NNN - What this patch does. r=gfritzsche" [0]

If you use mercurial queues, this can updated using "hg qref -m ..." or "hg qref -e".

0: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

Comment 17

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Create addObserver and removeObserver functions to keep track of the active observers

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8872445 [details] [diff] [review]
Create addObserver and removeObserver functions to keep track of the active observers

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

Thanks.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99a694a8b9a8
Improve keeping track of observer topics in TelemetrySession. r=gfritzsche

Comment 20

[Georg Fritzsche [:gfritzsche]]

(In reply to Pulsebot from comment #19)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/99a694a8b9a8
> Improve keeping track of observer topics in TelemetrySession. r=gfritzsche

I changed the message a little to be more about the "what" instead of the "how", i hope that works for you.

Comment 21

[Alejandro Rodriguez Salamanca [:alejandro]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #20)
> I changed the message a little to be more about the "what" instead of the
> "how", i hope that works for you.

Yes, no problem with that. Now I know how to do it right the next time. Thanks!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99a694a8b9a8