Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix() safeguard access to possibly undefined canvas #8722

Merged
merged 2 commits into from
Feb 27, 2023

Conversation

Ch1sKey
Copy link

@Ch1sKey Ch1sKey commented Feb 21, 2023

Motivation

Fix unguarded reference for object.canvas

Description

this.canvas.getRetinaScaling() call can cause TypeError in current release in some cases. It happened with us few times.
Would be really helpful to get this fix as soon as possible. I saw it already fixed in @beta, but I don't see it coming at the nearest future.

Changes

Enhance .drawControls method with guard for .getRetinaScaling call. Set default retinaScaling to 1.

In Action

Smallest example to represent an issue. This is a bit far-fetched but represents edge cases which could happen in huge variety of race conditions.

Steps: Try to move object on canvas.
https://codesandbox.io/s/fabricjs-playground-forked-hpodkw

@ShaMan123
Copy link
Contributor

note for v6: maybe we want to decide which properties (e.g. canvas) should not be public

@asturur
Copy link
Member

asturur commented Feb 25, 2023

So i think the reproduction case is definetely a bad, meaning if you do that, you get the error.
I m ok safeguarding this, even if i would have preferred to see a real use case.

canvas shouldn't be private, because many times you may have access to an object and not its own canvas, and you use the reference to find the canvas.

The doc should really say that should be considered read only

@ShaMan123
Copy link
Contributor

I m ok safeguarding this, even if i would have preferred to see a real use case.

Remember we solved this with the concurrency issue? dispose vs. destroy
So not to overkill efforts

@asturur
Copy link
Member

asturur commented Feb 27, 2023

Yes i was just wondering if there was something else around

@asturur asturur changed the title Fix ref for possibly undefined canvas Fix() safeguard access to possibly undefined canvas Feb 27, 2023
@asturur asturur merged commit 8f53632 into fabricjs:5.x Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants