Disposing Fabric object within 400 milliseconds after mouse up causes exception

[alex-shevchenko]

Version

1.7.16

Test Case

http://jsfiddle.net/Da7SP/578/

Steps to reproduce

Disposing Fabric object on mouse:up event or within 400 milliseconds afterward will cause an exception in timeout set by _onMouseUp handler, that will try to add event listener to destroyed element.

[asturur]

thabks for the info, may be related to a bug i need to check.

[asturur]

thanks fixed!

[kingpalethe]

I am having this exact same issue, I can reproduce it with this same fiddle:

http://jsfiddle.net/Da7SP/578/

This fiddle seems to be using 2.6, the most recent version of Fabric.js

I get the same error message in my application

fabric.js:7368 Uncaught TypeError: Cannot read property 'clearRect' of null
    at klass.clearContext (fabric.js:7368)
    at klass.renderTop (fabric.js:9624)
    at klass.__onMouseUp (fabric.js:11297)
    at klass._onMouseUp (fabric.js:11178)
clearContext @ fabric.js:7368
renderTop @ fabric.js:9624
__onMouseUp @ fabric.js:11297
_onMouseUp @ fabric.js:11178

Here is my use case.

I'm building an Electron app where I am persisting the state of the Fabric canvas to disk, and so far my strategy is to destroy and rebuild the canvas each time the user makes a drawing. Since I am persisting the data, I'd rather re-load it each time an object is added instead of something merging the added object with the canvas already in memory.

Starting from page load, here is my code.

When my page loads, I run this function.

    refreshDrawingCanvas() {
        self.disposeDrawingCanvas();
        self.drawingFabricCanvas = {};
        const canvas = new fabric.Canvas(
            "drawing-canvas", // hard coding ID here!
            {
                width: self.canvasWidth,
                height: self.canvasHeight,
                centeredScaling: true,
                containerClass: self.drawingCanvasContainerClass,
                fireRightClick: true
            }
        );
        self.drawingFabricCanvas = canvas;
        self.addEventListenersToCanvas()
        self.addExistingDrawingsToCanvas()
    },

That function uses these functions:

Dispose the canvas if it exists for some reason:

    disposeDrawingCanvas() {
        if (
            self.drawingFabricCanvas &&
            typeof self.drawingFabricCanvas.dispose === "function"
        ) {
            console.log("disposing drawing canvas")
            self.drawingFabricCanvas.dispose();
        }
    },

Add event listeners:

addEventListenersToCanvas() {
        const canvas = self.drawingFabricCanvas
        canvas.on('mouse:down', (mouseDownEvent) => {
            if (mouseDownEvent.button == 1) {

                self.resetNewDrawnObject()

                console.log("left button click")
                self.setLeftMouseToDown()
                self.initDrawing(mouseDownEvent)
            } else if (mouseDownEvent.button == 3) {
                console.log("right button click")
            }
        })

        canvas.on('mouse:move', function (mouseMoveEvent) {
            if (self.leftMouseIsDown) {
                self.continueDrawing(mouseMoveEvent)

            } else if (self.rightMouseIsDown) {

                console.log("handle mouse movement while right mouse is down")
            }
        });

        canvas.on('mouse:up', (mouseUpEvent) => {
            console.log("calling mouse up")
            self.setLeftMouseToUp()
            self.persistDrawing()
            self.resetNewDrawnObject()
            self.refreshDrawingCanvas()

        })

    },

Here are the functions triggered by the event listeners:

    initDrawing(mouseDownEvent) {
        console.log(mouseDownEvent.e)

        self.drawingFabricCanvas.selection = false;
        const pointer = self.drawingFabricCanvas.getPointer(mouseDownEvent.e)
        console.log(pointer.x)
        console.log(pointer.y)
        self.newDrawnObjectOrigX = pointer.x
        self.newDrawnObjectOrigY = pointer.y
        if (self.parentComposition.currentDrawingTool == 'circle') {
            self.newDrawnObject = new fabric.Circle({
                left: pointer.x,
                top: pointer.y,
                radius: 2,
                strokeWidth: 5,
                stroke: self.parentComposition.currentDrawingColor,
                fill: 'rgba(0,0,0,0)',
                selectable: false,
                originX: 'center',
                originY: 'center'
            })
        }
        console.log("new drawn object", self.newDrawnObject)
        self.drawingFabricCanvas.add(self.newDrawnObject)
        self.drawingFabricCanvas.setActiveObject(self.newDrawnObject)
        // persist new Drawing
    },

    continueDrawing(mouseMoveEvent) {
        console.log("handle mouse movement while left mouse is down")
        const pointer = self.drawingFabricCanvas.getPointer(mouseMoveEvent.e);
        if (self.parentComposition.currentDrawingTool == 'circle') {
            self.newDrawnObject.set({ radius: Math.abs(self.newDrawnObjectOrigX - pointer.x) });
        }
        self.newDrawnObject.setCoords()
        self.drawingFabricCanvas.renderAll()

    },
    persistDrawing() {
        const id = shortid.generate();
        console.log("in persist with this object", self.newDrawnObject)
        const left = self.newDrawnObject.left
        const top = self.newDrawnObject.top
        console.log("left", left, "top", top)

        const position = self.parentComposition.drawings.length
        const color = self.parentComposition.currentDrawingColor
        const drawingTool = self.parentComposition.currentDrawingTool
        const leftPercentage = left / self.canvasWidth
        console.log("leftPercentage", leftPercentage)
        const topPercentage = top / self.canvasHeight
        console.log("topPercentage", topPercentage)
        const radius = self.newDrawnObject.radius
        const strokeWidth = self.newDrawnObject.strokeWidth
        const fill = self.newDrawnObject.fill
        const originX = self.newDrawnObject.originX
        const originY = self.newDrawnObject.originY

        const drawing = Drawing.create({
            id, position, color, drawingTool, topPercentage, leftPercentage, radius,
            strokeWidth, fill, originX, originY
        });
        self.parentComposition.addDrawing(drawing)
    }
});

   resetNewDrawnObject() {
        self.newDrawnObject.selectable = false
        self.newDrawnObject.evented = false
        self.newDrawnObject = {}
        self.newDrawnObjectOrigX = null
        self.newDrawnObjectOrigY = null
    },

The error I am flagging in in this issue goes away if I remove this line from the "mouse:up" listener

self.refreshDrawingCanvas()

or, more specifically, if I stop calling this on "mouse:up"

self.disposeDrawingCanvas();

So it seems clear that the problem is that even ###after ### the canvas has been disposed with drawingFabricCanvas.dispose();, this is still running: _onMouseUp @ fabric.js:11178.... possibly because that runs on some kind of timeout and cannot be cancelled?

Is there an alternative to canvas.dispose() that guarantees that there won't still be events firing on the canvas after it is disposed?

[asturur]

ok i see the point now.
No there is no a way to stop the event from finishing and i do not think we should do it.
But since you are disposing an deleting all the contexts, we could safeguard the render code to avoid move on if there are no more valid contexts.

I assume you could avoid disposing the canvas and just clear it or reload the items on top of it, i m not sure how the app works and i do not know if this is the best pattern.

[kingpalethe]

@asturur thanks. It looks like I should, as you say, not attempt to .dispose() the Fabric canvas after each drawing, but instead should be using .clear()

[asturur]

still disposing would be legit. Is also ok to assume after an event there is dev code that has been run and something should be checked again. i want to see if it can be done fullproof

…

On Mon, 21 Jan 2019, 16:07 kingpalethe ***@***.*** wrote: @asturur <https://github.com/asturur> thanks. It looks like I should, as you say, not attempt to .dispose() the Fabric canvas after each drawing, but instead should be using .clear() — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4117 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABI4QI2ewE7bJfQ7cUfOeLrBPALfGgcUks5vFdevgaJpZM4Ofn5p> .

[Vuliniar]

@asturur this is still happening. One way is to check if contextTop exist, but still that's not always going to be accurate. Should we add something like state/status: 'painted', 'disposing' or something similar?

[asturur]

does it happen on the last version?

[asturur]

i think we need to go point by point and make the code safer.
Or explain clearly how the developer can avoid falling in this kind of traps