fix(): dispose-render concurrency

src/canvas.class.ts

@@ -1312,17 +1312,24 @@ import { Point } from './point.class';
     },
 
     /**
-     * Clears a canvas element and removes all event listeners
-     * @return {fabric.Canvas} thisArg
-     * @chainable
+     * Clears the canvas element, disposes objects, removes all event listeners and frees resources
+     * 
+     * **CAUTION**:
+     * 
+     * This method is **UNSAFE**.
+     * You may encounter a race condition using it if there's a requested render.
+     * Call this method only if you are sure rendering has settled. 
+     * Consider using {@link dispose} as it is **SAFE** 
+     * 
+     * @private
      */
-    dispose: function () {
+    destroy: function () {
       var wrapperEl = this.wrapperEl,
           lowerCanvasEl = this.lowerCanvasEl,
           upperCanvasEl = this.upperCanvasEl,
           cacheCanvasEl = this.cacheCanvasEl;
       this.removeListeners();
-      this.callSuper('dispose');
+      this.callSuper('destroy');
       wrapperEl.removeChild(upperCanvasEl);
       wrapperEl.removeChild(lowerCanvasEl);
       this.contextCache = null;

src/shapes/object.class.ts

@@ -1738,7 +1738,8 @@ import { pick } from '../util/misc/pick';
       // since this canvas is a simple element for the process, we remove references
       // to objects in this way in order to avoid object trashing.
       canvas._objects = [];
-      canvas.dispose();
+      // since render has settled it is safe to destroy canvas
+      canvas.destroy();
       canvas = null;
 
       return canvasEl;

src/static_canvas.class.ts

@@ -691,7 +691,7 @@ import { pick } from './util/misc/pick';
      * @chainable
      */
     renderAndReset: function() {
-      this.isRendering = 0;
+      this.nextRenderHandle = 0;
       this.renderAll();
     },
 
@@ -703,8 +703,8 @@ import { pick } from './util/misc/pick';
      * @chainable
      */
     requestRenderAll: function () {
-      if (!this.isRendering) {
-        this.isRendering = fabric.util.requestAnimFrame(this.renderAndResetBound);
+      if (!this.nextRenderHandle && !this.destroyed) {
+        this.nextRenderHandle = fabric.util.requestAnimFrame(this.renderAndResetBound);
       }
       return this;
     },
@@ -734,9 +734,9 @@ import { pick } from './util/misc/pick';
     },
 
     cancelRequestedRender: function() {
-      if (this.isRendering) {
-        fabric.util.cancelAnimFrame(this.isRendering);
-        this.isRendering = 0;
+      if (this.nextRenderHandle) {
+        fabric.util.cancelAnimFrame(this.nextRenderHandle);
+        this.nextRenderHandle = 0;
       }
     },
 
@@ -747,12 +747,17 @@ import { pick } from './util/misc/pick';
      * @return {fabric.Canvas} instance
      * @chainable
      */
-    renderCanvas: function(ctx, objects) {
+    renderCanvas: function (ctx, objects) {
+
+      if (this.destroyed) {
+        return;
+      }
+
       var v = this.viewportTransform, path = this.clipPath;
-      this.cancelRequestedRender();
       this.calcViewportBoundaries();
       this.clearContext(ctx);
       fabric.util.setImageSmoothing(ctx, this.imageSmoothingEnabled);
+      // node-canvas
       ctx.patternQuality = 'best';
       this.fire('before:render', { ctx: ctx, });
       this._renderBackground(ctx);
@@ -778,6 +783,10 @@ import { pick } from './util/misc/pick';
         this.drawControls(ctx);
       }
       this.fire('after:render', { ctx: ctx, });
+      if (this.__cleanupTask) {
+        this.__cleanupTask();
+        this.__cleanupTask = undefined;
+      }
     },
 
     /**
@@ -1609,16 +1618,43 @@ import { pick } from './util/misc/pick';
     },
 
     /**
-     * Clears a canvas element and dispose objects
-     * @return {fabric.Canvas} thisArg
-     * @chainable
+     * Waits until rendering has settled to destroy the canvas
+     * @returns {Promise} a promise resolving once the canvas has been destroyed
      */
     dispose: function () {
-      // cancel eventually ongoing renders
-      if (this.isRendering) {
-        fabric.util.cancelAnimFrame(this.isRendering);
-        this.isRendering = 0;
-      }
+      return new Promise<void>((resolve, reject) => {
+        const task = () => {
+          this.destroy();
+          resolve();
+        }
+        task.kill = reject;
+        if (this.__cleanupTask) {
+          this.__cleanupTask.kill();
+        }
+        if (this.nextRenderHandle) {
+          this.__cleanupTask = task;
+        }
+        else {
+          task();
+        }
+      });
+    },
+
+    /**
+     * Clears the canvas element, disposes objects and frees resources
+     * 
+     * **CAUTION**:
+     * 
+     * This method is **UNSAFE**.
+     * You may encounter a race condition using it if there's a requested render.
+     * Call this method only if you are sure rendering has settled. 
+     * Consider using {@link dispose} as it is **SAFE** 
+     * 
+     * @private
+     */
+    destroy: function () {
+      this.destroyed = true;
+      this.cancelRequestedRender();
       this.forEachObject(function(object) {
         object.dispose && object.dispose();
       });
@@ -1645,7 +1681,6 @@ import { pick } from './util/misc/pick';
       this.lowerCanvasEl.setAttribute('height', this.height);
       fabric.util.cleanUpJsdomNode(this.lowerCanvasEl);
       this.lowerCanvasEl = undefined;
-      return this;
     },
 
     /**

test/lib/visualTestLoop.js

@@ -156,7 +156,7 @@
           canvas.height = height;
           var ctx = canvas.getContext('2d');
           var output = ctx.getImageData(0, 0, width, height);
-          getImage(getGoldeName(golden), renderedCanvas, function(goldenImage) {
+          getImage(getGoldeName(golden), renderedCanvas, async (goldenImage) => {
             ctx.drawImage(goldenImage, 0, 0);
             visualCallback.addArguments({
               enabled: true,
@@ -181,7 +181,7 @@
             if ((!isOK && QUnit.debugVisual) || QUnit.recreateVisualRefs) {
               generateGolden(getGoldeName(golden), renderedCanvas);
             }
-            fabricCanvas.dispose();
+            await fabricCanvas.dispose();
             done();
           });
         });

test/unit/canvas.js

@@ -2083,7 +2083,7 @@
     assert.equal(aGroup._objects[3], circle2);
   });
 
-  QUnit.test('dispose', function(assert) {
+  QUnit.test('dispose', async function(assert) {
     //made local vars to do not dispose the external canvas
     var el = fabric.document.createElement('canvas'),
         parentEl = fabric.document.createElement('div'),
@@ -2123,10 +2123,11 @@
     assert.equal(parentEl.childNodes.length, 1, 'parent div should have 1 child');
     assert.notEqual(parentEl.firstChild, canvas.getElement(), 'canvas should not be parent div firstChild');
     assert.ok(typeof canvas.dispose === 'function');
+    assert.ok(typeof canvas.destroy === 'function');
     canvas.add(makeRect(), makeRect(), makeRect());
     canvas.item(0).animate('scaleX', 10);
     assert.equal(fabric.runningAnimations.length, 1, 'should have a running animation');
-    canvas.dispose();
+    await canvas.dispose();
     canvas.cancelRequestedRender();
     assert.equal(fabric.runningAnimations.length, 0, 'dispose should clear running animations');
     assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas');
@@ -2147,7 +2148,7 @@
 
   });
 
-  QUnit.test('dispose + set dimensions', function (assert) {
+  QUnit.test('dispose + set dimensions', async function (assert) {
     //made local vars to do not dispose the external canvas
     var el = fabric.document.createElement('canvas'),
       parentEl = fabric.document.createElement('div');
@@ -2170,15 +2171,15 @@
     assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal');
     assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed');
 
-    canvas.dispose();
+    await canvas.dispose();
     assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style');
     assert.equal(el.style.cssText, elStyle, 'restored original canvas style');
     assert.equal(el.width, 500, 'restored width');
     assert.equal(el.height, 500, 'restored height');
 
   });
 
-  // QUnit.test('dispose', function(assert) {
+  // QUnit.test('dispose', async function(assert) {
   //   function invokeEventsOnCanvas() {
   //     // nextSibling because we need to invoke events on upper canvas
   //     simulateEvent(canvas.getElement().nextSibling, 'mousedown');
@@ -2213,7 +2214,7 @@
   //   invokeEventsOnCanvas();
   //   assertInvocationsCount();
 
-  //   canvas.dispose();
+  //   await canvas.dispose();
   //   assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas');
 
   //   invokeEventsOnCanvas();

test/unit/canvas_events.js

@@ -562,8 +562,8 @@
     assert.deepEqual(fired, eventNames, 'bad drop event fired');
   });
 
-  QUnit.test('drag event cycle', function (assert) {
-    function testDragCycle(cycle, canDrop) {
+  QUnit.test('drag event cycle', async function (assert) {
+    async function testDragCycle(cycle, canDrop) {
       var c = new fabric.Canvas();
       var rect = new fabric.Rect({ width: 10, height: 10 });
       rect.canDrop = function () {
@@ -585,23 +585,23 @@
         event.clientY = 5;
         c.upperCanvasEl.dispatchEvent(event);
       });
-      c.dispose();
+      await c.dispose();
       assert.equal(canvasRegistry.length, cycle.length, 'should fire cycle on canvas');
       assert.deepEqual(canvasRegistry, cycle, 'should fire all events on canvas');
       return registery
     }
     var cycle, res;
     cycle = ['dragenter', 'dragover', 'dragover', 'dragover', 'drop'];
-    res = testDragCycle(cycle, true);
+    res = await testDragCycle(cycle, true);
     assert.deepEqual(res, cycle, 'should fire all events on rect');
     cycle = ['dragenter', 'dragover', 'dragover', 'dragover', 'dragleave'];
-    res = testDragCycle(cycle, true);
+    res = await testDragCycle(cycle, true);
     assert.deepEqual(res, cycle, 'should fire all events on rect');
     cycle = ['dragenter', 'dragover', 'dragover', 'dragover', 'drop'];
-    res = testDragCycle(cycle);
+    res = await testDragCycle(cycle);
     assert.deepEqual(res, cycle, 'should fire all events on rect');
     cycle = ['dragenter', 'dragover', 'dragover', 'dragover', 'dragleave'];
-    res = testDragCycle(cycle);
+    res = await testDragCycle(cycle);
     assert.deepEqual(res, cycle, 'should fire all events on rect');
   });
 

test/unit/canvas_static.js

@@ -1610,13 +1610,14 @@
     assert.equal(canvas.toString(), '#<fabric.Canvas (1): { objects: 1 }>');
   });
 
-  QUnit.test('dispose clear references', function(assert) {
+  QUnit.test('dispose clear references', async function(assert) {
     var canvas2 = new fabric.StaticCanvas(null, { renderOnAddRemove: false });
     assert.ok(typeof canvas2.dispose === 'function');
+    assert.ok(typeof canvas2.destroy === 'function');
     canvas2.add(makeRect(), makeRect(), makeRect());
     var lowerCanvas = canvas2.lowerCanvasEl;
     assert.equal(lowerCanvas.getAttribute('data-fabric'), 'main', 'lowerCanvasEl should be marked by fabric');
-    canvas2.dispose();
+    await canvas2.dispose();
     assert.equal(canvas2.getObjects().length, 0, 'dispose should clear canvas');
     assert.equal(canvas2.lowerCanvasEl, null, 'dispose should clear lowerCanvasEl');
     assert.equal(lowerCanvas.hasAttribute('data-fabric'), false, 'dispose should clear lowerCanvasEl data-fabric attr');
@@ -2003,11 +2004,11 @@
 
   QUnit.test('requestRenderAll and cancelRequestedRender', function(assert) {
     var canvas2 = new fabric.StaticCanvas();
-    assert.equal(canvas2.isRendering, undefined, 'no redering is in progress');
+    assert.equal(canvas2.nextRenderHandle, undefined, 'no redering is in progress');
     canvas2.requestRenderAll();
-    assert.notEqual(canvas2.isRendering, 0, 'a rendering is scehduled');
+    assert.notEqual(canvas2.nextRenderHandle, 0, 'a rendering is scehduled');
     canvas2.cancelRequestedRender();
-    assert.equal(canvas2.isRendering, 0, 'rendering cancelled');
+    assert.equal(canvas2.nextRenderHandle, 0, 'rendering cancelled');
   });
 
   // QUnit.test('backgroundImage', function(assert) {