fix(): dispose-render concurrency

src/canvas.class.ts

@@ -511,6 +511,10 @@ import { Point } from './point.class';
      * @chainable
      */
     renderAll: function () {
+      this.cancelRequestedRender();
+      if (this.destroyed) {
+        return;
+      }
       if (this.contextTopDirty && !this._groupSelector && !this.isDrawingMode) {
         this.clearContext(this.contextTop);
         this.contextTopDirty = false;
@@ -519,9 +523,8 @@ import { Point } from './point.class';
         this.renderTopLayer(this.contextTop);
         this.hasLostContext = false;
       }
-      var canvasToDrawOn = this.contextContainer;
       !this._objectsToRender && (this._objectsToRender = this._chooseObjectsToRender());
-      this.renderCanvas(canvasToDrawOn, this._objectsToRender);
+      this.renderCanvas(this.contextContainer, this._objectsToRender);
       return this;
     },
 
@@ -1313,17 +1316,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 { runningAnimations } from '../util/animation_registry';
       // 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

@@ -2,6 +2,7 @@
 import { config } from './config';
 import { VERSION } from './constants';
 import { Point } from './point.class';
+import { requestAnimFrame } from './util/animate';
 import { removeFromArray } from './util/internals';
 import { pick } from './util/misc/pick';
 
@@ -675,8 +676,11 @@ import { pick } from './util/misc/pick';
      * @chainable
      */
     renderAll: function () {
-      var canvasToDrawOn = this.contextContainer;
-      this.renderCanvas(canvasToDrawOn, this._objects);
+      this.cancelRequestedRender();
+      if (this.destroyed) {
+        return;
+      }
+      this.renderCanvas(this.contextContainer, this._objects);
       return this;
     },
 
@@ -691,7 +695,7 @@ import { pick } from './util/misc/pick';
      * @chainable
      */
     renderAndReset: function() {
-      this.isRendering = 0;
+      this.nextRenderHandle = 0;
       this.renderAll();
     },
 
@@ -703,8 +707,8 @@ import { pick } from './util/misc/pick';
      * @chainable
      */
     requestRenderAll: function () {
-      if (!this.isRendering) {
-        this.isRendering = fabric.util.requestAnimFrame(this.renderAndResetBound);
+      if (!this.nextRenderHandle && !this.disposed && !this.destroyed) {
+        this.nextRenderHandle = requestAnimFrame(this.renderAndResetBound);
       }
       return this;
     },
@@ -734,9 +738,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 +751,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);
       ctx.imageSmoothingEnabled = this.imageSmoothingEnabled;
+      // node-canvas
       ctx.patternQuality = 'best';
       this.fire('before:render', { ctx: ctx, });
       this._renderBackground(ctx);
@@ -778,6 +787,11 @@ 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 +1623,49 @@ 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<boolean>} a promise resolving to `true` once the canvas has been destroyed or to `false` if the canvas has was already destroyed
+     * @throws if aborted by a consequent call
      */
     dispose: function () {
-      // cancel eventually ongoing renders
-      if (this.isRendering) {
-        fabric.util.cancelAnimFrame(this.isRendering);
-        this.isRendering = 0;
-      }
+      this.disposed = true;
+      return new Promise<boolean>((resolve, reject) => {
+        const task = () => {
+          this.destroy();
+          resolve(true);
+        }
+        task.kill = reject;
+        if (this.__cleanupTask) {
+          this.__cleanupTask.kill('aborted');
+        }
+
+        if (this.destroyed) {
+          resolve(false);
+        }
+        else 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 +1692,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

@@ -196,7 +196,7 @@
           if ((!isOK && QUnit.debugVisual) || QUnit.recreateVisualRefs) {
             generateGolden(getGoldeName(golden), renderedCanvas);
           }
-          fabricCanvas.dispose();
+          await fabricCanvas.dispose();
           done();          
         });
       });

test/unit/canvas.js

@@ -2083,72 +2083,7 @@
     assert.equal(aGroup._objects[3], circle2);
   });
 
-  QUnit.test('dispose', function(assert) {
-    //made local vars to do not dispose the external canvas
-    var el = fabric.document.createElement('canvas'),
-        parentEl = fabric.document.createElement('div'),
-        wrapperEl, lowerCanvasEl, upperCanvasEl;
-    el.width = 200; el.height = 200;
-    parentEl.className = 'rootNode';
-    parentEl.appendChild(el);
-
-    fabric.config.configure({ devicePixelRatio: 1.25 });
-
-    assert.equal(parentEl.firstChild, el, 'canvas should be appended at partentEl');
-    assert.equal(parentEl.childNodes.length, 1, 'parentEl has 1 child only');
-
-    el.style.position = 'relative';
-    var elStyle = el.style.cssText;
-    assert.equal(elStyle, 'position: relative;', 'el style should not be empty');
-
-    var canvas = new fabric.Canvas(el, { enableRetinaScaling: true, renderOnAddRemove: false });
-    wrapperEl = canvas.wrapperEl;
-    lowerCanvasEl = canvas.lowerCanvasEl;
-    upperCanvasEl = canvas.upperCanvasEl;
-    assert.equal(parentEl.childNodes.length, 1, 'parentEl has still 1 child only');
-    assert.equal(wrapperEl.childNodes.length, 2, 'wrapper should have 2 children');
-    assert.equal(wrapperEl.tagName, 'DIV', 'We wrapped canvas with DIV');
-    assert.equal(wrapperEl.className, canvas.containerClass, 'DIV class should be set');
-    assert.equal(wrapperEl.childNodes[0], lowerCanvasEl, 'First child should be lowerCanvas');
-    assert.equal(wrapperEl.childNodes[1], upperCanvasEl, 'Second child should be upperCanvas');
-    assert.equal(canvas._originalCanvasStyle, elStyle, 'saved original canvas style for disposal');
-    assert.notEqual(el.style.cssText, canvas._originalCanvasStyle, 'canvas el style has been changed');
-    if (!fabric.isLikelyNode) {
-      assert.equal(parentEl.childNodes[0], wrapperEl, 'wrapperEl is appendend to rootNode');
-    }
-    //looks like i cannot use parentNode
-    //equal(wrapperEl, lowerCanvasEl.parentNode, 'lowerCanvas is appended to wrapperEl');
-    //equal(wrapperEl, upperCanvasEl.parentNode, 'upperCanvas is appended to wrapperEl');
-    //equal(parentEl, wrapperEl.parentNode, 'wrapperEl is appendend to rootNode');
-    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');
-    canvas.add(makeRect(), makeRect(), makeRect());
-    canvas.item(0).animate('scaleX', 10);
-    assert.equal(fabric.runningAnimations.length, 1, 'should have a running animation');
-    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');
-    assert.equal(parentEl.childNodes.length, 1, 'parent has always 1 child');
-    if (!fabric.isLikelyNode) {
-      assert.equal(parentEl.childNodes[0], lowerCanvasEl, 'canvas should be back to its firstChild place');
-    }
-    assert.equal(canvas.wrapperEl, null, 'wrapperEl should be deleted');
-    assert.equal(canvas.upperCanvasEl, null, 'upperCanvas should be deleted');
-    assert.equal(canvas.lowerCanvasEl, null, 'lowerCanvasEl should be deleted');
-    assert.equal(canvas.cacheCanvasEl, null, 'cacheCanvasEl should be deleted');
-    assert.equal(canvas.contextTop, null, 'contextTop should be deleted');
-    assert.equal(canvas.contextCache, null, 'contextCache should be deleted');
-    assert.equal(canvas._originalCanvasStyle, undefined, 'removed original canvas style');
-    assert.equal(el.style.cssText, elStyle, 'restored original canvas style');
-    assert.equal(el.width, 200, 'restored width');
-    assert.equal(el.height, 200, 'restored height');
-
-  });
-
-  QUnit.test('dispose + set dimensions', function (assert) {
-    //made local vars to do not dispose the external canvas
+  QUnit.test('set dimensions', async function (assert) {
     var el = fabric.document.createElement('canvas'),
       parentEl = fabric.document.createElement('div');
     el.width = 200; el.height = 200;
@@ -2170,56 +2105,14 @@
     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) {
-  //   function invokeEventsOnCanvas() {
-  //     // nextSibling because we need to invoke events on upper canvas
-  //     simulateEvent(canvas.getElement().nextSibling, 'mousedown');
-  //     simulateEvent(canvas.getElement().nextSibling, 'mouseup');
-  //     simulateEvent(canvas.getElement().nextSibling, 'mousemove');
-  //   }
-  //   var assertInvocationsCount = function() {
-  //     var message = 'event handler should not be invoked after `dispose`';
-  //     assert.equal(handlerInvocationCounts.__onMouseDown, 1);
-  //     assert.equal(handlerInvocationCounts.__onMouseUp, 1);
-  //     assert.equal(handlerInvocationCounts.__onMouseMove, 1);
-  //   };
-
-  //   assert.ok(typeof canvas.dispose === 'function');
-  //   canvas.add(makeRect(), makeRect(), makeRect());
-
-  //   var handlerInvocationCounts = {
-  //     __onMouseDown: 0, __onMouseUp: 0, __onMouseMove: 0
-  //   };
-
-  //   // hijack event handlers
-  //   canvas.__onMouseDown = function() {
-  //     handlerInvocationCounts.__onMouseDown++;
-  //   };
-  //   canvas.__onMouseUp = function() {
-  //     handlerInvocationCounts.__onMouseUp++;
-  //   };
-  //   canvas.__onMouseMove = function() {
-  //     handlerInvocationCounts.__onMouseMove++;
-  //   };
-
-  //   invokeEventsOnCanvas();
-  //   assertInvocationsCount();
-
-  //   canvas.dispose();
-  //   assert.equal(canvas.getObjects().length, 0, 'dispose should clear canvas');
-
-  //   invokeEventsOnCanvas();
-  //   assertInvocationsCount();
-  // });
-
   QUnit.test('clone', function(assert) {
     var done = assert.async();
     assert.ok(typeof canvas.clone === 'function');