Alternating grid.Add() and grid.Remove() lets program crash

[midu-SA]

Describe the bug:

I create 100 buttons (stored in an array), which I alternatingly add into and remove from a grid, using grid.Add() and grid.Remove(). After a while the program crashes.

To Reproduce:

Steps to reproduce the behaviour:

Run the code provided below, where

• a grid is created
• 100 buttons are created, which are stored in an array
• an add-button is created to add these 100 buttons to the grid using grid.Add() method
• a remove-button is created to remove these 100 buttons from the grid using grid.Remove() method

Now use the add-button and the remove-button alternatingly a few times. After a while the program crashes. The crash does not happen always at the same step. But it always crashes when grid.Remove() is executed. It seems to be a memory problem which is not deterministic, e.g. the crash might happen when button no. 17 is removed, or no. 35 or no. 72, or ...

The output in a terminal is e.g.

...
i= 32
i= 31
i= 30
i= 29
i= 28
i= 27
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x58 pc=0x62679e]

goroutine 10 [running, locked to thread]:
fyne.io/fyne/v2/layout.(*gridLayout).MinSize(0xc0005c98d0, {0xc0000c4000, 0x1d, 0x6255a6})
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/layout/gridlayout.go:131 +0x2fe
fyne.io/fyne/v2.(*Container).MinSize(0xc0005c9849)
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/container.go:71 +0x72
fyne.io/fyne/v2/layout.(*boxLayout).MinSize(0xc0005c9849, {0xc0001712c0, 0x3, 0x0})
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/layout/boxlayout.go:140 +0x15d
fyne.io/fyne/v2.(*Container).MinSize(0x463c25)
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/container.go:71 +0x72
fyne.io/fyne/v2/internal/driver/glfw.(*glCanvas).MinSize(0xc0001219e0)
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/internal/driver/glfw/canvas.go:72 +0x9f
fyne.io/fyne/v2/internal/driver/common.(*Canvas).EnsureMinSize(0xc0001219e0)
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/internal/driver/common/canvas.go:72 +0x82
fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).repaintWindow.func1()
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/internal/driver/glfw/loop.go:193 +0x35
fyne.io/fyne/v2/internal/driver/glfw.(*window).RunWithContext(0xc000420200, 0xc00017be30)
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/internal/driver/glfw/window.go:1349 +0x4f
fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).repaintWindow(0xc00017be98, 0xc0006ccd00)
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/internal/driver/glfw/loop.go:192 +0x4a
fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).drawSingleFrame(0xc00017bf90)
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/internal/driver/glfw/loop.go:87 +0x1c5
fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).startDrawThread.func1()
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/internal/driver/glfw/loop.go:248 +0x1ba
created by fyne.io/fyne/v2/internal/driver/glfw.(*gLDriver).startDrawThread
        /home/opc/go/pkg/mod/fyne.io/fyne/v2@v2.1.2/internal/driver/glfw/loop.go:224 +0xef
exit status 2

Example code:

package main
import (
        "fyne.io/fyne/v2"
        "fyne.io/fyne/v2/app"
        "fyne.io/fyne/v2/container"
        "fyne.io/fyne/v2/layout"
        "fyne.io/fyne/v2/widget"
        "fmt"
)

var buttons []*fyne.Container
var grid    *fyne.Container
var button1, button2 *widget.Button

func main() {
        app := app.NewWithID("Test")
        w := app.NewWindow("Test")
        w.SetContent(makeUI())
        w.Resize(fyne.NewSize(500, 700))
        w.ShowAndRun()
}

func makeUI() fyne.CanvasObject {
    
     buttons = make ([]*fyne.Container, 100)
     for i:=0; i<100; i++ {
       nr:=i
       b := widget.NewButton (fmt.Sprintf ("%d", nr), func() {
          fmt.Println ("Button pressed")
       })
       buttons[nr] = fyne.NewContainerWithLayout(layout.NewMaxLayout(), b)
     }

    button1 = widget.NewButton ("Add", func() { 
        for i:=0; i<100; i++ {
            grid.Add(buttons[i])
        }
        button1.Disable()
        button2.Enable()
    })
    
    button2 = widget.NewButton ("Remove", func() { 
        for i:=99; i>=0; i-- {
            fmt.Println ("i=",i)
            grid.Remove(buttons[i])
        }
        button1.Enable()
        button2.Disable()
    })
    
    button2.Disable()
    grid = fyne.NewContainerWithLayout(layout.NewGridLayout(20)) 
    main_container := container.NewVBox (button1, button2, grid)
    return main_container
}

Device (please complete the following information):

• OS: Linux, Debian bullseye
• Version: 11
• Go version: 1.17.5
• Fyne version: 2.1.1

[andydotxyz]

Looks like we have missed some safety in the container add/remove functions

[andydotxyz]

Device (please complete the following information):

• OS:
• Version:
• Go version:
• Fyne version:

Please complete this information so the bug can be correctly triaged/labelled/reproduced. (I cannot replicate on v2.1.2 locally)

[midu-SA]

Hmm, this Info is at the end of my issue Report... However I was not sure whether to include this between the comment signs or not. I did it within, believing these might be needed for some kind of automatic processing... KR, Mike 13.02.2022 23:00:23 Andy Williams ***@***.***>:

*Device (please complete the following information):*

* > *OS:* * > *Version:* * > *Go version:* * > *Fyne version:*

…

Please complete this information so the bug can be correctly triaged/labelled/reproduced. — Reply to this email directly, view it on GitHub[#2775 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AP3FEE6GM7M76OXBWSLKJBTU3ASXNANCNFSM5OJRPVWA]. Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub]. You are receiving this because you authored the thread. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEQAAABECAYAAAA4E5OyAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAoSURBVHic7cEBAQAAAIIg/69uSEABAAAAAAAAAAAAAAAAAAAAAACPBkiEAAHzeIrHAAAAAElFTkSuQmCC###24x24:true###][Verfolgungsbild][https://github.com/notifications/beacon/AP3FEE67GM34WVQJVA6IY5LU3ASXNA5CNFSM5OJRPVWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHXSXTMQ.gif]

[midu-SA]

Issue updated. Device information is now modified.

[andydotxyz]

Was this closed by mistake @midu-SA ?

[midu-SA]

No, I am mit Aware that I closed it... 15.02.2022 17:52:07 Andy Williams ***@***.***>:

…

Was this closed by mistake @midu-SA[https://github.com/midu-SA] ? — Reply to this email directly, view it on GitHub[#2775 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AP3FEE2FWED65MTCZ2ZJJ3LU3KADLANCNFSM5OJRPVWA]. Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub]. You are receiving this because you were mentioned. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEQAAABECAYAAAA4E5OyAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAoSURBVHic7cEBAQAAAIIg/69uSEABAAAAAAAAAAAAAAAAAAAAAACPBkiEAAHzeIrHAAAAAElFTkSuQmCC###24x24:true###][Verfolgungsbild][https://github.com/notifications/beacon/AP3FEE36ZHBZHTIVYGVB3KTU3KADLA5CNFSM5OJRPVWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHYCQT4Y.gif]

[midu-SA]

gridlayout.go.txt
log.txt
readme.txt

[midu-SA]

Hi Fyne Team

I uploaded 3 files:

a) modfied file layout/gridlayout.go with added debug information
b) readme
c) log file

a)
I modified your file ".../go/src/fyne.io/fyne.2.1.1/v2/layout/gridlayout.go" as follows:

• func countRows

  • when entered, I print no. of objects
  • when returning, I print no. of objects

• func MinSize

  • when entered, I print no. of objects,
  • then I wait 200 msec
  • then I print no. of objects again (is same number of course)

c)
As you can see from the log, during the waiting of 200 msec, the func countRows is called
several times from a different program. Obviously because the 100 buttons are removed from the grid.
This can be seen because the number of objects decreases with every call of countRows..

However, after the wating of 200 msec the func MinSize still assumes that the objects are available.
The proc then crashes.

Kind regards, Mike

[midu-SA]

Hi Fyne-Team,
I checked your code and found out the following:
if you remove an item from the grid, you set the object to nil. Hence, you may end up with some nil values within the objects slice.
However, in file layout/gridlayout.go, you have several places, where you loop over all objects. Within the loop you check if the child is visible or not. This however crashes, if the child is nil.
In the attached file I added in 3 places: if child==nil {continue}
I did this right before you check if the child is visible.
With this change, the sample code above works.

Please check if a similar change is needed in file container.go. In this file you also find several loops over all objects, without checking if the object is nil.
KR, Mike
gridlayout.go.txt

[midu-SA]

I forgot to mention: I marked my changes in gridlayout.go with "//mdu"

[Jacalz]

if you remove an item from the grid, you set the object to nil. Hence, you may end up with some nil values within the objects slice.

Just for posterity, I will copy over my answer from Slack. You won't end up with nil objects in the slice. It is the item that is being removed from the slice that is being set to nil to make sure that the GC runs on the element and thus doesn't cause a memory leak. The nil objects are coming from somewhere else.

We probably want to add some nil checks in certain places but I think we need to track down and fix where they are coming from first.

[midu-SA]

Yes, I also noticed that my argumentation was wrong. The nil objects come from somewhere else...
I can see with a test print command, that in file gridlayout.go - within the loop over the objects - it happens that an object is nil. I added the test print command before the check if the object is visible.

[Jacalz]

I've gone through and cleaned up the testing code to be a bit simpler. I don't think there are any bugs in it, (at least not as far as I can see). The crash also still happens with #2898, so the issue does not occur due to adding any nil items. Looks like we need to debug a bit more as to where they are coming from.

package main

import (
	"strconv"
	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/app"
	"fyne.io/fyne/v2/container"
	"fyne.io/fyne/v2/widget"
)

func main() {
	app := app.NewWithID("Test")
	w := app.NewWindow("Test")
	w.SetContent(makeUI())
	w.Resize(fyne.NewSize(700, 400))
	w.ShowAndRun()
}

func makeUI() fyne.CanvasObject {
	var grid *fyne.Container
	var button1, button2 *widget.Button

	const buttonsLength = 50
	
	buttons := make([]fyne.CanvasObject, buttonsLength)
	for i := range buttons {
		buttons[i] = widget.NewButton(strconv.Itoa(i), nil)
	}

	button1 = widget.NewButton("Add", func() {
		for i := range buttons {
			grid.Add(buttons[i])
		}
		button1.Disable()
		button2.Enable()
	})

	button2 = widget.NewButton("Remove", func() {
		for i := buttonsLength - 1; i >= 0; i-- {
			grid.Remove(buttons[i])
		}
		button1.Enable()
		button2.Disable()
	})

	button2.Disable()
	grid = container.NewGridWithColumns(20)
	return container.NewVBox(button1, button2, grid)
}

EDIT: Managed to slim the code down a bit more. I also added a constant to more easily change the amount of buttons. I managed to replicate it with 50 buttons now. Below that, I did not experience the crash.

[midu-SA]

Do I understand correctly that with your modified code, you can verify the bug?

1. As I wrote above, I noticed the crash only when items (here: buttons) were deleted
   (not when items were added),
2. I see with a simple print command that sometimes objects are nil.
   In file "gridlayout.go", func "MinSize", I added the following code as the first line within the for-loop :
   if child == nil {fmt.Println ("gridlayout.go, MinSize: YYYYY"); continue}
   This text is sometimes written to stdout, when I press the button to remove the 100 items.

Thanks for working on this issue!

[Jacalz]

Yes, I can replicate the issue both with the original code and my simplified version. I see the exact same thing with it crashing on removing the items. I ran out of time today and didn't get to do any debugging.

[Jacalz]

I tried to debug this in VSCode with delve, but it didn't seem to work very well. It did not show any callstack or local variable and when the crash occurred, it just froze the application without any output or useful stack traces. Someone with GoLand installed might have better luck with the debugging hopefully.

[Jacalz]

Closing as duplicate of #2826. I know this opened before, but Ithe issue s tracked down in the other issue.

[Jacalz]

This should now be fixed once #2844 lands.