From d78006e10611c69fe0e790c77e06240bb25c8a01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20N=C3=A9grier?= Date: Tue, 23 Jun 2020 14:56:57 +0200 Subject: [PATCH] Fixing memory leak with listeners The listeners from MediaManager and SimplePeer were never removed, leading to a huge amount of listeners all over the applications when switching regularly of scene. --- front/src/Phaser/Game/GameScene.ts | 2 ++ front/src/WebRtc/MediaManager.ts | 36 ++++++++++++++++------ front/src/WebRtc/SimplePeer.ts | 48 +++++++++++++++++------------- 3 files changed, 56 insertions(+), 30 deletions(-) diff --git a/front/src/Phaser/Game/GameScene.ts b/front/src/Phaser/Game/GameScene.ts index 8763f913..44c4feb6 100644 --- a/front/src/Phaser/Game/GameScene.ts +++ b/front/src/Phaser/Game/GameScene.ts @@ -190,6 +190,7 @@ export class GameScene extends Phaser.Scene { console.log('Player disconnected from server. Reloading scene.'); this.simplePeer.closeAllConnections(); + this.simplePeer.unregister(); const key = 'somekey'+Math.round(Math.random()*10000); const game : Phaser.Scene = GameScene.createFromUrl(this.MapUrlFile, this.instance, key); @@ -610,6 +611,7 @@ export class GameScene extends Phaser.Scene { if(nextSceneKey){ // We are completely destroying the current scene to avoid using a half-backed instance when coming back to the same map. this.connection.closeConnection(); + this.simplePeer.unregister(); this.scene.stop(); this.scene.remove(this.scene.key); this.scene.start(nextSceneKey.key, { diff --git a/front/src/WebRtc/MediaManager.ts b/front/src/WebRtc/MediaManager.ts index 03736e6e..8be141ec 100644 --- a/front/src/WebRtc/MediaManager.ts +++ b/front/src/WebRtc/MediaManager.ts @@ -3,7 +3,10 @@ const videoConstraint: boolean|MediaTrackConstraints = { height: { ideal: 720 }, facingMode: "user" }; -export class MediaManager { + +type UpdatedLocalStreamCallback = (media: MediaStream) => void; + +class MediaManager { localStream: MediaStream|null = null; private remoteVideo: Map = new Map(); myCamVideo: HTMLVideoElement; @@ -16,11 +19,9 @@ export class MediaManager { audio: true, video: videoConstraint }; - updatedLocalStreamCallBack : (media: MediaStream) => void; - - constructor(updatedLocalStreamCallBack : (media: MediaStream) => void) { - this.updatedLocalStreamCallBack = updatedLocalStreamCallBack; + updatedLocalStreamCallBacks : Set = new Set(); + constructor() { this.myCamVideo = this.getElementByIdOrFail('myCamVideo'); this.webrtcInAudio = this.getElementByIdOrFail('audio-webrtc-in'); this.webrtcInAudio.volume = 0.2; @@ -54,6 +55,21 @@ export class MediaManager { }); } + onUpdateLocalStream(callback: UpdatedLocalStreamCallback): void { + + this.updatedLocalStreamCallBacks.add(callback); + } + + removeUpdateLocalStreamEventListener(callback: UpdatedLocalStreamCallback): void { + this.updatedLocalStreamCallBacks.delete(callback); + } + + private triggerUpdatedLocalStreamCallbacks(stream: MediaStream): void { + for (const callback of this.updatedLocalStreamCallBacks) { + callback(stream); + } + } + activeVisio(){ const webRtc = this.getElementByIdOrFail('webRtc'); webRtc.classList.add('active'); @@ -64,7 +80,7 @@ export class MediaManager { this.cinema.style.display = "block"; this.constraintsMedia.video = videoConstraint; this.getCamera().then((stream: MediaStream) => { - this.updatedLocalStreamCallBack(stream); + this.triggerUpdatedLocalStreamCallbacks(stream); }); } @@ -79,7 +95,7 @@ export class MediaManager { }); } this.getCamera().then((stream) => { - this.updatedLocalStreamCallBack(stream); + this.triggerUpdatedLocalStreamCallbacks(stream); }); } @@ -88,7 +104,7 @@ export class MediaManager { this.microphone.style.display = "block"; this.constraintsMedia.audio = true; this.getCamera().then((stream) => { - this.updatedLocalStreamCallBack(stream); + this.triggerUpdatedLocalStreamCallbacks(stream); }); } @@ -102,7 +118,7 @@ export class MediaManager { }); } this.getCamera().then((stream) => { - this.updatedLocalStreamCallBack(stream); + this.triggerUpdatedLocalStreamCallbacks(stream); }); } @@ -308,3 +324,5 @@ export class MediaManager { } } + +export const mediaManager = new MediaManager(); diff --git a/front/src/WebRtc/SimplePeer.ts b/front/src/WebRtc/SimplePeer.ts index 381b3ac2..553c9307 100644 --- a/front/src/WebRtc/SimplePeer.ts +++ b/front/src/WebRtc/SimplePeer.ts @@ -4,7 +4,7 @@ import { WebRtcSignalMessageInterface, WebRtcStartMessageInterface } from "../Connection"; -import {MediaManager} from "./MediaManager"; +import { mediaManager } from "./MediaManager"; import * as SimplePeerNamespace from "simple-peer"; const Peer: SimplePeerNamespace.SimplePeer = require('simple-peer'); @@ -22,16 +22,15 @@ export class SimplePeer { private WebRtcRoomId: string; private Users: Array = new Array(); - private MediaManager: MediaManager; - private PeerConnectionArray: Map = new Map(); + private readonly updateLocalStreamCallback: (media: MediaStream) => void; constructor(Connection: Connection, WebRtcRoomId: string = "test-webrtc") { this.Connection = Connection; this.WebRtcRoomId = WebRtcRoomId; - this.MediaManager = new MediaManager((stream : MediaStream) => { - this.updatedLocalStream(); - }); + // We need to go through this weird bound function pointer in order to be able to "free" this reference later. + this.updateLocalStreamCallback = this.updatedLocalStream.bind(this); + mediaManager.onUpdateLocalStream(this.updateLocalStreamCallback); this.initialise(); } @@ -45,8 +44,8 @@ export class SimplePeer { this.receiveWebrtcSignal(message); }); - this.MediaManager.activeVisio(); - this.MediaManager.getCamera().then(() => { + mediaManager.activeVisio(); + mediaManager.getCamera().then(() => { //receive message start this.Connection.receiveWebrtcStart((message: WebRtcStartMessageInterface) => { @@ -105,8 +104,8 @@ export class SimplePeer { name = userSearch.name; } } - this.MediaManager.removeActiveVideo(user.userId); - this.MediaManager.addActiveVideo(user.userId, name); + mediaManager.removeActiveVideo(user.userId); + mediaManager.addActiveVideo(user.userId, name); const peer : SimplePeerNamespace.Instance = new Peer({ initiator: user.initiator ? user.initiator : false, @@ -143,15 +142,15 @@ export class SimplePeer { } }); if(microphoneActive){ - this.MediaManager.enabledMicrophoneByUserId(user.userId); + mediaManager.enabledMicrophoneByUserId(user.userId); }else{ - this.MediaManager.disabledMicrophoneByUserId(user.userId); + mediaManager.disabledMicrophoneByUserId(user.userId); } if(videoActive){ - this.MediaManager.enabledVideoByUserId(user.userId); + mediaManager.enabledVideoByUserId(user.userId); }else{ - this.MediaManager.disabledVideoByUserId(user.userId); + mediaManager.disabledVideoByUserId(user.userId); } this.stream(user.userId, stream); }); @@ -167,11 +166,11 @@ export class SimplePeer { // eslint-disable-next-line @typescript-eslint/no-explicit-any peer.on('error', (err: any) => { console.error(`error => ${user.userId} => ${err.code}`, err); - this.MediaManager.isError(user.userId); + mediaManager.isError(user.userId); }); peer.on('connect', () => { - this.MediaManager.isConnected(user.userId); + mediaManager.isConnected(user.userId); console.info(`connect => ${user.userId}`); }); @@ -192,7 +191,7 @@ export class SimplePeer { */ private closeConnection(userId : string) { try { - this.MediaManager.removeActiveVideo(userId); + mediaManager.removeActiveVideo(userId); const peer = this.PeerConnectionArray.get(userId); if (peer === undefined) { console.warn("Tried to close connection for user "+userId+" but could not find user") @@ -215,6 +214,13 @@ export class SimplePeer { } } + /** + * Unregisters any held event handler. + */ + public unregister() { + mediaManager.removeUpdateLocalStreamEventListener(this.updateLocalStreamCallback); + } + /** * * @param userId @@ -253,11 +259,11 @@ export class SimplePeer { */ private stream(userId : string, stream: MediaStream) { if(!stream){ - this.MediaManager.disabledVideoByUserId(userId); - this.MediaManager.disabledMicrophoneByUserId(userId); + mediaManager.disabledVideoByUserId(userId); + mediaManager.disabledMicrophoneByUserId(userId); return; } - this.MediaManager.addStreamRemoteVideo(userId, stream); + mediaManager.addStreamRemoteVideo(userId, stream); } /** @@ -266,7 +272,7 @@ export class SimplePeer { */ private addMedia (userId : string) { try { - const localStream: MediaStream|null = this.MediaManager.localStream; + const localStream: MediaStream|null = mediaManager.localStream; const peer = this.PeerConnectionArray.get(userId); if(localStream === null) { //send fake signal