-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Update canvas.go - Length check added to fix panic out of range #4532
base: develop
Are you sure you want to change the base?
Conversation
Hmm, this seems like a band-aid fix where there's actually a deeper issue that needs to be solved. Likely there is some missing locking somewhere or else an assumption is being violated in how we're using the overlay stack. |
Yes I believe the same. In my case I have almost no experience in the internal workings of Fyne, I have been using it for several months, but I do not have enough knowledge to find the underlying problem. I recognize that the solution I propose is superficial and attacks the consequence, which may not be ideal. Maybe someone with more knowledge can give us a hand with this kickoff that I'm trying to give or maybe they are already analyzing it. |
By just looking at it there does seem to be some sort of race. I had thought it might be an out of order free (stack assumes top most will go first) but I can't see how that is the case here.
Perhaps you can share this scenario so that someone can replicate it and understand what is going on to help out with the PR? |
Hello Andy, I am going to try to extract a simplified code that allows me to replicate the problem and if I can do so, I will share it here. In short, my code executes asynchronously in an anonymous function a "ShowPopup()" and then a "defer popUp.Hide()" based on the canvas of a window, which between the execution of "Show" and "Hide" performs a " dialog.Show()". The error seems to occur most of the time, but not always, which is a symptom of a possible race condition. These days I'm going to be a bit busy, so it may take me a while to replicate it in a simple way. Thanks for the answers |
If you are hiding/showing two different overlays in different threads there is clearly a race condition in your code. However it should only result in the topmost being undefined and not a crash in Fyne code. |
Hello, I have managed to replicate the race condition problem, forcing the execution of asynchronous threads. In the case of having "Windows" operating system, repeatedly press "Ctrl + F" and eventually the problem happens: Example of console output:
The code to replicate:
|
In that code you have race conditions, there is no way that you can expect the
|
@@ -582,8 +582,10 @@ func (o *overlayStack) add(overlay fyne.CanvasObject) { | |||
func (o *overlayStack) remove(overlay fyne.CanvasObject) { | |||
o.OverlayStack.Remove(overlay) | |||
overlayCount := len(o.List()) | |||
o.renderCaches[overlayCount] = nil // release memory reference to removed element | |||
o.renderCaches = o.renderCaches[:overlayCount] | |||
if len(o.renderCaches) > overlayCount { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "happy path" should be left aligned - i.e. this should really be a check for if the count is too low then returning early.
In general this sort of safety is probably a good thing as we shouldn't really panic even if developer triggers a bad path.
Description:
I recently started using PopUp messages in my app, and in a specific scenario, I encountered a panic due to an index out of range error when using a PopUp simultaneously with a Dialog in the same window.
The fix is simple. Despite the questionable usage of PopUps and Dialogs in this particular case, I believe that adding a length check contributes to stability improvement.
Panic message:
panic: runtime error: index out of range [1] with length 1
goroutine 27 [running]:
fyne.io/fyne/v2/internal/driver/common.(*overlayStack).remove(0xc002a8f710, {0x7ff676ac6118, 0xc00bb8a900})
C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/internal/driver/common/canvas.go:585 +0xd7
fyne.io/fyne/v2/internal/driver/common.(*overlayStack).Remove(0xc002a8f710, {0x7ff676ac6118, 0xc00bb8a900})
C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/internal/driver/common/canvas.go:574 +0x12e
fyne.io/fyne/v2/widget.(*PopUp).Hide(0xc00bb8a900)
C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/widget/popup.go:28 +0x56
Checklist: