Improve keeping track of observer topics in TelemetrySession

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: Dexter, Assigned: Alejandro Rodriguez Salamanca, Mentored)

Tracking

Trunk
mozilla55
Points:
2

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [measurement:client][lang=js])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

7 months ago
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
(Reporter)

Updated

7 months ago
Points: --- → 2
Priority: -- → P3
Whiteboard: [measurement:client][lang=js]
status-firefox57: affected → ---
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
Mentor: gfritzsche@mozilla.com
(Assignee)

Comment 2

7 months ago
This bug looks like an interesting starting point for me to start contributing to Mozilla. I will work on it during next week.
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
Assignee: nobody → alexrs95
(Assignee)

Comment 4

6 months ago
Created attachment 8866318 [details] [diff] [review]
Create addObserver and removeObserver functions in TelemetrySession to register a single observer
Attachment #8866318 - Flags: review?(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?
Attachment #8866318 - Flags: review?(gfritzsche)
(Assignee)

Comment 6

6 months ago
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?
Flags: needinfo?(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.
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 8

6 months ago
Created attachment 8869937 [details] [diff] [review]
Apply requested changes in  to TelemetrySession
(Assignee)

Updated

6 months ago
Attachment #8866318 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8869937 - Flags: review?(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?
Attachment #8869937 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 10

6 months ago
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.
Flags: needinfo?(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.
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 12

6 months ago
Created attachment 8870945 [details] [diff] [review]
Remove left-overs and remove observed topics at uninstall
(Assignee)

Updated

6 months ago
Attachment #8870945 - Flags: review?(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.
Attachment #8870945 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 14

6 months ago
Created attachment 8871472 [details] [diff] [review]
resolve conflicts
(Assignee)

Updated

6 months ago
Attachment #8870945 - Attachment is obsolete: true
(Assignee)

Comment 15

6 months ago
Created attachment 8871476 [details] [diff] [review]
merge all previous patches
(Assignee)

Updated

6 months ago
Attachment #8871472 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8869937 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8871476 - Flags: review?(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
Attachment #8871476 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 17

6 months ago
Created attachment 8872445 [details] [diff] [review]
Create addObserver and removeObserver functions to keep track of the active observers
(Assignee)

Updated

6 months ago
Attachment #8871476 - Attachment is obsolete: true
(Assignee)

Updated

6 months ago
Attachment #8872445 - Flags: review?(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.
Attachment #8872445 - Flags: review?(gfritzsche) → review+

Comment 19

6 months ago
Pushed by georg.fritzsche@googlemail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99a694a8b9a8
Improve keeping track of observer topics in TelemetrySession. r=gfritzsche
Summary: Store and cleanly remove active observers → Improve keeping track of observer topics in TelemetrySession
(In reply to Pulsebot from comment #19)
> Pushed by georg.fritzsche@googlemail.com:
> 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.
(Assignee)

Comment 21

6 months ago
(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

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/99a694a8b9a8
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.