Engineering at MindLink

Mixin that memory leak

July 05, 2018

This is a story of how, without much effort, mixins can give you memory leaks all over your shiny stuff.

TL;DR

Mixins share the properties with their target
Conflicts between property names with different semantics can lead to memory leaks or undefined behaviour
Ideally don’t use mixins
If you have to, either:
1. Make sure the mixin is using unique property names, or symbols as properties for ‘private’ members
2. Make it clear when a property is accessible to a target and when it isn’t

The Story

Performance is important to us, we heavily optimize our clients to get the most out of the browser for fast rendering, we also regularly perform memory profiling to ensure we don’t leak all over the place.

After introducing a new implementation of our rich text input (moving away from DraftJS on mobile devices because it doesn’t work properly) I did a quick memory profiling session over it and found just what I hoped I wouldn’t - a leak all over our nice new input.

The problem is that leak wasn’t in the new code, it was old… We missed it. We need to get better at making sure we don’t miss it in future, that’s a journey we’re taking.

The memory snapshot showing the leak looks like this:

That event handler "interactivitychanged" on the InteractivityMonitor shouldn’t be there, it should have been removed as part of the destruction of the view model.

My first instinct was that this must be because we’re just not destroying the view model at all. A quick debug later, nope that’s not it.

Next on the list is that we may be destroying it, but is that actually cleaning up the event handlers? Yes, yes it is…

/**
 * Destroys the interactable mixin.
 */
public destroyInteractable(): void {
    this.interactivityMonitor.un("interactivitychanged", this.handleInteractivityMonitorChanged, this); // <-- Yupp we're removing the listener.
    this.interactivityMonitor.destroy();
}

So what’s going on? The debug session showed that the InteractivityMonitor that we’re cleaning up has no registered handlers, but the memory profiler is definitive in its report that there is one there…

That means we’re looking at different instances of the same type and so the answer lies in who we add the listener to and remove the listener from.

Let’s start from the leaky view model:

/**
 * Represents an interface for the constructor configuration.
 */
export interface IComposeMessageInputViewModelConfig {
    /**
     * The interactivity monitor.
     */
    interactivityMonitor: FcfJS.viewmodel.AbstractInteractivityMonitor;
    
    /** -snip- **/
}

/**
 * Represents the view model for a compose message input view.
 */
class ComposeMessageInputViewModel extends FcfJS.viewmodel.MessageInputViewModel {
    /**
     * Initializes the interactable mixin.
     *
     * @param config The configuration object.
     */
    private initializeInteractable: (config: IInteractableConfig) => void;

    /**
     * Destroys the interactable mixin.
     */
    private destroyInteractable: () => void;

    /** -snip- **/

    /**
     * The interactivity monitor.
     */
    private interactivityMonitor: FcfJS.viewmodel.AbstractInteractivityMonitor;

    /**
     * Initializes a new instance of this class.
     *
     * @param config The initial configuration.
     */
    constructor(config: IComposeMessageInputViewModelConfig) {
        super();

        /** -snip- **/

        this.initializeInteractable({
            interactivityMonitor: config.interactivityMonitor
        });
    }

    /**
     * Destroys this instance.
     */
    public destroy(): void {
        /** -snip- **/

        this.interactivityMonitor.getInteractivityChangedEvent().un(this);
        
        super.destroy();
        this.destroyInteractable();
    }

    /** -snip- **/

    /**
     * Updates the interactivity monitor following a change in active conversation.
     *
     * @param interactivityMonitor The interactivity monitor for the conversation.
     */
    private updateInteractivityMonitor(interactivityMonitor: AbstractInteractivityMonitor): void {
        if (this.interactivityMonitor) {
            this.interactivityMonitor.getInteractivityChangedEvent().un(this);
            this.interactivityMonitor.destroy();
        }

        this.interactivityMonitor = interactivityMonitor;
        this.interactivityMonitor.getInteractivityChangedEvent().on(this.handleMessageSendabilityChanged, this);
    }
}

mixin(ComposeMessageInputViewModel, Interactable);

export default ComposeMessageInputViewModel;

We see that he mixes in an Interactable. The view model himself adds and removes a listener symmetrically, so that’s fine. He does have an interactivity monitor and through his lifetime changes it based on the conversation it represents, but that’s used for something else, not listening on the leaky event.

Enter the Interactable mixin.

/**
 * Represents an interface for the constructor configuration.
 */
export interface IInteractableConfig {
    /**
     * The interactivity monitor.
     */
    interactivityMonitor: AbstractInteractivityMonitor;
}

/**
 * Represents a simple mixin that determines whether the view model is interactable.
 */
export default class Interactable {
    /**
     * The interactivity monitor.
     */
    private interactivityMonitor: AbstractInteractivityMonitor = null;

    /**
     * Sets whether the property is interactable or not.
     *
     * @param name The property name.
     * @param value Whether the property is interactable or not.
     */
    private setProperty: (name: any, value: any) => void;

    /**
     * Gets whether the property is interactable or not.
     *
     * @param name The property name.
     * @returns `true` if the property is interactable, `false` otherwise.
     */
    private getProperty: (name: any) => any;

    /**
     * Add properties.
     *
     * @param name The property name.
     */
    private addProperties: (name: any) => void;

    /**
     * Initializes the interactable mixin.
     *
     * @param config The initial configuration.
     */
    public initializeInteractable(config: IInteractableConfig): void {
        this.addProperties({
            isInteractable: false
        });

        this.interactivityMonitor = config.interactivityMonitor;

        this.interactivityMonitor.on("interactivitychanged", this.handleInteractivityMonitorChanged, this);

        this.setIsInteractable(this.interactivityMonitor.isInteractive());
    }

    /**
     * Destroys the interactable mixin.
     */
    public destroyInteractable(): void {
        this.interactivityMonitor.un("interactivitychanged", this.handleInteractivityMonitorChanged, this);
        this.interactivityMonitor.destroy();
    }

    /**
     * Handles when the interactable state changes.
     *
     * @param _ The new interactable state.
     */
    protected handleInteractableChanged(_: boolean): void {
        return;
    }

    /**
     * Handles when the interactable state of the monitor changes.
     *
     * @param _ The interactivity monitor.
     * @param isInteractive The new interactable state.
     */
    protected handleInteractivityMonitorChanged(_: AbstractInteractivityMonitor, isInteractive: boolean): void {
        this.setIsInteractable(isInteractive);
    }

    /**
     * Gets a value indicating whether the view model is interactable.
     *
     * @returns `true` if the view model is interactable, `false` otherwise.
     */
    public getIsInteractable(): boolean {
        return this.getProperty("isInteractable");
    }

    /**
     * Sets whether the view model is interactable.
     *
     * @param isInteractable Whether the view model is interactable.
     */
    private setIsInteractable(isInteractable: boolean): void {
        if (this.getIsInteractable() === isInteractable) {
            return;
        }

        this.setProperty("isInteractable", isInteractable);
        this.handleInteractableChanged(isInteractable);
    }
}

Looking at him he seems all fine and dandy - initializing adds the listener, destroying removes it. Great, no problem here…

…

Hold on… What’s that right there? Can you see it? The damn interactivity monitor property has the same name in the mixin as in the view model! That’s a problem.

When we mix something in what we’re actually doing is copying implementation from the mixin to the target. There’s only a single instance with both the mixin and target properties, that means that things with the same name in both types refer to the same thing.

What’s happening here is that the Interactable mixin is adding the event listener to the initial instance of the interactivity monitor, then during its lifetime the view model is assigning to itself a new interactivity monitor. By the time the Interactable is asked to clean up, the shared property is referring to a different instance and the event listener is still on the old instance that we no longer have a reference to! LEAK! We have found you and now we have to patch you up!

The proper way to do this is to stop using mixins, they really aren’t great for a number of reasons - this is just one of them. However, that’s quite a big change that we will schedule for another time. For now we can avoid this issue by renaming the mixin property so that it’s unique (though there’s no guarantee unless we use symbols for properties).

With the property renamed the leak is gone :)

Written by Luke Terry.

Senior Engineer at MindLink. Enjoys technology, playing games and making things work, blogs at www.indescrible.co.uk.