Closed Bug 1114957 Opened 5 years ago Closed 5 years ago

Conversation window event listeners don't get cleaned up correctly

Categories

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

defect
Points:
1

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla36
Iteration:
37.3 - 12 Jan
Tracking Status
firefox40 --- fixed
Blocking Flags:
backlog Fx37+

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

Attachments

(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!

https://hg.mozilla.org/integration/fx-team/rev/9097517c27db
Flags: needinfo?(mdeboer)
https://hg.mozilla.org/mozilla-central/rev/9097517c27db
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Status: RESOLVED → REOPENED
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: https://hg.mozilla.org/integration/fx-team/rev/005433fa2c0a
https://hg.mozilla.org/mozilla-central/rev/005433fa2c0a
Status: REOPENED → RESOLVED
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.