Fix segmentation fault when pushing a class object based on Realm.Object to a list

[takameyer]

What, How & Why?

When creating a class that extends Realm.Object and pushing the instantiated object to a list, a segmentation fault would occur.

class TodoItem extends Realm.Object {
    constructor(description) {
        super()
        this.id = `${new ObjectId()}`
        this.description = description;
        this.done = false;
    }

    static schema = {
        name: "TodoItem",
        properties: {
            id: "string",
            description: "string",
            done: {type: "bool", default: false},
            deadline: "date?"
        },
        primaryKey: "id"
    }
}

class TodoList extends Realm.Object {

    constructor(name ) {
        super()
        this.id = `${new ObjectId()}`
        this.name = name;
        this.items = []
    }

    static schema = {
        name: "TodoList",
        properties: {
            id: "string",
            name: "string",
            items: "TodoItem[]"
        },
        primaryKey: "id"
    }
}

const realm = new Realm({schema: [TodoList, TodoItem]});
realm.write(() => {
    const list = realm.create(TodoList, new TodoList('My Todo List'))
    list.items.push(new TodoItem("Fix that bug"))

    TestCase.assertEqual(1, list.items.length)
})

The problem is that realm was assuming that if the object is of type Realm.Object that it has already been created. This assumption was causing a null pointer to be dereferenced which had caused the segmentation fault. The logic now checks for null and throws an exception.

Also attempting to create an object with an unmanged instance of Realm.Object will throw an exception. This is to avoid future attempts to create which fail, as the prototype has been changed.

☑️ ToDos

• 📝 Changelog entry
• 📝 Compatibility label is updated or copied from previous entry
• 🚦 Tests
• 📝 Public documentation PR created or is not necessary
• 💥 Breaking label has been applied or is not necessary

If this PR adds or changes public API's:

• typescript definitions file is updated
• jsdoc files updated
• Chrome debug API is updated if API is available on React Native

[kraenhansen]

Let's have a face 2 face conversation around this, as I don't think this is the right approach.
I agree that passing an instance of a class inheriting from Realm.Object shouldn't crash the app.
At the same time, I think it should throw a meaningful error instead of allowing object creation from this.

In my understanding of the current implementation (and limitations) of our "class-based schema" support, we do not support users to explicitly call the schema class's constructor, hence it's undefined behaviour and not something we should be providing work arounds for. I also agree that we should continue improving the class based support, ideally to the point where explicitly calling the class constructor is supported, but this requires more work than this PR suggests.

[fronck]

LGTM

[cesarvr]

LGTM

[takameyer]

@kraenhansen and I decided it would make more sense to throw an exception instead of implicitly creating the object on push. We also added an exception when someone tries to use the class constructor to create an object, as this breaks the second time.

In the future we will rethink class support.

[steffenagger]

It's definitely better to have consistent behaviour & exceptions instead of crashes, but should we word this as temporary fixes?

[fronck]

Just a few nitpicks and a general question.

Other than that, LGTM

[fronck]

[Question] The tests usually put their schemas in schemas.js.. Is there any specific reason why these don't?

[takameyer]

Well I would perhaps like to have a conversation about this. I find in tests it is best to not put such things in separate files, as they rarely are reused and it makes the reader of the test have a nicer time following what is happening. So I guess that's more my personal style and opinion.

[fronck]

I don't have a strong opinion on the subject myself..
@kneth ?

[kneth]

We are using the same schemas across tests, so we avoid code duplication with schemas.js.

[takameyer]

We are going to create a new issue for this where we will address refactor the tests and write a style guide.