Closed Bug 1114957 Opened 5 years ago Closed 5 years ago

Conversation window event listeners don't get cleaned up correctly


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



(firefox40 fixed)

37.3 - 12 Jan
Tracking Status
firefox40 --- fixed
Blocking Flags:
backlog Fx37+


(Reporter: mikedeboer, Assigned: mikedeboer)




(2 files)

See summary.
Flags: qe-verify-
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to IT 37.3
Iteration: 37.2 → 37.3
Flags: needinfo?(mmucci)
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.
Attachment #8540675 - Flags: review?(nperriault) → review+
After further discussion with Mike and Mark, I think this is a Fx37+, P2.
backlog: Fx35? → Fx37+
Priority: P1 → P2
Duplicate of this bug: 1114956
Hi Mike -- Is this ready to land?
Flags: needinfo?(mdeboer)
Ah yes, thanks for being on top of things... I certainly am not!
Flags: needinfo?(mdeboer)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Resolution: FIXED → ---
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.
Attachment #8587316 - Flags: review?(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.
Attachment #8587316 - Flags: review?(standard8) → review+
Pushed to fx-team with updated commit message:
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla37 → mozilla36
You need to log in before you can comment on or make changes to this bug.