1114957 - Conversation window event listeners don't get cleaned up correctly

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mike de Boer [:mikedeboer]]

See summary.

Comment 1

[Marco Mucci [:MarcoM]]

Added to IT 37.3

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: cleanup LoopRooms event listeners when the conversation window is closed

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Comment on attachment 8540675 [details] [diff] [review]
Patch v1: cleanup LoopRooms event listeners when the conversation window is closed

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

Looks good, though some minor remarks you might want to address before landing.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +204,5 @@
>       */
>      setupRoomInfo: function(actionData) {
> +      if (this._onUpdateListener) {
> +        console.error("Room info already set up!");
> +        return;

Is there any real chance for this to happen (subscribing twice)? If so, keep that test; if not, let's get rid of it.

@@ +395,5 @@
>       */
>      windowUnload: function() {
>        this._leaveRoom(ROOM_STATES.CLOSING);
>  
> +      if (!this._onUpdateListener) {

Same question, I can't see a case where this could happen; but maybe I'm missing something obvious here.

Comment 4

[Maire Reavy [:mreavy]]

After further discussion with Mike and Mark, I think this is a Fx37+, P2.

Comment 6

[Maire Reavy [:mreavy]]

Hi Mike -- Is this ready to land?

Comment 7

[Mike de Boer [:mikedeboer]]

Ah yes, thanks for being on top of things... I certainly am not!

https://hg.mozilla.org/integration/fx-team/rev/9097517c27db

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/9097517c27db

Comment 9

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: now truly, really cleanup mozLoop event listeners when a window is closed

Well, it turns out this previous patch didn't make the situation better in the sense that it'd remove the listener function that MozLoopAPI registered to proxy events to content.

It's not really possible to make this work directly, because in that case we'd have to re-implement eventEmitter.(on|once|off) to keep a map of listener proxies that can be removed using `off` directly.

The result of this patch is basically that content scripts _never_ need to call `off`, really, as mozLoop will properly clean up things after the window was closed.

Notice that the browser console won't report errors anymore for update events, which is how you can validate this patch.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8587316 [details] [diff] [review]
Patch 2: now truly, really cleanup mozLoop event listeners when a window is closed

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

This looks reasonable, though I think we should change the commit message to be clearer about the fact that this isn't when the window is closed, but when the event is next called after the window was closed.

I'm partially thinking we should abandon the calls to "off" in content, but it might be that we need those later when we redo for e10s or something, so lets keep them for now.

Comment 11

[Mike de Boer [:mikedeboer]]

Pushed to fx-team with updated commit message: https://hg.mozilla.org/integration/fx-team/rev/005433fa2c0a

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/005433fa2c0a