Detached Element Memory Leak When Disposing Dynamic Textures

Bugs
1

I discovered this bug when running the Memlab tool over an app that adds and removes Dynamic Textures. It turns out the the _canvas element can sometimes cause memory to fail to be garbage collected. This snippet in dynamicTexture.ts appears to fix it.

image
image1342Ɨ614 92 KB


    /**
     * Disposes the dynamic texture.  The DOM elements should be removed before the texture is disposed
     */
    public dispose(): void {
        if(this._canvas) {
            this._canvas.remove();
            (this._canvas as any) = null;
        }
        if(this._context) {
            (this._context as any) = null;
        }
        super.dispose();
    }

And then in ICanvas, add:


     /**
     * Removes the canvas from the document.
     */
     remove(): void;

I am also to wrap the dispose function in my own code but it seems like the BabylonJS class is the better place for it. Apologies, but I canā€™t submit a pull request.

2

Iā€™m not sure why the canvas is not automatically garbage collected by the browser, if the dynamic texture is not referenced by any object anymore. Maybe the texture was still referenced somewhereā€¦

Anyway, it does no harm to add a dispose method to DynamicTexture, so I created a PR with your code (with a small change, because we should not do anything with the canvas if we didnā€™t create it in the first place):

1 Like
3

@Evgeni_Popov

This just pushed out to NPM and I was able to pull it down and confirm the memory leak is fixed.

image
image1124Ɨ990 104 KB

4

Ah, found a bug. It turns out that this line needs a null check because the sometimes after the texture is disposed, _canvas can be null.

    /**
     * Disposes the dynamic texture.
     */
    public dispose(): void {
        super.dispose();

        if (this._ownCanvas) {
            this._canvas?.remove();
        }
        (this._canvas as any) = null;
        (this._context as any) = null;
    }

Orā€¦

    /**
     * Disposes the dynamic texture.
     */
    public dispose(): void {
        super.dispose();

        if (this._ownCanvas && this._canvas) { \\ more readable?
            this._canvas.remove();
        }
        (this._canvas as any) = null;
        (this._context as any) = null;
    }

Or, just remove the canvas firstā€¦

    /**
     * Disposes the dynamic texture.
     */
    public dispose(): void {  
        if (this._ownCanvas && this._canvas) {
            this._canvas.remove();
        }
        (this._canvas as any) = null;
        (this._context as any) = null;
        super.dispose();
    }
5

One last clue here:

I cannot repro in this PG:

This makes me think thereā€™s just a race condition here so a null check is required in the case that the canvas is garbage collected correctly.

6

It may mean you are calling dispose several times for the same texture, but the change is small enough. Hereā€™s the PR:

1 Like
7

I looked through our code and couldnā€™t find anywhere weā€™re disposing multiple times, but I was able to do a simple PG that repros the issue that way so it sounds like a plausible explanation. Thank you for the update. I also pushed a PR but Iā€™ll close mine since you did the same thing already.

8

Ah sorry, I didnā€™t see you created a PR!

9

No problem. With your PR, I can confirm that it does not error if you dispose twice:

1 Like