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(Canvas): dispose race condition #7885

Merged
merged 1 commit into from
Apr 19, 2022
Merged

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Apr 18, 2022

calling static dispose before interactive dispose to prevent race condition as described in #7876
should close #7876 and some other strange bugs.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 18, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.92 |    75.35 |   86.25 |   82.63 |                                               
 fabric.js |   82.92 |    75.35 |   86.25 |   82.63 | ...,30456,30530,30541-30606,30729,30828,31064 
-----------|---------|----------|---------|---------|-----------------------------------------------

@fabricjs fabricjs deleted a comment from github-actions bot Apr 18, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 18, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.97 |    75.38 |   86.25 |   82.68 |                                               
 fabric.js |   82.97 |    75.38 |   86.25 |   82.68 | ...,30456,30530,30541-30606,30729,30828,31064 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented Apr 18, 2022

Code Coverage Summary

> fabric@5.1.0 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   82.91 |    75.31 |   86.25 |   82.62 |                                               
 fabric.js |   82.91 |    75.31 |   86.25 |   82.62 | ...,30456,30530,30541-30606,30729,30828,31064 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member

asturur commented Apr 18, 2022

The PR seems fine to me, and seem innocent enough to merge it without digging deeper.
But would be nice to understand where the race condition was and caused by which operation.
Is because only the staticCanvas dispose call for a cancel render?

@asturur
Copy link
Member

asturur commented Apr 18, 2022

Is approved, so you can merge whenever. But try to explain

@ShaMan123
Copy link
Contributor Author

Is because only the staticCanvas dispose call for a cancel render?

Yes. What happened was that Canvas#dispose nullified topContext and there was a small gap of time that rendering could occur before StaticCanvas#dispose terminated requested rendering.

@ShaMan123 ShaMan123 merged commit 4b3f9a3 into master Apr 19, 2022
@ShaMan123 ShaMan123 deleted the fix-canvas-dispose-racing branch April 19, 2022 00:21
elbunuelo added a commit to elbunuelo/fabric.js that referenced this pull request Oct 13, 2022
k1w1 added a commit to aha-app/fabric.js that referenced this pull request Dec 27, 2022
…d-properties-null-reading-save

Implement Fabric PR fabricjs#7885
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error "Cannot read properties of null (reading 'save')" for renderTopLayer method calling
2 participants