Container.Remove() race causes crash

[arma7x]

Describe the bug:

• Execute Container.Remove(object) by loop over Container.Objects to remove all Object does not work as expected

To Reproduce:

Steps to reproduce the behaviour:

1. Click Populate Cards to populate dummy cards
2. Click Remove All Cards(bug) to reproduce the bug
3. Click Remove All Cards(temporary solution)

Example code:

https://go.dev/play/p/7wYM2bYBSaP

Device (please complete the following information):

• OS: Lubuntu
• Version: 18.04.6 LTS
• Go version: go1.17.7 linux/amd64
• Fyne version: v2 v2.1.2

[arma7x]

Sometime panic: runtime error: invalid memory address or nil pointer dereference if the Container.Objects was too big
https://go.dev/play/p/-68fye203eh

[Jacalz]

I’ve tried looking at this but I don’t understand why it is behaving the way it is. Something is strange with the container remove code. This is probably related to #2775.

We should obviously track down the cause of this issue, but I would just like to clarify something for future readers. For removing everything at once, the method used above is terribly inefficient. I’d suggest the use of one of the methods mentioned in https://yourbasic.org/golang/clear-slice/ instead. Using that on the container .Objects slice should be the best solution.

[arma7x]

maybe iterate range of current Container.Objects is bad approach

[Jacalz]

We should probably keep this open until the underlying issues have been fixed.

[arma7x]

I am not a native english speaker, but this is my explanation https://go.dev/play/p/Kv1G_WSsgWE

[andydotxyz]

It is much easier to set Container.Objects = nil to remove them all :)

[scmu1]

I'm not a english speaker, please forgive my english. Yesterday i found this bug too. Now i find bug reason.

1. Main thread in loop refresh all canvas objects and call Layout.Layout() refresh layout position and size
2. container.Remove() method manipulate c.Objects directly, c.Objects[len(c.Objects)-1] = nil set latest object nill, it cause main thread call Layout.Layout() get c.Objects latest nil object then panic

		copy(c.Objects[i:], c.Objects[i+1:])

		c.Objects[len(c.Objects)-1] = nil     //  latest object set to nil
		c.Objects = c.Objects[:len(c.Objects)-1]

I rewrite container.Remove() delete object logic and no nil pointer error

		copy(c.Objects[i:], c.Objects[i+1:])
		// set temp array remove object
		objs := make([]CanvasObject, len(c.Objects))
		for j, o := range c.Objects {
			objs[j] = o
		}

		objs[len(objs)-1] = nil
		objs = objs[:len(objs)-1]
		c.Objects = objs

[andydotxyz]

Ready on release/v2.1.x for 2.1.4