Repository: phoenixframework/phoenix (elixir)

Issue:
Memory leak in stateChangeCallbacks 
<!--
### Precheck

* Phoenix JS library v1.4.9
* For proposing and discussing new features, start a thread on the Phoenix Core mailing list
* For bugs, do a quick search and make sure the bug has not yet been reported
* Ensure that this issue is related to the Phoenix library and not one of the dependencies listed in mix.exs (Ecto, Plug, etc.)
* All checked? Be nice and have fun!
-->

### Environment

* Phoenix JS library v1.4.9

### Steps to reproduce

- Go to https://codesandbox.io/s/jolly-curie-furb6 
- Click Add Channel button few times and then remove channel
- Open devtools and check console logs

### Expected behavior

When removing a channel through `socket.remove(channel)`, socket should properly clean up any lifecycle events added by the removed channel.


### Actual behavior

```
channels count 1
Callbacks count
- open: 1
- close: 0
- error: 1
- message: 0

channels count 2
Callbacks count

- open: 2
- close: 0
- error: 2
- message: 0

channels count 1
Callbacks count

- open: 2
- close: 0
- error: 2
- message: 0

channels count 0
Callbacks count

- open: 2
- close: 0
- error: 2
- message: 0

```

As you can see the number of callbacks in open and close arrays doesn't decrease after channel is removed. This happens because in line [#L379](https://github.com/phoenixframework/phoenix/blob/master/assets/js/phoenix.js#L380) and [#L379](https://github.com/phoenixframework/phoenix/blob/master/assets/js/phoenix.js#L380) these 2 callbacks are added to the socket using anonymous function but they're not  cleaned up when removing a channel.

Pull request:
stateChangeCallbacks filter on channel remove

<img width="489" alt="Screen Shot 2019-09-13 at 10 32 48 PM" src="https://user-images.githubusercontent.com/7374640/64903910-72adf980-d676-11e9-8b93-4fbf61090aee.png">



close #3517 

Patch:
diff --git a/assets/js/phoenix.js b/assets/js/phoenix.js
index f1004e0c9..457eb8018 100644
--- a/assets/js/phoenix.js
+++ b/assets/js/phoenix.js
@@ -372,15 +372,17 @@ export class Channel {
     this.joinedOnce  = false
     this.joinPush    = new Push(this, CHANNEL_EVENTS.join, this.params, this.timeout)
     this.pushBuffer  = []
+    this.stateChangeRefs = [];
 
     this.rejoinTimer = new Timer(() => {
       if(this.socket.isConnected()){ this.rejoin() }
     }, this.socket.rejoinAfterMs)
-    this.socket.onError(() => this.rejoinTimer.reset())
-    this.socket.onOpen(() => {
-      this.rejoinTimer.reset()
-      if(this.isErrored()){ this.rejoin() }
-    })
+    this.stateChangeRefs.push(this.socket.onError(() => this.rejoinTimer.reset()))
+    this.stateChangeRefs.push(this.socket.onOpen(() => {
+        this.rejoinTimer.reset()
+        if(this.isErrored()){ this.rejoin() }
+      })
+    )
     this.joinPush.receive("ok", () => {
       this.state = CHANNEL_STATES.joined
       this.rejoinTimer.reset()
@@ -860,13 +862,21 @@ export class Socket {
    *
    * @param {Function} callback
    */
-  onOpen(callback){ this.stateChangeCallbacks.open.push(callback) }
+  onOpen(callback){
+    let ref = this.makeRef()
+    this.stateChangeCallbacks.open.push([ref, callback])
+    return ref
+  }
 
   /**
    * Registers callbacks for connection close events
    * @param {Function} callback
    */
-  onClose(callback){ this.stateChangeCallbacks.close.push(callback) }
+  onClose(callback){
+    let ref = this.makeRef()
+    this.stateChangeCallbacks.close.push([ref, callback])
+    return ref
+  }
 
   /**
    * Registers callbacks for connection error events
@@ -875,13 +885,21 @@ export class Socket {
    *
    * @param {Function} callback
    */
-  onError(callback){ this.stateChangeCallbacks.error.push(callback) }
+  onError(callback){
+    let ref = this.makeRef()
+    this.stateChangeCallbacks.error.push([ref, callback])
+    return ref
+  }
 
   /**
    * Registers callbacks for connection message events
    * @param {Function} callback
    */
-  onMessage(callback){ this.stateChangeCallbacks.message.push(callback) }
+  onMessage(callback){
+    let ref = this.makeRef()
+    this.stateChangeCallbacks.message.push([ref, callback])
+    return ref
+  }
 
   /**
    * @private
@@ -893,7 +911,7 @@ export class Socket {
     this.flushSendBuffer()
     this.reconnectTimer.reset()
     this.resetHeartbeat()
-    this.stateChangeCallbacks.open.forEach( callback => callback() )
+    this.stateChangeCallbacks.open.forEach(([, callback]) => callback() )
   }
 
   /**
@@ -922,7 +940,7 @@ export class Socket {
     if(!this.closeWasClean){
       this.reconnectTimer.scheduleTimeout()
     }
-    this.stateChangeCallbacks.close.forEach( callback => callback(event) )
+    this.stateChangeCallbacks.close.forEach(([, callback]) => callback(event) )
   }
 
   /**
@@ -931,7 +949,7 @@ export class Socket {
   onConnError(error){
     if (this.hasLogger()) this.log("transport", error)
     this.triggerChanError()
-    this.stateChangeCallbacks.error.forEach( callback => callback(error) )
+    this.stateChangeCallbacks.error.forEach(([, callback]) => callback(error) )
   }
 
   /**
@@ -966,9 +984,22 @@ export class Socket {
    * @param {Channel}
    */
   remove(channel){
+    this.off(channel)
     this.channels = this.channels.filter(c => c.joinRef() !== channel.joinRef())
   }
 
+  /**
+   * stateChangeCallbacks is an array of "tuples" [[ref, callback], [...]]
+   * and can be cleaned up when a channel goes offline
+   * @param {Channel}
+   */
+  off(channel) {
+    let refs = channel.stateChangeRefs
+    this.stateChangeCallbacks.open = this.stateChangeCallbacks.open.filter(([ref]) => {
+      return !refs.includes(ref)
+    })
+  }
+
   /**
    * Initiates a new channel for the given topic
    *
@@ -1046,7 +1077,8 @@ export class Socket {
       }
 
       for (let i = 0; i < this.stateChangeCallbacks.message.length; i++) {
-        this.stateChangeCallbacks.message[i](msg)
+        let [, callback] = this.stateChangeCallbacks.message[i]
+        callback(msg)
       }
     })
   }
@@ -1190,7 +1222,6 @@ export class Ajax {
     try {
       return JSON.parse(resp)
     } catch(e) {
-      console && console.log("failed to parse JSON response", resp)
       return null
     }
   }


Test patch:
diff --git a/assets/test/channel_test.js b/assets/test/channel_test.js
index 959e6f57f..1d38f2fd3 100644
--- a/assets/test/channel_test.js
+++ b/assets/test/channel_test.js
@@ -377,12 +377,12 @@ describe("with transport", () => {
       })
 
       it("removes channel bindings", () => {
-        let bindings = helpers.getBindings("chan_reply_1")
+        let bindings = helpers.getBindings("chan_reply_3")
         assert.equal(bindings.length, 1)
 
         helpers.receiveOk()
 
-        bindings = helpers.getBindings("chan_reply_1")
+        bindings = helpers.getBindings("chan_reply_3")
         assert.equal(bindings.length, 0)
       })
 
diff --git a/assets/test/socket_test.js b/assets/test/socket_test.js
index 9d587d723..042379d36 100644
--- a/assets/test/socket_test.js
+++ b/assets/test/socket_test.js
@@ -390,8 +390,12 @@ describe("with transports", done =>{
       sinon.stub(channel1, "joinRef").returns(1)
       sinon.stub(channel2, "joinRef").returns(2)
 
+      assert.equal(socket.stateChangeCallbacks.open.length, 2)
+
       socket.remove(channel1)
 
+      assert.equal(socket.stateChangeCallbacks.open.length, 1)
+
       assert.equal(socket.channels.length, 1)
 
       const [foundChannel] = socket.channels