Merge pull request #117 from realm/sk-listener-fixes

Make sure Realm.addListener uniques on callbacks and doesn't leak memory

lib/realm.js

@@ -42,7 +42,7 @@ class Realm {
         this[keys.id] = realmId;
         this[keys.realm] = realmId;
         this[keys.type] = objectTypes.REALM;
-        this[listenersKey] = [];
+        this[listenersKey] = new Set();
 
         [
             'path',
@@ -59,28 +59,24 @@ class Realm {
         if (name != 'change') { 
             throw new Error("Only 'change' notification is supported.");
         }
-        this[listenersKey].push(callback);
+        this[listenersKey].add(callback);
     }
 
     removeListener(name, callback) {
         if (typeof callback != 'function') {
-            throw new Error('Realm.addListener must be passed a function!');
+            throw new Error('Realm.removeListener must be passed a function!');
         }
         if (name != 'change') { 
             throw new Error("Only 'change' notification is supported.");
         }
-
-        let index = 0;
-        while ((index = this[listenersKey].indexOf(callback, index)) != -1) {
-            this[listenersKey].splice(index, 1);
-        }
+        this[listenersKey].delete(callback);
     }
 
     removeAllListeners(name) {
         if (name != undefined && name != 'change') {
             throw new Error("Only 'change' notification is supported.");
         }
-        this[listenersKey] = [];
+        this[listenersKey].clear();
     }
 
     write(callback) {

src/RJSRealm.mm

@@ -49,13 +49,17 @@ public:
     }
 
     void add_notification(JSObjectRef notification) {
+        if (!m_notifications.count(notification)) {
             JSValueProtect(m_context, notification);
             m_notifications.insert(notification);
         }
+    }
     void remove_notification(JSObjectRef notification) {
+        if (m_notifications.count(notification)) {
             JSValueUnprotect(m_context, notification);
             m_notifications.erase(notification);
         }
+    }
     void remove_all_notifications() {
         for (auto notification : m_notifications) {
             JSValueUnprotect(m_context, notification);

tests/RealmTests.js

@@ -289,8 +289,11 @@ module.exports = BaseTest.extend({
         var secondNotificationCount = 0;
         function secondNotification(realm, name) {
             secondNotificationCount++;
-        };
-        realm.addListener('change', secondNotification)
+        }
+
+        // The listener should only be added once.
+        realm.addListener('change', secondNotification);
+        realm.addListener('change', secondNotification);
 
         realm.write(function() {});
         TestCase.assertEqual(notificationCount, 2);