Merge pull request #426 from realm/sk-delegate-leak

Plug RealmDelegate leak and support changing constructors/defaults

lib/browser/index.js

@@ -19,11 +19,11 @@
 'use strict';
 
 import { NativeModules } from 'react-native';
-import { keys, propTypes, objectTypes } from './constants';
+import { keys, objectTypes } from './constants';
 import Collection, * as collections from './collections';
 import List, { createList } from './lists';
 import Results, { createResults } from './results';
-import RealmObject, { createObject, registerConstructors, typeForConstructor } from './objects';
+import RealmObject, * as objects from './objects';
 import * as rpc from './rpc';
 import * as util from './util';
 
@@ -31,7 +31,7 @@ const {debugHosts, debugPort} = NativeModules.Realm;
 
 rpc.registerTypeConverter(objectTypes.LIST, createList);
 rpc.registerTypeConverter(objectTypes.RESULTS, createResults);
-rpc.registerTypeConverter(objectTypes.OBJECT, createObject);
+rpc.registerTypeConverter(objectTypes.OBJECT, objects.createObject);
 rpc.registerTypeConverter(objectTypes.REALM, createRealm);
 
 function createRealm(_, info) {
@@ -59,7 +59,7 @@ function setupRealm(realm, realmId) {
 export default class Realm {
     constructor(config) {
         let schemas = typeof config == 'object' && config.schema;
-        let constructors = {};
+        let constructors = schemas ? {} : null;
 
         for (let i = 0, len = schemas ? schemas.length : 0; i < len; i++) {
             let item = schemas[i];
@@ -83,14 +83,15 @@ export default class Realm {
         }
 
         let realmId = rpc.createRealm(Array.from(arguments));
-
-        registerConstructors(realmId, constructors);
         setupRealm(this, realmId);
+
+        // This will create mappings between the id, path, and potential constructors.
+        objects.registerConstructors(realmId, this.path, constructors);
     }
 
     create(type, ...args) {
         if (typeof type == 'function') {
-            type = typeForConstructor(this[keys.realm], type);
+            type = objects.typeForConstructor(this[keys.realm], type);
         }
 
         let method = util.createMethod(objectTypes.REALM, 'create', true);
@@ -99,7 +100,7 @@ export default class Realm {
 
     objects(type, ...args) {
         if (typeof type == 'function') {
-            type = typeForConstructor(this[keys.realm], type);
+            type = objects.typeForConstructor(this[keys.realm], type);
         }
 
         let method = util.createMethod(objectTypes.REALM, 'objects');
@@ -147,6 +148,7 @@ Object.defineProperties(Realm, {
     clearTestState: {
         value: function() {
             collections.clearMutationListeners();
+            objects.clearRegisteredConstructors();
             rpc.clearTestState();
         },
     },

lib/browser/objects.js

@@ -21,7 +21,8 @@
 import { keys, objectTypes } from './constants';
 import { getterForProperty, setterForProperty, createMethods } from './util';
 
-const registeredConstructors = {};
+let registeredConstructors = {};
+let registeredRealmPaths = {};
 
 export default class RealmObject {
 }
@@ -31,9 +32,15 @@ createMethods(RealmObject.prototype, objectTypes.OBJECT, [
     'isValid',
 ]);
 
+export function clearRegisteredConstructors() {
+    registeredConstructors = {};
+    registeredRealmPaths = {};
+}
+
 export function createObject(realmId, info) {
     let schema = info.schema;
-    let constructor = (registeredConstructors[realmId] || {})[schema.name];
+    let realmPath = registeredRealmPaths[realmId];
+    let constructor = (registeredConstructors[realmPath] || {})[schema.name];
     let object = Object.create(constructor ? constructor.prototype : RealmObject.prototype);
 
     object[keys.realm] = realmId;
@@ -58,12 +65,17 @@ export function createObject(realmId, info) {
     return object;
 }
 
-export function registerConstructors(realmId, constructors) {
+export function registerConstructors(realmId, realmPath, constructors) {
-    registeredConstructors[realmId] = constructors;
+    registeredRealmPaths[realmId] = realmPath;
+
+    if (constructors) {
+        registeredConstructors[realmPath] = constructors;
+    }
 }
 
 export function typeForConstructor(realmId, constructor) {
-    let constructors = registeredConstructors[realmId];
+    let realmPath = registeredRealmPaths[realmId];
+    let constructors = registeredConstructors[realmPath];
 
     for (let name in constructors) {
         if (constructors[name] == constructor) {

src/js_realm.hpp

@@ -37,6 +37,9 @@
 namespace realm {
 namespace js {
 
+template<typename T>
+class Realm;
+
 template<typename T>
 struct RealmClass;
 
@@ -60,7 +63,7 @@ class RealmDelegate : public BindingContext {
     }
     virtual void will_change(std::vector<ObserverState> const& observers, std::vector<void*> const& invalidated) {}
 
-    RealmDelegate(std::weak_ptr<Realm> realm, GlobalContextType ctx) : m_context(ctx), m_realm(realm) {}
+    RealmDelegate(std::weak_ptr<realm::Realm> realm, GlobalContextType ctx) : m_context(ctx), m_realm(realm) {}
 
     ~RealmDelegate() {
         // All protected values need to be unprotected while the context is retained.
@@ -95,7 +98,7 @@ class RealmDelegate : public BindingContext {
   private:
     Protected<GlobalContextType> m_context;
     std::list<Protected<FunctionType>> m_notifications;
-    std::weak_ptr<Realm> m_realm;
+    std::weak_ptr<realm::Realm> m_realm;
     
     void notify(const char *notification_name) {
         SharedRealm realm = m_realm.lock();
@@ -112,6 +115,8 @@ class RealmDelegate : public BindingContext {
             Function<T>::call(m_context, callback, realm_object, 2, arguments);
         }
     }
+
+    friend class Realm<T>;
 };
 
 std::string default_path();
@@ -120,6 +125,7 @@ void delete_all_realms();
 
 template<typename T>
 class Realm {
+    using GlobalContextType = typename T::GlobalContext;
     using ContextType = typename T::Context;
     using FunctionType = typename T::Function;
     using ObjectType = typename T::Object;
@@ -251,6 +257,7 @@ void Realm<T>::constructor(ContextType ctx, ObjectType this_object, size_t argc,
     realm::Realm::Config config;
     typename Schema<T>::ObjectDefaultsMap defaults;
     typename Schema<T>::ConstructorMap constructors;
+    bool schema_updated = false;
 
     if (argc == 0) {
         config.path = default_path();
@@ -281,6 +288,7 @@ void Realm<T>::constructor(ContextType ctx, ObjectType this_object, size_t argc,
             if (!Value::is_undefined(ctx, schema_value)) {
                 ObjectType schema_object = Value::validated_to_object(ctx, schema_value, "schema");
                 config.schema.reset(new realm::Schema(Schema<T>::parse_schema(ctx, schema_object, defaults, constructors)));
+                schema_updated = true;
             }
 
             static const String schema_version_string = "schemaVersion";
@@ -305,7 +313,6 @@ void Realm<T>::constructor(ContextType ctx, ObjectType this_object, size_t argc,
                 };
             }
 
-
             static const String encryption_key_string = "encryptionKey";
             ValueType encryption_key_value = Object::get_property(ctx, object, encryption_key_string);
             if (!Value::is_undefined(ctx, encryption_key_value)) {
@@ -322,14 +329,23 @@ void Realm<T>::constructor(ContextType ctx, ObjectType this_object, size_t argc,
     ensure_directory_exists_for_file(config.path);
 
     SharedRealm realm = realm::Realm::get_shared_realm(config);
-    auto delegate = new RealmDelegate<T>(realm, Context<T>::get_global_context(ctx));
+    GlobalContextType global_context = Context<T>::get_global_context(ctx);
+    BindingContext *binding_context = realm->m_binding_context.get();
+    RealmDelegate<T> *js_binding_context = dynamic_cast<RealmDelegate<T> *>(binding_context);
 
-    if (!realm->m_binding_context) {
+    if (!binding_context) {
-        realm->m_binding_context.reset(delegate);
+        js_binding_context = new RealmDelegate<T>(realm, global_context);
+        realm->m_binding_context.reset(js_binding_context);
+    }
+    else if (!js_binding_context || js_binding_context->m_context != global_context) {
+        throw std::runtime_error("Realm is already open in another context on this thread: " + config.path);
     }
 
-    delegate->m_defaults = std::move(defaults);
+    // If a new schema was provided, then use its defaults and constructors.
-    delegate->m_constructors = std::move(constructors);
+    if (schema_updated) {
+        js_binding_context->m_defaults = std::move(defaults);
+        js_binding_context->m_constructors = std::move(constructors);
+    }
 
     set_internal<T, RealmClass<T>>(this_object, new SharedRealm(realm));
 }

tests/js/realm-tests.js

@@ -46,12 +46,10 @@ module.exports = BaseTest.extend({
         var defaultDir = Realm.defaultPath.substring(0, Realm.defaultPath.lastIndexOf("/") + 1)
         var testPath = 'test1.realm';
         var realm = new Realm({schema: [], path: testPath});
-        //TestCase.assertTrue(realm instanceof Realm);
         TestCase.assertEqual(realm.path, defaultDir + testPath);
 
         var testPath2 = 'test2.realm';
         var realm2 = new Realm({schema: [], path: testPath2});
-        //TestCase.assertTrue(realm2 instanceof Realm);
         TestCase.assertEqual(realm2.path, defaultDir + testPath2);
     },
 
@@ -68,10 +66,9 @@ module.exports = BaseTest.extend({
 
         var realm = new Realm({path: 'test1.realm', schema: [], schemaVersion: 1});
         TestCase.assertEqual(realm.schemaVersion, 1);
-        // FIXME - enable once Realm exposes a schema object
+        TestCase.assertEqual(realm.schema.length, 0);
-        //TestCase.assertEqual(realm.schema.length, 0);
- 
         realm.close();
+
         // FIXME - enable once realm initialization supports schema comparison
         // TestCase.assertThrows(function() {
         //     realm = new Realm({path: testPath, schema: [schemas.TestObject], schemaVersion: 1});
@@ -81,7 +78,9 @@ module.exports = BaseTest.extend({
         realm.write(function() {
             realm.create('TestObject', {doubleCol: 1});
         });
-        TestCase.assertEqual(realm.objects('TestObject')[0].doubleCol, 1)
+        TestCase.assertEqual(realm.objects('TestObject')[0].doubleCol, 1);
+        TestCase.assertEqual(realm.schemaVersion, 2);
+        TestCase.assertEqual(realm.schema.length, 1);
     },
 
     testRealmConstructorDynamicSchema: function() {
@@ -403,7 +402,7 @@ module.exports = BaseTest.extend({
     testRealmCreateWithDefaults: function() {
         var realm = new Realm({schema: [schemas.DefaultValues, schemas.TestObject]});
 
-        realm.write(function() {
+        var createAndTestObject = function() {
             var obj = realm.create('DefaultValuesObject', {});
             var properties = schemas.DefaultValues.properties;
 
@@ -418,7 +417,36 @@ module.exports = BaseTest.extend({
             TestCase.assertEqual(obj.nullObjectCol, null);
             TestCase.assertEqual(obj.arrayCol.length, properties.arrayCol.default.length);
             TestCase.assertEqual(obj.arrayCol[0].doubleCol, properties.arrayCol.default[0].doubleCol);
-        });
+        };
+
+        realm.write(createAndTestObject);
+
+        // Defaults should still work when creating another Realm instance.
+        realm = new Realm();
+        realm.write(createAndTestObject);
+    },
+
+    testRealmCreateWithChangingDefaults: function() {
+        var objectSchema = {
+            name: 'IntObject',
+            properties: {
+                intCol: {type: 'int', default: 1},
+            }
+        };
+
+        var realm = new Realm({schema: [objectSchema]});
+
+        var createAndTestObject = function() {
+            var object = realm.create('IntObject', {});
+            TestCase.assertEqual(object.intCol, objectSchema.properties.intCol.default);
+        };
+
+        realm.write(createAndTestObject);
+
+        objectSchema.properties.intCol.default++;
+
+        realm = new Realm({schema: [objectSchema]});
+        realm.write(createAndTestObject);
     },
 
     testRealmCreateWithConstructor: function() {
@@ -426,7 +454,6 @@ module.exports = BaseTest.extend({
 
         function CustomObject() {
             customCreated++;
-            this.intCol *= 100;
         }
         CustomObject.schema = {
             name: 'CustomObject',
@@ -447,7 +474,7 @@ module.exports = BaseTest.extend({
             properties: {
                 intCol: 'int'
             }
-        }
+        };
 
         var realm = new Realm({schema: [CustomObject, InvalidObject]});
 
@@ -457,15 +484,11 @@ module.exports = BaseTest.extend({
             TestCase.assertTrue(Object.getPrototypeOf(object) == CustomObject.prototype);
             TestCase.assertEqual(customCreated, 1);
 
-            // Should have been multiplied by 100 in the constructor.
-            TestCase.assertEqual(object.intCol, 100);
-
             // Should be able to create object by passing in constructor.
             object = realm.create(CustomObject, {intCol: 2});
             TestCase.assertTrue(object instanceof CustomObject);
             TestCase.assertTrue(Object.getPrototypeOf(object) == CustomObject.prototype);
             TestCase.assertEqual(customCreated, 2);
-            TestCase.assertEqual(object.intCol, 200);
         });
 
         TestCase.assertThrows(function() {
@@ -483,6 +506,36 @@ module.exports = BaseTest.extend({
                 realm.create(InvalidCustomObject, {intCol: 1});
             });
         });
+
+        // The constructor should still work when creating another Realm instance.
+        realm = new Realm();
+        TestCase.assertTrue(realm.objects('CustomObject')[0] instanceof CustomObject);
+        TestCase.assertTrue(realm.objects(CustomObject).length > 0);
+    },
+
+    testRealmCreateWithChangingConstructor: function() {
+        function CustomObject() {}
+        CustomObject.schema = {
+            name: 'CustomObject',
+            properties: {
+                intCol: 'int'
+            }
+        };
+
+        var realm = new Realm({schema: [CustomObject]});
+        realm.write(function() {
+            var object = realm.create('CustomObject', {intCol: 1});
+            TestCase.assertTrue(object instanceof CustomObject);
+        });
+
+        function NewCustomObject() {}
+        NewCustomObject.schema = CustomObject.schema;
+
+        realm = new Realm({schema: [NewCustomObject]});
+        realm.write(function() {
+            var object = realm.create('CustomObject', {intCol: 1});
+            TestCase.assertTrue(object instanceof NewCustomObject);
+        });
     },
 
     testRealmDelete: function() {